-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Run fixed time-step in an exclusive system #5467
Conversation
d1a58be
to
35d9486
Compare
399fd34
to
b611dfd
Compare
55d9e7f
to
b07208b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay!! I actually really like this.
The "sub-schedules" seem like a very nice and useful abstraction, more flexible than what I've been doing in iyes_loopless
with a custom Stage impl. I can also envision plenty of other uses besides fixed timestep. :)
The new fixed timestep implementation seems more reliable and in line with what stageless and iyes_loopless
can offer.
I would be very happy to see this merged into Bevy, before the full Stageless rework.
I approve of the overall design and implementation, but I left a couple of comments for a few things I'd like to be addressed.
{ | ||
let label = label.as_label(); | ||
|
||
// Extract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about the overloading of the term "extract" in this PR, overlapping with the current use of that term in the bevy Rendering architecture. It may lead to confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the word extract is general enough that it shouldn't cause confusion, but I wonder what other term we could use. Partition? Isolate?
|
||
/// The internal state of each [`FixedTimestep`]. | ||
#[derive(Debug)] | ||
#[derive(Debug, Clone, Copy)] | ||
pub struct FixedTimestepState { | ||
step: f64, | ||
accumulator: f64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider switching to using Duration
instead of floating point values for the internal state of the fixed timestep.
I know that floats are how the existing Bevy code does it, but it was a mistake. Floats are not an accurate or reliable representation of time.
My implementation in iyes_loopless
uses Duration
s instead, allowing it to be more exact.
It should be a very easy and noncontroversial replacement.
Given that this PR is reworking all of FixedTimestep, I believe this small-but-important thing should be addressed, while we are at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
I do support the changing fixed timestep from being a run criteria, but I have some reservations about merging this by itself. Please excuse me as I fumble my way through this.
Almost every type and trait involved in this PR is (supposedly temporary) public API that wasn't described in the stageless RFC.
I don't mean that negatively, but this PR still weighs in on more than just the fixed timestep. Are we committing to this specific API language for later PRs? I've been wondering if proceeding incrementally is going to create more work as we go. For example, I am not a fan of migrating multiple times. I think we've completely avoided answering the "What is the best way to handle the migration process from an organizational perspective?" question from the RFC. None of the people involved with stageless (myself included) are apparently on the same page for how we should get there. I think we should come to a consensus on that now, so that we can coordinate. Personally, I think we should try to collectively work on single PR or a small handful (e.g. add (I do especially like what you have for |
Most of this issue stems from all of these PR's being non-final, and not yet merged. Once the system registry PR gets merged, for example, I can change up this PR to work with that, but I can't really do that until it gets merged. And these are orthogonal IMO so it wouldn't make. I honestly think it would be best to just merge stageless all it once, but it seems like the consensus is that the migration should be incremental.
Can you elaborate on how this would work? Everything in |
I think the idea was that everything system-related (including system sets and their dependency graphs) would go into a single resource. So your
I don't think there's a consensus. I think I just turned people off by coming in with a PR that had 10k line changes (in my defense, it included both the changes to the schedule module and porting the rest of the engine).
My suggestion would be:
|
I just don't see how that would work cleanly. If we split schedule into If the idea does end up working out, it is probably the easiest path forward though. |
I meant keep Maybe we could do something similar to what you've done here with |
That could work maybe, but I think I prefer the original two options: either merge it all it once, or do it incrementally (like this PR). If your concern is just with some of the API decisions, I can change them. This just happened to be the most comfortable API I found by iterating, but I'm not super attached. |
Also, the point about migrating twice: as long as this gets replaced before 0.9, there would only really be one migration. The engine itself barely uses fixed timestep, so migration is trivial. This PR would make incremental migration towards stageless significantly easier by removing one of the last few uses of looping run criteria. |
We agreed on discord that our migration towards stageless should be more formalized. Closing this out for now. |
Objective
Solution
SubSchedule
-- a schedule that does not necessarily run along with the main schedule.OnEnter
orOnExit
for states.SubSchedule
is worse since it's split into stages.Changelog
todo
Migration Guide
Before:
After:
More complex schedules:
Notes
This PR adds
thiserror
as a direct dependency forbevy_app
, but it was already in the dependency tree.