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

Propagate transforms in fixed update #7929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

james7132
Copy link
Member

Objective

Fix #7836. GlobalTransform is not updated in FixedUpdate.

Solution

Add it to FixedUpdate, keep the same system sets, clean up the code a bit.


Changelog

Changed: Transform propagation now runs in CoreSet::FixedUpdate.

@james7132 james7132 added A-Transform Translations, rotations and scales C-Feature A new feature, making something new possible A-Time Involves time keeping and reporting labels Mar 6, 2023
@james7132 james7132 added this to the 0.11 milestone Mar 6, 2023
@tim-blackbird
Copy link
Contributor

We will need to add base sets to the FixedUpdate schedule, otherwise users will need to explicitly run their systems before transform propagation.

@james7132
Copy link
Member Author

Marking this as blocked on #7835 then.

@james7132 james7132 added the S-Blocked This cannot move forward until something else changes label Mar 6, 2023
@hymm
Copy link
Contributor

hymm commented Mar 6, 2023

won't this potentially run propagate_transforms multiple times in a frame even if a user isn't using fixed update? Or is the cost of that negligible with the change checking?

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

It would be really nice if there was a way for multiple systems/instances of a system to share the same last_run change tick. That would allow us to avoid duplicate propagations across schedules.

Comment on lines +87 to +90
// A set for `propagate_transforms` to mark it as ambiguous with `sync_simple_transforms`.
// Used instead of the `SystemTypeSet` as that would not allow multiple instances of the system.
#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)]
struct PropagateTransformsSet;
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a local type inside of fn propagate_in_schedule.

Comment on lines +115 to +126
fn propagate_in_schedule(schedule: &mut Schedule) {
// FIXME: https://github.com/bevyengine/bevy/issues/4381
// These systems cannot access the same entities,
// due to subtle query filtering that is not yet correctly computed in the ambiguity detector
schedule
.add_system(
sync_simple_transforms
.in_set(TransformSystem::TransformPropagate)
.ambiguous_with(PropagateTransformsSet),
)
.add_system(propagate_transforms.in_set(PropagateTransformsSet));
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding systems to an &mut Schedule, can you make this return a SystemConfigs struct? Then the caller would just call add_systems with the result to add both systems to a schedule.

@alice-i-cecile alice-i-cecile removed this from the 0.11 milestone Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Time Involves time keeping and reporting A-Transform Translations, rotations and scales C-Feature A new feature, making something new possible S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update GlobalTransform as part of FixedUpdate
5 participants