Don't mark a previous mesh transform as changed if it didn't actually change. #17688
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
change. This patch fixes a bug whereby we're re-extracting every mesh every frame. It's a regression from PR bevyengine#17413. The code in question has actually been in the tree with this bug for quite a while; it's that just the code didn't actually run unless the renderer considered the previous view transforms necessary. Occlusion culling expanded the set of circumstances under which Bevy computes the previous view transforms, causing this bug to appear more often. This patch fixes the issue by checking to see if the previous transform of a mesh actually differs from the current transform before copying the current transform to the previous transform.
pcwalton
added
A-Rendering
IceSentry
reviewed
Feb 5, 2025
| // Make sure not to trigger change detection on | ||
| // `PreviousGlobalTransform` if the previous transform hasn't | ||
| // changed. | ||
| if old_previous_transform != Some(&new_previous_transform) { |
There was a problem hiding this comment.
IIRC there's a way to skip change detection while letting you still mutate the component directly instead of reinserting. I don't know if it would have a significant perf impact on this though.
There was a problem hiding this comment.
We would still need to potentially insert because the mesh might not have a PreviousGlobalTransform to begin with.
IceSentry
approved these changes
Feb 5, 2025
tychedelia
approved these changes
Feb 5, 2025
tychedelia
added
S-Ready-For-Final-Review
|
Wasm CI issues should be resolved once you upstream from |
superdump
approved these changes
Feb 5, 2025
pcwalton
added a commit
to pcwalton/bevy
that referenced
this pull request
Feb 7, 2025
PR bevyengine#17688 broke motion vector computation, and therefore motion blur, because it enabled retention of `MeshInputUniform`s, and `MeshInputUniform`s contain the indices of the previous frame's transform and the previous frame's skinned mesh joint matrices. On frame N, if a `MeshInputUniform` is retained on GPU from the previous frame, the `previous_input_index` and `previous_skin_index` would refer to the indices for frame N - 2, not the index for frame N - 1. This patch fixes the problems. It solves these issues in two different ways, one for transforms and one for skins: 1. To fix transforms, this patch supplies the *frame index* to the shader as part of the view uniforms, and specifies which frame index each mesh's previous transform refers to. So, in the situation described above, the frame index would be N, the previous frame index would be N - 1, and the `previous_input_frame_number` would be N - 2. The shader can now detect this situation and infer that the mesh has been retained, and can therefore conclude that the mesh's transform hasn't changed. 2. To fix skins, this patch replaces the explicit `previous_skin_index` with an invariant that the index of the joints for the current frame and the index of the joints for the previous frame are the same. This means that the `MeshInputUniform` never has to be updated even if the skin is animated. The downside is that we have to copy joint matrices from the previous frame's buffer to the current frame's buffer in `extract_skins`. The rationale behind (2) is that we currently have no mechanism to detect when joints that affect a skin have been updated, short of comparing all the transforms and setting a flag for `extract_meshes_for_gpu_building` to consume, which would regress performance as we want `extract_skins` and `extract_meshes_for_gpu_building` to be able to run in parallel. To test this change, use `cargo run --example motion_blur`.
mrchantey
pushed a commit
to mrchantey/bevy
that referenced
this pull request
Feb 17, 2025
… change. (bevyengine#17688) This patch fixes a bug whereby we're re-extracting every mesh every frame. It's a regression from PR bevyengine#17413. The code in question has actually been in the tree with this bug for quite a while; it's that just the code didn't actually run unless the renderer considered the previous view transforms necessary. Occlusion culling expanded the set of circumstances under which Bevy computes the previous view transforms, causing this bug to appear more often. This patch fixes the issue by checking to see if the previous transform of a mesh actually differs from the current transform before copying the current transform to the previous transform.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 18, 2025
PR #17688 broke motion vector computation, and therefore motion blur, because it enabled retention of `MeshInputUniform`s, and `MeshInputUniform`s contain the indices of the previous frame's transform and the previous frame's skinned mesh joint matrices. On frame N, if a `MeshInputUniform` is retained on GPU from the previous frame, the `previous_input_index` and `previous_skin_index` would refer to the indices for frame N - 2, not the index for frame N - 1. This patch fixes the problems. It solves these issues in two different ways, one for transforms and one for skins: 1. To fix transforms, this patch supplies the *frame index* to the shader as part of the view uniforms, and specifies which frame index each mesh's previous transform refers to. So, in the situation described above, the frame index would be N, the previous frame index would be N - 1, and the `previous_input_frame_number` would be N - 2. The shader can now detect this situation and infer that the mesh has been retained, and can therefore conclude that the mesh's transform hasn't changed. 2. To fix skins, this patch replaces the explicit `previous_skin_index` with an invariant that the index of the joints for the current frame and the index of the joints for the previous frame are the same. This means that the `MeshInputUniform` never has to be updated even if the skin is animated. The downside is that we have to copy joint matrices from the previous frame's buffer to the current frame's buffer in `extract_skins`. The rationale behind (2) is that we currently have no mechanism to detect when joints that affect a skin have been updated, short of comparing all the transforms and setting a flag for `extract_meshes_for_gpu_building` to consume, which would regress performance as we want `extract_skins` and `extract_meshes_for_gpu_building` to be able to run in parallel. To test this change, use `cargo run --example motion_blur`.
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.
This patch fixes a bug whereby we're re-extracting every mesh every frame. It's a regression from PR #17413. The code in question has actually been in the tree with this bug for quite a while; it's that just the code didn't actually run unless the renderer considered the previous view transforms necessary. Occlusion culling expanded the set of circumstances under which Bevy computes the previous view transforms, causing this bug to appear more often.
This patch fixes the issue by checking to see if the previous transform of a mesh actually differs from the current transform before copying the current transform to the previous transform.