Fix the example regressions from packed growable buffers. #14375
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
added
P-Regression
IceSentry
approved these changes
Jul 18, 2024
|
This is currently slated for 0.15 (in the issue), but should it be included in 0.14.1 as well? |
|
No version of 0.14 includes uberbuffers, right? This fix is specific to uberbuffers. |
|
Yup, 0.15 is right, uberbuffers will not be included in a point release of 0.14 |
tychedelia
approved these changes
Jul 18, 2024
NthTensor
added
the
S-Ready-For-Final-Review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The "uberbuffers" PR #14257 caused some examples to fail intermittently for different reasons:
morph_targetscould 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 theMeshUniform(andMeshInputUniformif 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 inMeshUniform, 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
MeshInputUniformcaused an ordering problem. TheMeshInputUniformwas 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 theMeshInputUniformcreation (thecollect_meshes_for_gpu_buildingsystem) 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.The
linesandfog_volumesexamples could fail because their custom drawing nodes weren't updated to supply the vertex and index offsets in theirdraw_indexedanddrawcalls. This commit fixes this oversight.Fixes #14366.