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 backend incorrectly assumes WGPUTextureView pointers will never be reused #7765

Open
dkaste opened this issue Jul 2, 2024 · 7 comments

Comments

@dkaste
Copy link

dkaste commented Jul 2, 2024

Version/Branch of Dear ImGui:

Version 1.90.6, Branch: docking (through cimgui)

Back-ends:

imgui_impl_glfw.cpp + imgui_impl_wgpu.cpp

Compiler, OS:

Windows 10 + Zig (MSVC)

Full config/build information:

No response

Details:

My Issue/Question:

The WebGPU backend keeps a cache of WGPUBindGroups that internally reference WGPUTextureViews which are simply typedef'd opaque pointers. The backend uses a hash of the texture view pointer as the key for the cache. If a texture view is destroyed and another is created with the same pointer, the old, stale bind group will be used.

I ran into this when rapidly creating/destroying textures to resize a custom 3D viewport embedded in IMGUI.

My current workaround is to manually clear the internal bind group cache whenever changing textures used in IMGUI. It works, but it's far from ideal.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

@ocornut
Copy link
Owner

ocornut commented Jul 3, 2024

Hello,

Do you have a suggested solution as to how to decently solve this?

@dkaste
Copy link
Author

dkaste commented Jul 3, 2024

I'm not sure WebGPU exposes any kind of truly unique identifier for resources.

The only decent solution I can think of is to remove the cache, use WGPUBindGroup as ImTextureID, and provide a helper function for the user to create bind groups from texture views. It would unfortunately require the user to manually manage those bind groups, which kind of feels like it goes against the spirit of IMGUI.

@ocornut
Copy link
Owner

ocornut commented Jul 3, 2024

I don't really know/understand WebGPU yet but I would assume we could call wgpuDeviceCreateBindGroup() one time per frame with a list of all unique WGPUTextureView, and then call wgpuRenderPassEncoderSetBindGroup() with the right index?

So we remove bd->ImageBindGroups[] as a persistent storage, instead we use it as temporary storage during the frame:

  • iterate all draw cmd to create a list of unique WGPUTextureView, with each a unique integer.
  • create the bind group
  • iterate all draw cmd to perform render, call wgpuRenderPassEncoderSetBindGroup() with the right index pulled from that map?

@dkaste
Copy link
Author

dkaste commented Jul 3, 2024

That sounds like a reasonable solution. I'm new to WebGPU as well, so that didn't even occur to me.

It might be worth supporting multiple bind groups for "chunks", since WGPULimits::maxBindingsPerBindGroup could be as low as 640.

Edit: Actually, I think such a solution would require the proposed binding_array feature, which isn't in the spec yet.

@ocornut
Copy link
Owner

ocornut commented Jul 3, 2024

Let's start with a single bind group and we can easily change it to multiple of them based on runtime limits.

@ocornut
Copy link
Owner

ocornut commented Jul 3, 2024

I don't have test code for loading a texture in the WebGPU example so one would be helpful but otherwise I'll work toward one eventually.

@rdeepak2002
Copy link

rdeepak2002 commented Jul 6, 2024

Hello @dkaste, could you share your workaround solution? I'm facing the same issue and a temporary work around would help. Thanks!

Edit: never mind, was able to figure it out. Just called bd->renderResources.ImageBindGroups.Clear(); at the beginning of ImGui_ImplWGPU_RenderDrawData

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants