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

WebGPU: Use ClampToEdge instead of Repeat for the texture sampler #7468

Closed
wants to merge 1 commit into from

Conversation

JulesFouchy
Copy link
Contributor

The sampler is currently set to use Repeat, which can cause unexpected behavior when using ImGui::Image(). For some image sizes and positions, UVs can go slightly bigger than 1, causing the sampler to kick in. For example this can produce this thin line of red pixels at the top of the image:
image

With the proposed changes, the red line disappears:
image

NB: if there was a reason to use Repeat for the font texture, maybe we could create two samplers, one for the Font texture, and one for user-provided textures.

@PathogenDavid
Copy link
Contributor

I don't think Dear ImGui's rendering actually relies on repeat texture sampling, but this change would make the WebGPU backend inconsistent with other backends so a proper fix is not going to be this simple.

Related issues for other backends: #7230 #5502

I'm not super familiar with WebGPU myself so I can't say for sure what the proper solution would be, but I do have some suggestions. I suspect it's the second solution listed below. It's basically what the Vulkan backend does but it'd be a breaking change. That might be OK though considering WebGPU is still relatively young.

Modifying the sampler in a callback

The solution Dear ImGui "expects" would be for you to override the sampler temporarily in a callback passed to ImDrawList::AddCallback before you add the image.

However this isn't always practical with modern graphics APIs, but it looks like it almost might be with WebGPU? It looks like the texture sampler could be overridden with wgpuRenderPassEncoderSetBindGroup if we separated the uniforms buffer and the sampler into separate bind groups?

Main problem here is I assume the WGPURenderPassEncoder is not readily available when the callback is added. Even if it is available, it presumably couldn't be for WebGPU on desktop whenever multiple viewports are added. D3D12 and Vulkan have had this problem as well, see #5834

Changing ImTextureID for WebGPU

Alternatively the sampler could be moved to the image bind group and the texture IDs for WebGPU could be modified to expect a WGPUBindGroup in the right layout instead of a WGPUTextureView as they currently do. (Seeing that the WebGPU backend is using an ImGuiStorage to cache pairs bind groups for textures makes me think this is a better long-term solution, but it would be a breaking change.)

(This is also basically what the Vulkan backend does via ImGui_ImplVulkan_AddTexture.)

Manually rendered images

A final alternative that probably works today would be to use ImGui::Dummy instead of ImGui::Image and using a callback to render the image yourself. (This runs into the same WGPURenderPassEncoder issue as mentioned above though.)

@ypujante
Copy link
Contributor

This is the exact fix to the issue I reported here #7511

@ypujante
Copy link
Contributor

As I pointed in my bug description (#7511), the demo that comes with ImGui is actually showing the problem. You can check it out for yourself: https://pongasoft.github.io/imgui/pr-7151/

Check Widgets/Images and you will see the artifacts at the bottom of the image as well as at the top of the triangle (somewhat hard to see because of the surrounding box). So it is very easy to reproduce since the built-in demo shows the issue...

ocornut added a commit that referenced this pull request Oct 7, 2024
ocornut added a commit that referenced this pull request Oct 7, 2024
…t texture sampler to Clamp instead of Repeat/Wrap. (#7468, #7511, #5999, #5502)
ocornut added a commit that referenced this pull request Oct 7, 2024
…rRenderState' -> 'platform_io.Renderer_RenderState'. (#6969, #5834, #7468, #3590

Amend e94f95d
@ocornut
Copy link
Owner

ocornut commented Oct 7, 2024

I have pushed a wider change 42206b3
See #7230 (comment) for more details.

@ocornut ocornut closed this Oct 7, 2024
ocornut added a commit that referenced this pull request Oct 9, 2024
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.

4 participants