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

Update GlobalTransform as part of FixedUpdate #7836

Open
jabuwu opened this issue Feb 28, 2023 · 6 comments · May be fixed by #7929
Open

Update GlobalTransform as part of FixedUpdate #7836

jabuwu opened this issue Feb 28, 2023 · 6 comments · May be fixed by #7929
Labels
A-Time Involves time keeping and reporting A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes

Comments

@jabuwu
Copy link
Contributor

jabuwu commented Feb 28, 2023

What problem does this solve or what need does it fill?

The purpose of FixedUpdate and fixed timesteps in general is to improve determinism in systems, but any fixed update system relying on GlobalTransform will currently suffer from a lack of determinism if it runs multiple times in 1 frame.

What solution would you like?

GlobalTransform should be updated between fixed updates.

What alternative(s) have you considered?

Games could add these systems to FixedUpdate themselves. It's actually fairly easy to do already.

use bevy::{
    prelude::*,
    transform::systems::{propagate_transforms, sync_simple_transforms},
};

#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)]
pub enum FixedTransformSystem {
    TransformPropagate,
}

// ...

app.add_system_to_schedule(
    CoreSchedule::FixedUpdate,
    sync_simple_transforms.in_set(FixedTransformSystem::TransformPropagate),
)
.add_system_to_schedule(
    CoreSchedule::FixedUpdate,
    propagate_transforms.in_set(FixedTransformSystem::TransformPropagate),
);
@jabuwu jabuwu added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 28, 2023
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales A-Time Involves time keeping and reporting and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 28, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 28, 2023
@alice-i-cecile
Copy link
Member

WRT transform propagation, @inodentry has previously advocated for separating out a "physics transform" from a "rendering transform", and using interpolation to avoid visual hiccups.

This is pretty compelling to me, but definitely doesn't seem compatible with the approach proposed here.

@alice-i-cecile
Copy link
Member

I would also be interested in seeing if #7838 could be used to resolve these problems more gracefully.

@jabuwu
Copy link
Contributor Author

jabuwu commented Feb 28, 2023

I would also be interested in seeing if #7838 could be used to resolve these problems more gracefully.

While writing this issue, I did think about suggesting a way to manually "flush" GlobalTransform the same way we do command buffers, but quickly realized this is already possible. It's just not very pretty. Your before_and_seperated_by syntax is a nice solution here. I think it needs to be before_and_seperated_by(b, (sync_simple_transforms, propagate_transforms)). With two systems?

@alice-i-cecile
Copy link
Member

Yeah agreed: that's a nice catch that we may need to force separation by a collection of systems instead.

@james7132
Copy link
Member

In it's current state, transform propagation requires a full hierarchy traversal, this can be quite expensive, even if nothing changes, on large worlds. While this is probably needed for ECS-native physics simulation to work, I'm hesitant to suggest adding it now when the FixedUpdate schedule is empty by default, and runs potentially multiple times per frame. Ideally we should have some optimizations like #7840 before we enable this.

@james7132 james7132 linked a pull request Mar 6, 2023 that will close this issue
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 19, 2023
@alice-i-cecile alice-i-cecile removed this from the 0.11 milestone Jun 19, 2023
@JoJoJet JoJoJet added S-Blocked This cannot move forward until something else changes and removed X-Controversial There is active debate or serious implications around merging this PR labels Aug 26, 2023
@JoJoJet
Copy link
Member

JoJoJet commented Aug 26, 2023

IMO this isn't controversial, just not feasible performance-wise for now.

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-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants