Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulkan sampler fix #6001

Closed

Conversation

martin-ejdestig
Copy link
Contributor

Do not set VkDescriptorSetLayoutBinding::pImmutableSamplers

From https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkDescriptorSetLayoutBinding.html :

"pImmutableSamplers affects initialization of samplers. If descriptorType specifies a VK_DESCRIPTOR_TYPE_SAMPLER or VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER type descriptor, then pImmutableSamplers can be used to initialize a set of immutable samplers. Immutable samplers are permanently bound into the set layout and must not be changed; updating a VK_DESCRIPTOR_TYPE_SAMPLER descriptor with immutable samplers is not allowed and updates to a VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER descriptor with immutable samplers does not modify the samplers (the image views are updated, but the sampler updates are ignored)."

So setting it means the same sampler will be used for all textures, basically making sampler argument to ImGui_ImplVulkan_AddTexture() useless.

Fixes #5502 .

(Also removes some dead code, see first commit, to not have to make same fix in two places.)

Sampler, descriptor set layout and pipeline layout are created in exact
same way directly in ImGui_ImplVulkan_CreateDeviceObjects(). The removed
functions are local and only has call chain that starts in
ImGui_ImplVulkan_CreateDeviceObjects(), so will always do early return.
From https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkDescriptorSetLayoutBinding.html :

"pImmutableSamplers affects initialization of samplers. If descriptorType
specifies a VK_DESCRIPTOR_TYPE_SAMPLER or
VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER type descriptor, then
pImmutableSamplers can be used to initialize a set of immutable samplers.
Immutable samplers are permanently bound into the set layout and must not
be changed; updating a VK_DESCRIPTOR_TYPE_SAMPLER descriptor with
immutable samplers is not allowed and updates to a
VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER descriptor with immutable
samplers does not modify the samplers (the image views are updated, but
the sampler updates are ignored)."

So setting it means the same sampler will be used for all textures,
basically making sampler argument to ImGui_ImplVulkan_AddTexture() useless.

Fixes ocornut#5502 .
ocornut pushed a commit that referenced this pull request Jan 2, 2023
…6001)

Sampler, descriptor set layout and pipeline layout are created in exact
same way directly in ImGui_ImplVulkan_CreateDeviceObjects(). The removed
functions are local and only has call chain that starts in
ImGui_ImplVulkan_CreateDeviceObjects(), so will always do early return.
ocornut pushed a commit that referenced this pull request Jan 2, 2023
…6001)

Sampler, descriptor set layout and pipeline layout are created in exact
same way directly in ImGui_ImplVulkan_CreateDeviceObjects(). The removed
functions are local and only has call chain that starts in
ImGui_ImplVulkan_CreateDeviceObjects(), so will always do early return.
@ocornut
Copy link
Owner

ocornut commented Jan 2, 2023

Thanks for this. Catching up with a few PR now.

I've merged the "dead code removal" commit as 14d4b36 and - wow :)
I guess when doing an earlier cross-branch refactor for multi-viewport this somehow happened. I can confirm the dead-code removal is same for docking branch for multi-viewport.

ocornut pushed a commit that referenced this pull request Jan 2, 2023
…6001)

Sampler, descriptor set layout and pipeline layout are created in exact
same way directly in ImGui_ImplVulkan_CreateDeviceObjects(). The removed
functions are local and only has call chain that starts in
ImGui_ImplVulkan_CreateDeviceObjects(), so will always do early return.
ocornut pushed a commit that referenced this pull request Jan 2, 2023
…Samplers, allow changing sampler. (#6001, #5502, #914)

Follow up to c9aef16 which removec three funtions worth of duplicate code.
@ocornut
Copy link
Owner

ocornut commented Jan 2, 2023

And merged second commit as e5d5186. Thanks a lot @martin-ejdestig and @rytisss!

@ocornut ocornut closed this Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulkan Backend image rendering. Image sampler always 'REPEAT'
2 participants