[Merged by Bors] - Add TimeUpdateStrategy resource for manual Time updating

[ShinySapphic]

Objective

• Addresses Add app.update_with_instant #6146 by allowing manual Time updating

Solution

• Create TimeUpdateStrategy config resource
• Allow users to specify a manual Instant/Duration or leave as default (automatic)
• Get resource in bevy_time::time_systemand update time with desired value

---

Changelog

• Add TimeUpdateStrategy resource
• Update bevy_time::time_system to use optional manual values

[alice-i-cecile]

Is there a reason this isn't pub?

[ShinySapphic]

Not really. I just assumed since it was going to be inserted from the method that it didn't need to be accessed externally. Now that I think about it, there's probably some use cases where you might want to access it externally so I'll go ahead and make it public.

[alice-i-cecile]

I think a doc test on update_with_instant would be nice, but I'm okay if we merge this as is.

[maniwani]

Is it OK that the TimeReceiver channel interaction has precedence over the manual time setting?

If the answer is yes, then this all seems fine to me.

[ShinySapphic]

Is it OK that the TimeReceiver channel interaction has precedence over the manual time setting?

If the answer is yes, then this all seems fine to me.

I think so. I wasn't 100% sure how important TimeReceiver was. I even thought about combining it with DesiredTime since they almost do the same thing, but I wasn't too sure.

[alice-i-cecile]

Is it OK that the TimeReceiver channel interaction has precedence over the manual time setting?

Ping @hymm; do you have opinions on this?

[hymm]

I even thought about combining it with DesiredTime since they almost do the same thing, but I wasn't too sure.

Overall it feels weird to only update time once instead of taking over the machinery completely. Are there use cases for only updating the time once and then going back to using the Instant::now()? Seems like something that would be problematic with frames reporting zero time for multiple frames or an extra long frame time once.

I feel like a better approach would be to somehow not use the time_system and just do everything (including updating the Res) in the app.update_with_instant function.

[alice-i-cecile]

Are there use cases for only updating the time once and then going back to using the Instant::now()? Seems like something that would be problematic with frames reporting zero time for multiple frames or an extra long frame time once.

When reproducing tricky bugs, you may want to immediately "fast forward" time to the recorded problem state, and then play through as normal. I don't think it's common though.

I feel like a better approach would be to somehow not use the time_system and just do everything (including updating the Res) in the app.update_with_instant function.

Agreed, this feels like the more coherent strategy overall.

[ShinySapphic]

I feel like a better approach would be to somehow not use the time_system and just do everything (including updating the Res) in the app.update_with_instant function.

I agree as well. I would've preferred to not do it the way I did but I didn't think it was possible. Given the information in the issue, time should only be changed after time_system but before everything else otherwise the desired time isn't going to be accurate. The only other thing I can think of is having an option to prevent the system from running.

[alice-i-cecile]

Can we have a config resource that controls whether time is updated? This resource could even supply the "next time", and then get reset after that Instant is consumed.

[ShinySapphic]

Can we have a config resource that controls whether time is updated? This resource could even supply the "next time", and then get reset after that Instant is consumed.

Something like this?

pub struct TimeConfig {
    pub should_update: bool,
    pub next_time: Instant,
}

If so, would it be best to set next_time to Instant::now() or should it be wrapped in an Option? It might also be better if next_time holds a Duration instead assuming users are constantly adding that to Instant::now() to get the next time.

[alice-i-cecile]

Yep, that's the basic strategy I had in mind :) should_update probably needs a clearer name, but I think this is a good path forward.

would it be best to set next_time to Instant::now() or should it be wrapped in an Option?

I think just setting the next_time field directly should work fine.

It might also be better if next_time holds a Duration instead assuming users are constantly adding that to Instant::now() to get the next time

In my use case, the pattern is actually to set the exact time that was previously observed :) However when testing, you'll often want to increment it by a Duration. Maybe a

#[derive(Resource, Default)
pub enum TimeUpdateStrategy {
   #[default]
   Automatic,
   ManualInstant(Instant),
   ManualDuration(Duration),
}

[ShinySapphic]

If it's going to be a config resource, is there still a reason to have the update_with_instant method? It seems kind of redundant if you're just going to insert the desired instant/duration and then manually change the values anyways. I suppose it would be pretty annoying to constantly do:

if let Some(TimeUpdateStrategy::ManualInstant(mut instant)) = app.world.get_resource::<TimeUpdateStrategy>() {
    instant = prev_instant;
    app.update();
}

// or
app.insert_resource(TimeUpdateStrategy::ManualInstant(prev_instant));
app.update();

Maybe TimeUpdateStrategy should only have Automatic & Manual with no instant/duration values provided. Manual implies it's up to the user to grab the time resource and call time.update_with_instant(desired_time). app.update_with_instant could still be used to set a specific time and have update() called. If the UpdateStrategy isn't set to Manual, calling that method could either not do anything or would set the strategy to manual for a single frame.

[alice-i-cecile]

If it's going to be a config resource, is there still a reason to have the update_with_instant method? It seems kind of redundant if you're just going to insert the desired instant/duration and then manually change the values anyways

I think we can cut it; the insert_resource strategy is very clear and idiomatic. Then we can point to the TimeUpdateStrategy resource (with three variants) from the App::update docs.

[hymm]

You probably want to try_recv on the channel in the manual cases too and discard the value. The time channel only can have 2 times in it and will panic if the channel gets full.

[hymm]

Not sure if this matters, but this is a change in behavior from before. This is updating the time with Instant::now(), while the old code was skipping updating the time if nothing was received. I'm not sure if either way is more right or wrong as skipping an update could be just as bad as assuming Instant::now() depending on what caused the time not to be sent.

[ShinySapphic]

I'm not sure either but it's probably not expected behavior. I think it might be safer to skip updating like before because at least then future delta time values will remain some what accurate. Then again I don't really know. Thoughts?

[hymm]

I think we can keep the current behavior for now. With the way bevy is sending the time from render right now this shouldn't ever be hit. Issues would only arise when people are doing custom stuff. In those cases, I'm not sure what the better behavior would be. We can just punt that into the future when we have more concrete evidence of what is better. It'll be easy to change things to skip the update if this is found to be problematic.

[alice-i-cecile]

Can you update the PR description and title to reflect the new changes?

[alice-i-cecile]

The PR description is still outdated and the comments should be resolved. Do fix that for us if you notice to make writing patch notes easier? I won't block on it though.

bors r+

[bors]

Build failed (retrying...):

• check-compiles

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12

[hymm]

Might be worth adding the breaking change label to this PR as it changes the behavior of time.delta on startup.

Before it would be [0, 0, correct] and this PR changes it to be [0, "approximately the time between the time_system and present_frame", correct].