The library requires samplerAnisotropy for SamplerVulkan, which nothing uses any more #168

New issue

Open

opened 2026-10-31 14:45:39 +00:00 by jorijnvdgraaf · 0 comments

jorijnvdgraaf commented

2026-10-31 14:45:39 +00:00

Owner

Summary

Device::Initialize puts samplerAnisotropy among the library's own requirements because "SamplerVulkan turns anisotropic filtering on" (implementations/Crafter.Graphics-Device.cpp:977-978). Since 6e94cc6 replaced Sponza's sampler, nothing in the library, the tests or the examples uses SamplerVulkan, so the library requires a feature for a class that is only exported (interfaces/Crafter.Graphics.cppm:34). Few GPUs lack the feature, so this is mostly dead code and a needless requirement. Line numbers refer to 670df31.

Details

interfaces/Crafter.Graphics-SamplerVulkan.cppm:17-46: a template on a pixel type it does not use. Its constructor makes a linear, repeating sampler with anisotropyEnable = VK_TRUE and maxAnisotropy = 16 (:30-31), not clamped to the device's maxSamplerAnisotropy.
It has no destructor, so its VkSampler is never released; the other handle owners release theirs through Device::EnqueueDeletion.
The samplers the library and the tests do make set maxAnisotropy = 1 (implementations/Crafter.Graphics-UI.cpp:455, tests/UIFallbackParity/main.cpp:125).
The README says the features the library enables are the ones its own binding model and shaders need, and the rest are the application's to ask for.

Suggested fix

Remove SamplerVulkan and the requirement. Or keep the class, give it deferred deletion and the clamp, and move anisotropy to the features an application asks for, with the class falling back to no anisotropy where it is off.

## Summary `Device::Initialize` puts `samplerAnisotropy` among the library's own requirements because "SamplerVulkan turns anisotropic filtering on" (`implementations/Crafter.Graphics-Device.cpp:977-978`). Since 6e94cc6 replaced Sponza's sampler, nothing in the library, the tests or the examples uses `SamplerVulkan`, so the library requires a feature for a class that is only exported (`interfaces/Crafter.Graphics.cppm:34`). Few GPUs lack the feature, so this is mostly dead code and a needless requirement. Line numbers refer to `670df31`. ## Details - `interfaces/Crafter.Graphics-SamplerVulkan.cppm:17-46`: a template on a pixel type it does not use. Its constructor makes a linear, repeating sampler with `anisotropyEnable = VK_TRUE` and `maxAnisotropy = 16` (`:30-31`), not clamped to the device's `maxSamplerAnisotropy`. - It has no destructor, so its `VkSampler` is never released; the other handle owners release theirs through `Device::EnqueueDeletion`. - The samplers the library and the tests do make set `maxAnisotropy = 1` (`implementations/Crafter.Graphics-UI.cpp:455`, `tests/UIFallbackParity/main.cpp:125`). - The README says the features the library enables are the ones its own binding model and shaders need, and the rest are the application's to ask for. ## Suggested fix Remove `SamplerVulkan` and the requirement. Or keep the class, give it deferred deletion and the clamp, and move anisotropy to the features an application asks for, with the class falling back to no anisotropy where it is off.