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.

Sign up for GitHub

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

Jump to bottom

Some examples are broken after packed growable buffers #14366

Closed
mockersf opened this issue Jul 17, 2024 · 0 comments · Fixed by #14375
Closed

Some examples are broken after packed growable buffers #14366

mockersf opened this issue Jul 17, 2024 · 0 comments · Fixed by #14375
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this!
Milestone
0.15

Comments

@mockersf
Copy link
Member

Bevy version

main since #14257

[Optional] Relevant system information

Issues seen on Metal and Vulkan

What you did

Run examples:

  • lines
    c1f51f32bbfc8a9a2b9c6f47373c50b32c14d53d813e4b0244966a427b561327

  • fog_volumes
    5bb9696c57f83700b85d9922bfb2ad6429ae5af31a62ee6dd02372a350431195

  • morph_targets
    4adad56c11400e92e840247fe8d3b333e6e7c288f059790452b68568d885f7df

What went wrong

Rendering is not correct

Additional information

This is random and doesn't always happen, or not always the same way
@mockersf mockersf added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-Regression Functionality that used to work but no longer does. Add a test for this! labels Jul 17, 2024
@mockersf mockersf added this to the 0.15 milestone Jul 17, 2024
pcwalton added a commit to pcwalton/bevy that referenced this issue Jul 18, 2024
@pcwalton
Fix the example regressions from packed growable buffers.
783d96c 
The "uberbuffers" PR bevyengine#14257 caused some examples to fail intermittently
for different reasons:

1. `morph_targets` could fail because vertex displacements for morph
   targets are keyed off the vertex index. With buffer packing, the
   vertex index can vary based on the position in the buffer, which
   caused the morph targets to be potentially incorrect. The solution is
   to include the first vertex index with the `MeshUniform` (and
   `MeshInputUniform` if GPU preprocessing is in use), so that the
   shader can calculate the true vertex index before performing the
   morph operation. This results in wasted space in `MeshUniform`, which
   is unfortunate, but we'll soon be filling in the padding with the ID
   of the material when bindless textures land, so this had to happen
   sooner or later anyhow.

   Including the vertex index in the `MeshInputUniform` caused an
   ordering problem. The `MeshInputUniform` was created during the
   extraction phase, before the allocations occurred, so the extraction
   logic didn't know where the mesh vertex data was going to end up. The
   solution is to move the `MeshInputUniform` creation (the
   `collect_meshes_for_gpu_building` system) to after the allocations
   phase. This should be better for parallelism anyhow, because it
   allows the extraction phase to finish quicker. It's also something
   we'll have to do for bindless in any event.

2. The `lines` and `fog_volumes` examples could fail because their
   custom drawing nodes weren't updated to supply the vertex and index
   offsets in their `draw_indexed` and `draw` calls. This commit fixes
   this oversight.

Fixes bevyengine#14366.
@pcwalton pcwalton mentioned this issue Jul 18, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Jul 22, 2024
@pcwalton
Fix the example regressions from packed growable buffers. (#14375)
d235d41 
The "uberbuffers" PR #14257 caused some examples to fail intermittently
for different reasons:

1. `morph_targets` could fail because vertex displacements for morph
targets are keyed off the vertex index. With buffer packing, the vertex
index can vary based on the position in the buffer, which caused the
morph targets to be potentially incorrect. The solution is to include
the first vertex index with the `MeshUniform` (and `MeshInputUniform` if
GPU preprocessing is in use), so that the shader can calculate the true
vertex index before performing the morph operation. This results in
wasted space in `MeshUniform`, which is unfortunate, but we'll soon be
filling in the padding with the ID of the material when bindless
textures land, so this had to happen sooner or later anyhow.

Including the vertex index in the `MeshInputUniform` caused an ordering
problem. The `MeshInputUniform` was created during the extraction phase,
before the allocations occurred, so the extraction logic didn't know
where the mesh vertex data was going to end up. The solution is to move
the `MeshInputUniform` creation (the `collect_meshes_for_gpu_building`
system) to after the allocations phase. This should be better for
parallelism anyhow, because it allows the extraction phase to finish
quicker. It's also something we'll have to do for bindless in any event.

2. The `lines` and `fog_volumes` examples could fail because their
custom drawing nodes weren't updated to supply the vertex and index
offsets in their `draw_indexed` and `draw` calls. This commit fixes this
oversight.

Fixes #14366.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Milestone
0.15
Development

Successfully merging a pull request may close this issue.

Fix the example regressions from packed growable buffers. pcwalton/bevy
1 participant
@mockersf