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

index buffer size of zero now causes black screen #4766

Closed
meshula opened this issue Nov 27, 2021 · 3 comments
Closed

index buffer size of zero now causes black screen #4766

meshula opened this issue Nov 27, 2021 · 3 comments

Comments

@meshula
Copy link

meshula commented Nov 27, 2021

Version/Branch of Dear ImGui:

Version: top of tree
Branch: master/docking

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_wgpu.cpp

My Issue/Question:

When running ImGui with the latest Dawn (Chrome wgpu engine), nothing currently draws. I traced it down to this:

fr->IndexBufferSize = draw_data->TotalIdxCount + 10000;

This line of code adds 10,000 to the index count to create a buffer. The wgpu back end defaults to a 16 bit index size. It's not especially a problem, to my knowledge, that an over-large buffer is created here, although the number is undocumented, and I am wondering if this is left-over debugging code?

That's not the actual problem though, later. Some validation layers (e.g. latest Chrome) fault with this message:

Error: Validation error: Index range (first: 0, count: 105, format: IndexFormat::Uint16) does not fit in index buffer size (0).
 - While encoding [RenderPassEncoder].DrawIndexed(105, 1, 0, 0, 0).

That issue results from this line of code:

wgpuRenderPassEncoderSetIndexBuffer(ctx, fr->IndexBuffer, sizeof(ImDrawIdx) == 2 ? WGPUIndexFormat_Uint16 : WGPUIndexFormat_Uint32, 0, 0);

    wgpuRenderPassEncoderSetIndexBuffer(ctx, fr->IndexBuffer, sizeof(ImDrawIdx) == 2 ? WGPUIndexFormat_Uint16 : WGPUIndexFormat_Uint32, 0, 0);

where the last parameter is the size of the index buffer. It should be 10000 + index count, according to the previous line of code. Since zero is being passed in, it now causes validation errors.

Fix:

the value is available in fr->IndexBufferSize, and changing the last parameter to that restores drawing functionality. I think the "trick" of passing zero as the index buffer size has been deprecated recently.

@ocornut
Copy link
Owner

ocornut commented Nov 29, 2021

This line of code adds 10,000 to the index count to create a buffer. The wgpu back end defaults to a 16 bit index size. It's not especially a problem, to my knowledge, that an over-large buffer is created here, although the number is undocumented, and I am wondering if this is left-over debugging code?

This is intentional, every time we grow the buffer we reserve extra space to avoid having to recreate the buffer too often as it is likely to increase again shortly after. It's a simple amortizing strategy for the underlying allocation. Many other strategies are possible, though they don't matter so much as unlike a typical std::vector+reserve use, here our buffer is write only from the POV of the CPU so we just recreate one and there's no extra copy involved.

the value is available in fr->IndexBufferSize, and changing the last parameter to that restores drawing functionality. I think the "trick" of passing zero as the index buffer size has been deprecated recently.

Will apply this to both wgpuRenderPassEncoderSetVertexBuffer() and wgpuRenderPassEncoderSetIndexBuffer() calls then.

The specs says:
https://www.w3.org/TR/webgpu/#dom-gpurenderencoderbase-setindexbuffer
If size is missing, set size to max(0, buffer.[[size]] - offset).

(Technically speaking, but it would depends on what the specs says, it might be advantageous to pass draw_data->TotalVtxCount and draw_data->TotalIdxCount to those functions... may be advantageous in term of reducing supposed copy+validation.. but the fact that we are passing a value smaller than the buffer create an additional copy where it wouldn't be needed without then it's not advantageous..)

ocornut added a commit that referenced this issue Nov 29, 2021
…appears to not do what the in-flux specs says. (#4766
@ocornut
Copy link
Owner

ocornut commented Nov 29, 2021

Pushed tentative fix 5ccb667

@ocornut ocornut closed this as completed Nov 29, 2021
@meshula
Copy link
Author

meshula commented Dec 1, 2021

Fixes the validation error I ran into, thanks!

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

2 participants