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

Allow ordering variable timesteps around fixed timesteps #14881

Merged
merged 13 commits into from
Aug 23, 2024

Conversation

janhohenheim
Copy link
Member

@janhohenheim janhohenheim commented Aug 22, 2024

Objective

Solution

Note that the gizmo systems, LWIM and now a new plugin I'm working on are all already ordering against run_fixed_main_schedule, so having a dedicated system set should be more robust and hopefully also more discoverable.


Showcase

I can add a little video of a smooth camera later if this gets merged :)
Apparently a release note is not needed, so I'll leave it out. See the changes in the fixed timestep example for usage showcase and the video in #14873 for a more or less accurate video of the effect (it does not use the same solution though, so it is not quite the same)

Migration Guide

run_fixed_main_schedule is no longer public. If you used to order against it, use the new dedicated RunFixedMainLoopSystem system set instead. You can replace your usage of run_fixed_main_schedule one for one by RunFixedMainLoopSystem::FixedMainLoop, but it is now more idiomatic to place your systems in either RunFixedMainLoopSystem::BeforeFixedMainLoop or RunFixedMainLoopSystem::AfterFixedMainLoop

Old:

app.add_systems(
    RunFixedMainLoop,
    some_system.before(run_fixed_main_schedule)
);

New:

app.add_systems(
    RunFixedMainLoop,
    some_system.in_set(RunFixedMainLoopSystem::BeforeFixedMainLoop)
);

@janhohenheim janhohenheim added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins labels Aug 22, 2024
@janhohenheim
Copy link
Member Author

janhohenheim commented Aug 22, 2024

Addressed all replies. I made run_fixed_main_schedule private as it is no longer needed, given that we have a proper system set now. I also showcase the new set with its intended usage in the fixed timestep example. Feel free to tell me to revert it if it is too big now; the changes are not strictly necessary, I just thought it would be a neat place to make users aware of this.
I also added the release note label given that this will help people deal better with a fairly common issue when working with fixed time steps. I can add nice little video showing the effect side-by-side in a first-person example, if you want :)

@janhohenheim janhohenheim added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 22, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@janhohenheim janhohenheim added M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed M-Needs-Release-Note Work that should be called out in the blog due to impact labels Aug 22, 2024
@alice-i-cecile alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 22, 2024
@chescock
Copy link
Contributor

Having a dedicated system set seems much cleaner than ordering against the system name!

What's the advantage of having three sets versus just one, though? Could you remove the BeforeFixedMainLoop and AfterFixedMainLoop variants and just use some_system.before(RunFixedMainLoopSystem::FixedMainLoop) and some_system.after(RunFixedMainLoopSystem::FixedMainLoop)? You could even shorten it from an enum to a unit struct.

(I see @NiseVoid's comment about running "after all the other before things but before FixedMain", but I don't think there is any ordering between in_set(BeforeFixedMainLoop) and before(FixedMainLoop); they're just both before in_set(FixedMainLoop). And even if there is, you'd have ambiguities if two systems both wanted to run last, so the extra label wouldn't fully solve the problem.)

@chescock
Copy link
Contributor

It might be worth noting in the doc comments that the RunFixedMainLoop schedule uses ExecutorKind::SingleThreaded, so nothing scheduled using these sets will have system-level parallelism. That's unusual for built-in schedules that users add systems to.

@janhohenheim
Copy link
Member Author

@chescock the ordering comes from foo.after(FixedMainLoop).before(AfterFixedMainLoop).
Bottom line is that using multiple variants is cleaner as you get a definitive place to use .in_set with. Plus, this won't break in the unlikely case where we split FixedMainLoop into multiple system sets.

As for the single-threaded stuff: good point, definitely worth noting or even changing entirely

@alice-i-cecile
Copy link
Member

IMO we shouldn't change it, but it should be noted in the docs for the schedule.

@TrialDragon TrialDragon added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Aug 23, 2024
@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 23, 2024
janhohenheim and others added 2 commits August 23, 2024 16:18
Co-authored-by: Tau Gärtli <git@tau.garden>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 23, 2024
Merged via the queue into bevyengine:main with commit c92ee31 Aug 23, 2024
26 checks passed
@janhohenheim janhohenheim deleted the around-fixed branch August 23, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Bevy has no clear schedule for updating a camera's Transform when using a fixed time step
6 participants