Skip to content

Remove unnecessary Option wrapper from PreviousGlobalTransform in meshlet extraction #20306

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fairhill1
Copy link
Contributor

Objective

Solution

  • Changed extract_meshlet_mesh_entities query from Option<&PreviousGlobalTransform> to &PreviousGlobalTransform
  • Updated add_instance function parameter from Option<&PreviousGlobalTransform> to &PreviousGlobalTransform
  • Simplified usage from previous_transform.map(|t| t.0).unwrap_or(transform) to previous_transform.0
  • Added clarifying comments to existing update_mesh_previous_global_transforms function

The initialization logic was already working correctly - the prepass system ensures PreviousGlobalTransform equals GlobalTransform on the first frame for new entities.

Testing

  • Created a test example that verifies PreviousGlobalTransform is correctly initialized to match GlobalTransform on the first frame
  • Ran the test and confirmed output: "SUCCESS: PreviousGlobalTransform correctly initialized to GlobalTransform"
  • No compilation errors or runtime issues observed
  • The existing meshlet rendering functionality continues to work as expected

Reviewers can test by:

  1. Running cargo run --example test_previous_transform to verify initialization works correctly
  2. Testing any existing meshlet-based examples to ensure no regressions
  3. Verifying that motion vectors still work properly in meshlet rendering

@alice-i-cecile alice-i-cecile requested a review from JMS55 July 27, 2025 23:20
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 27, 2025
@alice-i-cecile alice-i-cecile requested a review from atlv24 July 27, 2025 23:59
@fairhill1
Copy link
Contributor Author

fairhill1 commented Jul 28, 2025

the test file in examples/ should not be merged but leaving it for now for others to test during review

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

looks good to me (assuming the test example is removed for merge)

@@ -101,7 +101,7 @@ impl InstanceManager {
) {
// Build a MeshUniform for the instance
let transform = transform.affine();
let previous_transform = previous_transform.map(|t| t.0).unwrap_or(transform);
let previous_transform = previous_transform.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let previous_transform = previous_transform.0;

Imo remove this line, just use previous_transform.0 later on directly.

@JMS55
Copy link
Contributor

JMS55 commented Jul 28, 2025

I don't think this fix is correct.

update_mesh_previous_global_transforms runs in PreUpdate, so any MeshletMeshes spawned in Update or later will still have the incorrect PreviousGlobalTransform.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Requiring PreviousGlobalTransform for MeshletMesh is probably wrong
4 participants