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.

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

Fix ambiguities in transform example #9845

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

rparrett
Copy link
Contributor

Objective

Fix these

 -- rotate_cube and move_cube
    conflict on: ["bevy_transform::components::transform::Transform", "transform::CubeState"]
 -- rotate_cube and scale_down_sphere_proportional_to_cube_travel_distance
    conflict on: ["bevy_transform::components::transform::Transform", "transform::CubeState"]
 -- move_cube and scale_down_sphere_proportional_to_cube_travel_distance
    conflict on: ["bevy_transform::components::transform::Transform", "transform::CubeState"]

The three systems in this example depend on the results of the others. This leads to minor but detectable differences in output between runs by automated screenshot diffing depending on the order of the schedule.

We don't necessarily need to be able to do this for every example, but I think this is a case where fixing it is easy / maybe the right thing to do anyway.

Solution

Chain the three systems

@hymm
Copy link
Contributor

hymm commented Sep 19, 2023

is the order just arbitrary?

@rparrett
Copy link
Contributor Author

rparrett commented Sep 19, 2023

The order of the scaling operation isn't as important and it probably only needs to be ordered relative to the movement system.

I think move -> rotate is actually slightly important, as without this order the cube might not actually be facing the center. I'm guessing this would be pretty obvious at lower framerates but I haven't checked.

So I guess

(
    move_cube,
    rotate_cube.after(move_cube),
    scale_down_sphere_proportional_to_cube_travel_distance.after(move_cube),
),

would probably also be fine.

Fixing up ambiguities for the sake of it in examples probably isn't necessary but technically that setup could also use a .ambiguous_with(rotate_cube) on scale_down_sphere_proportional_to_cube_travel_distance if we really wanted to be correct.

Let me know if you'd prefer the ordering above.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Since it is just an example (and a stress test at that), I'm fine with the chain. It looks cleaner.

@mockersf mockersf added C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it A-Transform Translations, rotations and scales labels Sep 19, 2023
@mockersf mockersf added this pull request to the merge queue Sep 19, 2023
Merged via the queue into bevyengine:main with commit d3f612c Sep 19, 2023
25 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Fix these

```
 -- rotate_cube and move_cube
    conflict on: ["bevy_transform::components::transform::Transform", "transform::CubeState"]
 -- rotate_cube and scale_down_sphere_proportional_to_cube_travel_distance
    conflict on: ["bevy_transform::components::transform::Transform", "transform::CubeState"]
 -- move_cube and scale_down_sphere_proportional_to_cube_travel_distance
    conflict on: ["bevy_transform::components::transform::Transform", "transform::CubeState"]
```

The three systems in this example depend on the results of the others.
This leads to minor but detectable differences in output between runs by
automated screenshot diffing depending on the order of the schedule.

We don't necessarily need to be able to do this for **every** example,
but I think this is a case where fixing it is easy / maybe the right
thing to do anyway.

## Solution

Chain the three systems
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Examples An addition or correction to our examples 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.

3 participants