Objective

Sometimes, like when using shaders, you can only use a time value in f32. Unfortunately this suffers from floating precision issues pretty quickly. The standard approach to this problem is to wrap the time after a given period
This is necessary for https://github.com/bevyengine/bevy/pull/5409

Solution

Add a seconds_since_last_wrapping_period method on Time that returns a f32 that is the seconds_since_startup modulo the max_wrapping_period

Changelog

Added seconds_since_last_wrapping_period to Time

Additional info

I'm very opened to hearing better names. I don't really like the current naming, I just went with something descriptive.

Sep 14 '22 00:09 IceSentry

I think @cart should probably review this too.

Sep 14 '22 02:09 superdump

I originally started with seconds_since_startup_wrapped but since it's not actually since startup it felt a bit misleading.

I feel like the global max wrapping period should be considered a global default and the method should take an optional wrapping period argument.

Interesting idea, I didn't do this because originally it wasn't just a modulo, but when I realized I was pretty much just reimplementing a less efficient modulo I went with this instead. I don't like that most use of this function would probably just be passing it a None value though. Also, if we go this way, we'll probably want an enum with Default, Custom(Duration), Max instead of an optional param.

Sep 14 '22 03:09 IceSentry

I originally started with seconds_since_startup_wrapped but since it's not actually since startup it felt a bit misleading.

It is the second since startup, but wrapped. :)

I feel like the global max wrapping period should be considered a global default and the method should take an optional wrapping period argument.

Interesting idea, I didn't do this because originally it wasn't just a modulo, but when I realized I was pretty much just reimplementing a less efficient modulo I went with this instead. I don't like that most use of this function would probably just be passing it a None value though. Also, if we go this way, we'll probably want an enum with Default, Custom(Duration), Max instead of an optional param.

Hahaha. I was going to suggest the same with the enum. And if only we could have optional function arguments. :B

Sep 14 '22 07:09 superdump

I added the enum with a max duration of 1 day, not sure what that value should be, but this seems like a safe max value. f32 have about 6-7 significant numbers and a day is 86400 seconds. Add a few decimal places for millis and you already start to get errors.

Sep 14 '22 14:09 IceSentry

Moving the conversation about this in #5752 back here.

I understand its motivation, but putting this "global wrapping period" on Time feels like a hack. I don't mean to be dismissive (I know wrapping is a good practice), but why do shaders and other applications even use "total elapsed app time"? Is it just because it's continuous and updated every frame?

Wouldn't the most idiomatic pattern be to use a repeating Timer with your specified period? If you want to provide that globally, I think it would be clearer signalling to provide it as a separate resource. Because we would never completely remove access to the non-wrapping values, and having "non-wrapping f32" convenience methods will actually help us warn users about precision loss and point them to your solution.

Sep 14 '22 16:09 maniwani

Is it just because it's continuous and updated every frame?

Pretty much, yes, but it's also important to be continuous for a long time and for that time to be configurable.

The wrapping functions are intended as convenience methods too, not to replace the seconds_since_startup function. I'm not sure why you consider that hacky.

The main reason I wanted this on Time and not just something separate is that I needed this information to be globally available for shaders and I wanted to make this easily discoverable for anyone that needs a wrapped time value. Having a GlobalWrappedTimer or whatever it would be called would also work, but it would be harder to discover.

Sep 14 '22 16:09 IceSentry

@superdump I just realized, using an enum doesn't work because you want to be able to configure how long the period is globally if you want to change the value passed to the shaders, but with an enum like I did here it would just always use the Default value in shaders. I guess I could add a Res to configure this in my other PR.

Sep 14 '22 16:09 IceSentry

I'm not sure why you consider that hacky.

I didn't mean in a super negative way. More in the sense of clutter. I think it's "hacky" because Time is a clock. It's purpose is "tracking the time elapsed since startup and since the last update", and this wrapped value is neither of those things.

It would only exist to address the specific scenario of "I need a monotonically increasing time-related value, and app elapsed time usually fits the bill, but it loses significant precision over time as an f32".

Having a GlobalWrappedTimer or whatever it would be called would also work, but it would be harder to discover.

Glad you think it would work too. Honestly, I could go either way. Time::*_wrapped methods are clear enough. This just struck me as something you'd normally use a Timer for, so it felt more appropriate. But I'll defer to @alice-i-cecile.

Sep 14 '22 17:09 maniwani

Having a GlobalWrappedTimer or whatever it would be called would also work, but it would be harder to discover.

I think that a WrappedTime resource is actually clearer here, and marginally reduces the size of the resource. IMO if we use that in our examples + have docs pointing to it from Time I think it'll be discoverable enough.

seconds_since_startup should simply be removed as part of this PR; it can be recovered from time_since_startup for users who really care, and is effectively just a footgun.

Sep 14 '22 17:09 alice-i-cecile

seconds_since_startup should simply be removed as part of this PR; it can be recovered from time_since_startup for users who really care, and is effectively just a footgun.

This method exists because converting a Duration into an f64 isn't free and the compiler isn't smart enough to avoid it if you repeatedly call the method (see here).

IMO it's beneficial to keep anyway (as would adding a non-wrapped f32 method). Gives us more places to document this situation and makes it easier to identify when users encounter it. The only other thing I would do is log a message when the time since startup reaches a point where its f32 precision falls below a certain threshold.

Sep 14 '22 18:09 maniwani

Yeah, I'd prefer not touch the current existing apis and let #5752 deal with updating the apis. I'll think about it a bit more, but I'm warming up to the idea of just having a WrappedTime resource and link it from the docs on Time

Sep 14 '22 18:09 IceSentry

@Nilirad just to clarify a little. The point is to have a globally available value that can be used for shaders and for other parts of the app to want the same wrapped value if they need to.

The implementation was also changed which made a few things inconsistent and after what @maniwani said I was already planning to change most of this to use a separate Resource instead.

As for it not being necessary because it's just a modulo, sure, that's true that it's easy enough to do without a specific api, but the point is to have something easy to use and consistent. Not everyone is familiar with the modulo operator, especially not beginners. I know it took me a few years before I could just intuitively think about just using a modulo to wrap around like this.

Sep 14 '22 20:09 IceSentry

The point is to have a globally available value

I struggle to understand the need of having a global wrapped time value. From what I understand, a wrapped time should only be used in periodic phenomena, like animations and shader uniforms, but such periods are wildly variable by nature. Also, giving the user the responsibility to specify the duration (I know there is a “default”, but the method still requires to provide an argument) does not transmit that intent.

Sep 15 '22 07:09 Nilirad

The period for an animation in a shader needs to be configurable, but it needs to be bound to a monotonically increasing value that is limited to an f32. In every game engine I'm aware of (and shadertoy) this value is the time since startup as an f32, the main issue here being that this f32 becomes very imprecise after a few hours which would cause shader based animations to behave strangely. The point of the period here is for the global period to wrap at an expected time, not to be the period of an animation.

As far as to why it needs to be global, well, that's because when passed to shaders it will always pass the same value to every shaders and therefore is a global value in this context. This is also something that every game engine supports because it's a generally useful thing to have and many people have expressed the need for it already. (This is already done in #5409, it's just missing the wrapping part)

Based on this, it would make sense to only add a resource to configure the period, since it would support making it globally configurable.

Based on what @maniwani recently talked about on discord though, it made me realize that we also need to be able to pause that value. Pausing time, while not currently supported will be supported as soon as https://github.com/bevyengine/bevy/pull/5752 is merged. We need to be able to pause this value, otherwise if someone pauses the global time, but shader based animation keep moving it would look really weird. Of course we'll need to have a variant that ignores pauses.

The last reason why I wanted to add it directly on Time is that precision loss happens fairly quickly when using f32 so offering a way to access an f32 value that wraps and correctly reacts to pausing and unpausing seems useful enough to just be added directly on Time as a convenience method alongside non wrapping methods instead of just adding a line in the Time docs to avoid casting to an f32 without wrapping it first.

Taking all of this into account. I see a few possibility.

Close this PR and simply add a period configuration that is strictly used for shaders in the related PR and just handle everything separately from Time
Add the wrap period as a public field on Time and add utility functions to get a wrapped f32 directly on Time
- Default to 1 hour because it's a safe value and is also what godot uses
- Making the period globally configurable is important, but I don't think it actually needs a way to change the period locally like how the current PR is doing. If you need a smaller period for an animation you should handle that directly.
Add the wrap period configuration on Time, don't add convenience functions but recommend people to use the wrap period with a modulo when casting to an f32

I would like to hear which approach would be preferred here. Personally, I'd tend to go with 2, since I think this is still strongly related to Time and not strictly just a value that constantly goes up so it seems like that's where it should be handled, but clearly there's some pushback.

Sep 15 '22 22:09 IceSentry

The goal here with wrapping of elapsed time is for when you need an elapsed time value that does not suffer loss of precision beyond some threshold. I was thinking originally that one might say ‘give me the elapsed time in seconds, but wrap it around to give me a value with at most this amount of error’ and it would wrap accordingly. I see @Nilirad ’s point though about wanting to specify a wrapping period that matches a period of use such as for sinusoidal mapping of time. Hmm.

This all came because we want to have time in shaders and there we can only have f32 and then we’ll get noticeable loss of precision pretty quickly. Suggestions for solutions definitely welcome.

Sep 15 '22 22:09 superdump

I agree with everything @IceSentry wrote there though I’m not sure about whether there are more available options nor which to choose at this moment.

@cart any input?

Sep 15 '22 22:09 superdump

I'm okay with putting it on Time, but let me clarify the separate resource idea.

You should be able to run a system like this right after the system that updates Time.


#[derive(Resource)]
pub struct WrappedTime {
    // repeating timer
    timer: Timer,
    elapsed_secs_f32: f32,
    elapsed_secs_f64: f64,
}

impl WrappedTime {
    pub(crate) fn update(&mut self, time: &Time) {
        self.timer.reset();
        // pretty sure this is O(1)
        self.timer.tick(time.elapsed());
        self.elapsed_secs_f32 = self.timer.elapsed().as_secs_f32();
        self.elapsed_secs_f64 = self.timer.elapsed().as_secs_f64();
    }

    /* ... */
}

// system
fn updated_wrapped_time(
    time: Res<Time>,
    mut wrapped_time: ResMut<WrappedTime>,
) {
    wrapped_time.update(time);
}

Like this, as long as Time itself does the right thing, you shouldn't see any issues, pausing or not. You'll always see the current elapsed time modulo the wrapping period. (And with global time scaling, we would just add another set of fields.)

But yeah, I'm cool with putting this inside Time for convenience/fewer system params. Probably easier that way if you want to change the wrapping period mid-frame.

Sep 15 '22 22:09 maniwani

I updated the PR to use option 2. until we have more feedback.

Sep 17 '22 18:09 IceSentry

Ok, I understand the ergonomics of using a global shader value. It is clearly more important than the minor visual glitch that can be introduced by the modulo operation.

We can document somewhere (better on the bevy_render crate) that using the global time in shaders may cause visual artifacts in the moment the time wraps. This way, a user can choose a customized solution for some prominent animations (e.g. item enchanting glow in Minecraft).

We can also make the wrapping period more friendly towards trigonometric functions (which is the most common case) by wrapping every 1146 * PI seconds by default, which is approximately equal to 3600.26518.

Sep 18 '22 13:09 Nilirad

Generally I would expect people to scale and possibly offset the time value before taking the sine or cosine to adjust the frequency and phase. I haven’t thought so hard but my gut says that would mess up basically any attempts to use a helpful wrapping value, right? I’d expect people to use something like sin(2 pi f t).

Sep 18 '22 17:09 superdump

I haven’t thought so hard but my gut says that would mess up basically any attempts to use a helpful wrapping value, right?

Hmm, yes, you're definitely right. Scaling the period by a non-integer factor will create a discontinuity even if the wrapping value is an integer multiple of 2π. Furthermore, the longer is the wrapping period, the more chaotic the discontinuity will be across small period changes. Sure it is a price we have to pay if we want a global shader uniform for time. Still better on the longer side that letting the time value go adrift, but using a local uniform does not have this issue.

PS: a user could solve the discontinuity problem by setting the global time wrapping period as the least common multiple of all animation periods, but it could easily grow to a huge number.

Sep 20 '22 10:09 Nilirad

Sorry for the delay. I've been fully focused on getting specific areas merged (such as stageless and "components as bundles") and I'm (once again) behind on my discord notifications.

I agree that "option 2" feels best. I'll give this a quick review.

Sep 22 '22 00:09 cart

bors r+

Sep 23 '22 20:09 cart

Pull request successfully merged into main.

Build succeeded:

Sep 23 '22 20:09 bors[bot]

add time wrapping to Time

Objective

Solution

Changelog

Additional info