Skip to content

Fix Gizmo joint rendering in webgpu#14721

Merged
alice-i-cecile merged 4 commits intobevyengine:mainfrom
akimakinai:fix-gizmo-joint-webgpu
Aug 25, 2024
Merged

Fix Gizmo joint rendering in webgpu#14721
alice-i-cecile merged 4 commits intobevyengine:mainfrom
akimakinai:fix-gizmo-joint-webgpu

Conversation

@akimakinai
Copy link
Contributor

Objective

Solution

I'm noob and just copied their solution, please correct me if I did something wrong.

Testing

  • Tested 2d-gizmos and 3d-gizmos examples in WebGPU on Chrome. No rendering errors, and the gizmo joints are apparently rendered ok.

@IQuick143 IQuick143 added C-Bug An unexpected or incorrect behavior O-Web Specific to web (WASM) builds A-Gizmos Visual editor and debug gizmos S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 12, 2024
@rparrett rparrett added this to the 0.14.2 milestone Aug 12, 2024
@nuzzles
Copy link
Member

nuzzles commented Aug 18, 2024

@alice-i-cecile looks like @irate-devil has no GitHub activity over the last month and change... Maybe choose a different reviewer?

@alice-i-cecile
Copy link
Member

Yep, I'll ask another couple of folks :)

Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely cannot figure out how to reproduce, but was able to trigger a crash while testing:

wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(34962, 1, Metal)>`
    In a draw command, indexed:false indirect:false
      note: render pipeline = `LineGizmo Pipeline`
    Instance 16 extends beyond limit 15 imposed by the buffer in slot 3. Did you bind the correct `Instance` step-rate vertex buffer?

@tychedelia
Copy link
Member

Was also able to trigger in the 2d example:

wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(2692, 1, Metal)>`
    In a draw command, indexed:false indirect:false
      note: render pipeline = `LineGizmo Pipeline 2D`
    Instance 149 extends beyond limit 148 imposed by the buffer in slot 3. Did you bind the correct `Instance` step-rate vertex buffer?

@tychedelia
Copy link
Member

The examples work fine, still can't figure out how I produced those crashes. If no one else can repro I'll approve with the note that there might be some kind of racy bug we should keep an eye out for.

@akimakinai
Copy link
Contributor Author

akimakinai commented Aug 18, 2024

I tested on browsers and Windows native, and I didn't encounter the validation error.
Currently I am on mac and it seems to reproduce occasionally for me too. I see Metal also in your log, so this could be Metal-only issue.
No, this seems to reproduce on every plaform.

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(19342, 1, Metal)>`
    In a draw command, indexed:false indirect:false
      note: render pipeline = `LineGizmo Pipeline`
    Instance 555 extends beyond limit 554 imposed by the buffer in slot 3. Did you bind the correct `Instance` step-rate vertex buffer?

I wonder why it's LineGizmo pipeline and not LineJointGizmo pipeline, but evidently triggered by enabling joints.

@akimakinai
Copy link
Contributor Author

akimakinai commented Aug 20, 2024

The same underlying buffer is used as vertex buffer with the same offset but different sizes from two draw functions. TrackedRenderPass::set_vertex_buffer wrongly assumes vertex buffer need not be updated in this case. (#14841)

I've added system ordering so that lines (draw function for lines uses larger size) are always drawn first to workaround vertex buffer not updated. This should fix the occasional validation error. Can you confirm?
We may want to fix TrackerRenderPass::set_vertex_buffer to properly update vertex buffer when silce size changed (https://discord.com/channels/691052431525675048/743663924229963868/1275593235535761409), but that should be separate PR, I think.

@tychedelia
Copy link
Member

I've added system ordering so that lines are always drawn first to workaround vertex buffer not updated. This should fix the occasional validation error. Can you confirm?

Thanks for tracking that down. I'm not able to reproduce here, but it sounds like we're more confident in the underlying issue, which can be a follow up. Approving.

@tychedelia tychedelia added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 25, 2024
Merged via the queue into bevyengine:main with commit 89a5c74 Aug 25, 2024
@akimakinai akimakinai deleted the fix-gizmo-joint-webgpu branch August 25, 2024 15:18
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
# Objective

- Fixes #14841

## Solution

- Compute BufferSlice size manually and use it for comparison in
`TrackedRenderPass`

## Testing

- Gizmo example does not crash with #14721 (without system ordering),
and `slice` computes correct size there

---

## Migration Guide

- `TrackedRenderPass::set_vertex_buffer` function has been modified to
update vertex buffers when the same buffer with the same offset is
provided, but its size has changed. Some existing code may rely on the
previous behavior, which did not update the vertex buffer in this
scenario.

---------

Co-authored-by: Zachary Harrold <zac@harrold.com.au>
mockersf pushed a commit that referenced this pull request Sep 5, 2024
# Objective

- Gizmo rendering on WebGPU has been fixed by #14653, but gizmo joints
still cause error
(#14696 (comment))
when enabled.

## Solution

- Applies the same fix as #14653 to Gizmo joints.

I'm noob and just copied their solution, please correct me if I did
something wrong.

## Testing

- Tested 2d-gizmos and 3d-gizmos examples in WebGPU on Chrome. No
rendering errors, and the gizmo joints are apparently rendered ok.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Gizmos Visual editor and debug gizmos C-Bug An unexpected or incorrect behavior O-Web Specific to web (WASM) builds S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants