Make Time update_with_instant pub #2549

ostwilkens · 2021-07-26T17:17:47Z

Objective

A lot of systems depend on Time to implement frame-independent logic.
To be able to write tests for these systems, one needs to be able to mock Time. By mocking time, it's possible to write tests that confirm systems are frame-independent.

Solution

To mock Time, one needs to be able to manually update the Time resource with an Instant defined by the developer.
There is a function for this, but it's currently only public within the crate. I propose this function is exposed to the developer, to make it easy to write tests for systems utilizing the Time resource.

alice-i-cecile · 2021-07-26T17:22:58Z

bors try

alice-i-cecile · 2021-07-26T17:25:25Z

Welcome! I agree with the need for this, and think this is an excellent, simple solution.

As updating the time step manually is typically going to be a mistake in end user code outside of tests, we should add careful doc strings to both this method and the Time type to explain the correct usage :)

This would also be a nice addition to testing_systems.rs, found in the root tests folder.

alice-i-cecile · 2021-07-26T17:31:44Z

Actually, this may also be useful for running simulations at faster than real time too. I've seen that requested before too; we should try to document that use case too. A full example might be better for that (you'll need to mess with one of the default plugins), but that work could be saved for a later PR.

LegNeato · 2021-07-26T17:31:53Z

Perhaps only make this pub under #[test] to remove that footgun?

bjorn3 · 2021-07-26T17:35:35Z

Perhaps only make this pub under #[test] to remove that footgun?

That is not possible. #[cfg(test)] only works when the current crate is being tested, not any dependent crate.

bors · 2021-07-26T17:39:30Z

try

Build succeeded:

mockersf · 2021-07-27T00:07:15Z

Actually, this may also be useful for running simulations at faster than real time too. I've seen that requested before too; we should try to document that use case too. A full example might be better for that (you'll need to mess with one of the default plugins), but that work could be saved for a later PR.

It's probably a bad idea to manually update with an instant in this case, as it would break all deltas exposed by Time. To change the speed from real time it's much simpler to apply a factor to everything you get from Time, or to recode the resource including the factor, it's not very complicated...

To make this useful outside of test, one would need to disable the time_system system

bevy/crates/bevy_core/src/time/time.rs

Lines 87 to 89 in f71f93c

    
           pub(crate) fn time_system(mut time: ResMut<Time>) { 
        
               time.update(); 
        
           }

I don't think that's a good idea, so I would prefer clear doc comments explaining this is only useful for tests. Could a feature to enable test helpers work?

alice-i-cecile · 2021-07-27T00:31:03Z

To make this useful outside of test, one would need to disable the time_system system

This was what I had in mind; it's much more useful in scientific settings but I could see it being useful for accelerated simulation in ordinary games (e.g. for machine learning or automated integration tests).

Out of scope for this though, and I'd want to try testing that strategy on a full game to see if anything breaks before recommending it.

ostwilkens · 2021-07-27T12:15:17Z

In the case of non-linear or scaled time as a feature in games, it might be recommended to keep track of simulated time as a separate resource. In such a case it would be nice to be able to wrap or alias Time, and modify it as needed for your usecase. It's something that should be made possible by this change.

I'll add the doc strings discussed.

ostwilkens · 2021-07-27T12:27:32Z

I'm not sure if it's feasible, or a good idea, but one way to prevent breaking the default Time resource would be to somehow declare the built-in resource as read-only (outside the bevy crate).

ostwilkens · 2021-07-27T12:55:01Z

Sorry about that; rebased from 0.5.0 release.. should I open a new PR?

mockersf · 2021-07-27T13:36:36Z

Sorry about that; rebased from 0.5.0 release.. should I open a new PR?

Yup, if you can't clean this PR, it would probably be best to open a new one

ostwilkens · 2021-07-27T13:50:59Z

Sorry about the fuzz, I think it's cleaned up now. Open to input on the doc string and test.

mdickopp · 2022-04-10T17:43:13Z

This change would be very useful to me as well, so I would like to help. What needs to be done to get this ready to be merged?

alice-i-cecile · 2022-04-10T18:45:41Z

crates/bevy_core/src/time/time.rs

@@ -31,7 +31,8 @@ impl Time {
        self.update_with_instant(now);
    }

-    pub(crate) fn update_with_instant(&mut self, instant: Instant) {
+    /// Update time with a specified [`Instant`], for use in tests. Should never be called on the default Time resource.


Suggested change

/// Update time with a specified [`Instant`], for use in tests. Should never be called on the default Time resource.

/// Update time with a specified [`Instant`]

///

/// This method is for use in tests, and should generally not be called on the [`Time`] resource as part of your app.

/// Doing so will conflict with the `time_system` called by [`CorePlugin`], and result in inaccurate performance measurements.

Just inaccurate timekeeping in general. I would keep the recommendation to only use this to write tests.

alice-i-cecile · 2022-04-10T18:51:14Z

So to get this merged, the next step is that we need at least two approvals, including from community members, and all blocking concerns addressed.

On my end, I want to see:

Better doc strings, explaining exactly what the risks are when calling this inside an App.
Sign-off from @maniwani, who's been redesigning how Bevy handles time.

maniwani

I'm cool with making update_with_instant pub since update already is.

I will say tho that the featured test seems a little misleading.

At the end it highlights the results between 30 FPS and 60 FPS being similar after a single frame as if that would be the case forever (which is false, would diverge pretty quickly). Integrating by time.delta_seconds() does decouple motion from the framerate but not in a way that is repeatable or consistent between different FPS.

You only get real framerate independence if you have a constant step size (and then FixedTimestep or a dedicated thread).

something something running simulations faster than real-time

Is what #3002 is for.

tim-blackbird · 2022-04-10T23:39:36Z

I believe most people understand framerate-independence to mean that motion is separate from the framerate. i.e. the game does not appear to move in slow-motion when the framerate dips, or vise versa. Rather than separating simulation-time from real-time in a fixed timestep setting.

With that in mind, I don't think the test is pretending to be more accurate than it is.

tests/how_to_test_systems.rs

+    let result_30fps = test_fps(30);
+    let result_60fps = test_fps(60);


maniwani · 2022-04-11T01:27:28Z

I believe most people understand framerate-independence to mean [...]

In my experience, time-related things confuse users a lot.

I understand this is the kind of test OP wanted to mock, but I'd prefer that we didn't feature a test with this premise on the off-chance someone interprets it as an "officially-endorsed" way of handling physics.

This is all fine for UI, etc, but if users want stable game physics, they should use a fixed timestep (with interpolation to make it look smooth) because that's the standard and correct way to do it.

With that in mind, I don't think the test is pretending to be more accurate than it is.

I was more pointing out that the test—checking that the motion didn't desync by more than an arbitrary amount under arbitrary FPS values after an arbitrarily small duration—seems pretty convoluted if the point is just to confirm that we didn't forget to multiply by time.delta_seconds().

(Edit: But yeah, IMO we shouldn't have a test whose premise is checking the "consistency" of variable timestep motion.)

tim-blackbird · 2022-04-11T10:03:36Z

seems pretty convoluted if the point is just to confirm that we didn't forget to multiply by time.delta_seconds().

Sorry, I probably got too preoccupied with your comment. I agree with you here. what's being tested is basically how much float addition drifts over 30 iterations compared to 60 iterations. I'm in favor of removing the test.

mdickopp · 2022-04-11T15:44:05Z

I'm not the OP, so I cannot update the OP's branch. Should I open a new PR?

I'm in favor of removing the test.

I would like to rework the test into an example demonstrating how the function can be used in a test. What do you think?

alice-i-cecile · 2022-04-11T15:48:36Z

I'm not the OP, so I cannot update the OP's branch. Should I open a new PR?

Yep sounds great: just link this PR in the description and we can close this one out.

I would like to rework the test into an example demonstrating how the function can be used in a test. What do you think?

Yes please! I think a doc test on Time itself would be a nice place for this :) That feels like the most likely place users will look if they're wondering about "how do I mock this".

ostwilkens · 2022-04-11T15:59:49Z

I don't currently have the environment set up to assist on this, I'm glad it's being looked into. Feel free to rework it in a new PR.

mdickopp · 2022-04-13T19:00:47Z

I opened #4469 as a follow-up/replacement PR to this one.

alice-i-cecile · 2022-04-13T19:30:57Z

Closed in favor of #4469

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

# Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to #2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

…4469) # Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to bevyengine#2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

@ostwilkens

…4469) # Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to bevyengine#2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org> (cherry picked from commit 93cee3b)

@ostwilkens

…4469) # Objective To test systems that implement frame rate-independent update logic, one needs to be able to mock `Time`. By mocking time, it's possible to write tests that confirm systems are frame rate-independent. This is a follow-up PR to bevyengine#2549 by @ostwilkens and based on his work. ## Solution To mock `Time`, one needs to be able to manually update the Time resource with an `Instant` defined by the developer. This can be achieved by making the existing `Time::update_with_instant` method public for use in tests. ## Changelog - Make `Time::update_with_instant` public - Add doc to `Time::update_with_instant` clarifying that the method should not be called outside of tests. - Add doc test to `Time` demonstrating how to use `update_with_instant` in tests. Co-authored-by: Martin Dickopp <martin@zero-based.org>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 26, 2021

bors bot added a commit that referenced this pull request Jul 26, 2021

Try #2549:

02171a2

alice-i-cecile added A-Core Common functionality for all bevy apps C-Usability A simple quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Jul 26, 2021

ostwilkens closed this Jul 27, 2021

ostwilkens force-pushed the pub-update_with_instant branch from 06e312f to 5583f9a Compare July 27, 2021 13:45

ostwilkens added 3 commits July 27, 2021 15:48

Make Time update_with_instant pub

5859b84

Add doc string to update_with_instant

622fed6

Add test confirm_system_is_framerate_independent

8f20b2e

ostwilkens reopened this Jul 27, 2021

alice-i-cecile added the S-Needs-Review label Sep 22, 2021

cart removed the S-Needs-Review label Dec 16, 2021

alice-i-cecile reviewed Apr 10, 2022

View reviewed changes

maniwani approved these changes Apr 10, 2022

View reviewed changes

tim-blackbird reviewed Apr 10, 2022

View reviewed changes

tests/how_to_test_systems.rs

Comment on lines +162 to +163

let result_30fps = test_fps(30);

let result_60fps = test_fps(60);

This comment was marked as outdated.

Sign in to view

mdickopp mentioned this pull request Apr 13, 2022

[Merged by Bors] - Make Time::update_with_instant public for use in tests #4469

Closed

alice-i-cecile closed this Apr 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Time update_with_instant pub #2549

Make Time update_with_instant pub #2549

ostwilkens commented Jul 26, 2021

alice-i-cecile commented Jul 26, 2021

alice-i-cecile commented Jul 26, 2021 •

edited

Loading

alice-i-cecile commented Jul 26, 2021

LegNeato commented Jul 26, 2021

bjorn3 commented Jul 26, 2021

bors bot commented Jul 26, 2021

mockersf commented Jul 27, 2021 •

edited

Loading

alice-i-cecile commented Jul 27, 2021

ostwilkens commented Jul 27, 2021

ostwilkens commented Jul 27, 2021

ostwilkens commented Jul 27, 2021

mockersf commented Jul 27, 2021

ostwilkens commented Jul 27, 2021 •

edited

Loading

mdickopp commented Apr 10, 2022

alice-i-cecile Apr 10, 2022 •

edited

Loading

maniwani Apr 10, 2022

alice-i-cecile commented Apr 10, 2022

maniwani left a comment •

edited

Loading

tim-blackbird commented Apr 10, 2022

This comment was marked as outdated.

maniwani commented Apr 11, 2022 •

edited

Loading

tim-blackbird commented Apr 11, 2022 •

edited

Loading

mdickopp commented Apr 11, 2022

alice-i-cecile commented Apr 11, 2022

ostwilkens commented Apr 11, 2022

mdickopp commented Apr 13, 2022

alice-i-cecile commented Apr 13, 2022

		let result_30fps = test_fps(30);
		let result_60fps = test_fps(60);

Make Time update_with_instant pub #2549

Make Time update_with_instant pub #2549

Conversation

ostwilkens commented Jul 26, 2021

Objective

Solution

alice-i-cecile commented Jul 26, 2021

alice-i-cecile commented Jul 26, 2021 • edited Loading

alice-i-cecile commented Jul 26, 2021

LegNeato commented Jul 26, 2021

bjorn3 commented Jul 26, 2021

bors bot commented Jul 26, 2021

try

mockersf commented Jul 27, 2021 • edited Loading

alice-i-cecile commented Jul 27, 2021

ostwilkens commented Jul 27, 2021

ostwilkens commented Jul 27, 2021

ostwilkens commented Jul 27, 2021

mockersf commented Jul 27, 2021

ostwilkens commented Jul 27, 2021 • edited Loading

mdickopp commented Apr 10, 2022

alice-i-cecile Apr 10, 2022 • edited Loading

Choose a reason for hiding this comment

maniwani Apr 10, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Apr 10, 2022

maniwani left a comment • edited Loading

Choose a reason for hiding this comment

tim-blackbird commented Apr 10, 2022

This comment was marked as outdated.

maniwani commented Apr 11, 2022 • edited Loading

tim-blackbird commented Apr 11, 2022 • edited Loading

mdickopp commented Apr 11, 2022

alice-i-cecile commented Apr 11, 2022

ostwilkens commented Apr 11, 2022

mdickopp commented Apr 13, 2022

alice-i-cecile commented Apr 13, 2022

alice-i-cecile commented Jul 26, 2021 •

edited

Loading

mockersf commented Jul 27, 2021 •

edited

Loading

ostwilkens commented Jul 27, 2021 •

edited

Loading

alice-i-cecile Apr 10, 2022 •

edited

Loading

maniwani left a comment •

edited

Loading

maniwani commented Apr 11, 2022 •

edited

Loading

tim-blackbird commented Apr 11, 2022 •

edited

Loading