-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Merged by Bors] - add time wrapping to Time #5982
Conversation
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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 feel like the global max wrapping period should be considered a global default and the method should take an optional wrapping period argument.
As for the name and documentation, I’ve seen lots of uses of time since startup being cast to f32 without consideration and indeed I’ve done it myself without thinking. It would be good if documentation either pointed you to this method and/or perhaps the method name would suggest when to use it. I thought of seconds_since_startup_f32_wrapped() for example. This should also show up in autocompletion lists.
I think @cart should probably review this too. |
I originally started with
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 |
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.
Just minor formatting suggestions and a nit on assert_float_eq
It is the second since startup, but wrapped. :)
Hahaha. I was going to suggest the same with the enum. And if only we could have optional function arguments. :B |
Co-authored-by: Afonso Lage <lage.afonso@gmail.com>
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. |
Moving the conversation about this in #5752 back here. I understand its motivation, but putting this "global wrapping period" on Wouldn't the most idiomatic pattern be to use a repeating |
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 |
@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. |
I didn't mean in a super negative way. More in the sense of clutter. I think it's "hacky" because 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
Glad you think it would work too. Honestly, I could go either way. |
I think that a
|
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.
Some doc suggestions.
Co-authored-by: Cameron <51241057+maniwani@users.noreply.github.com>
This method exists because converting a IMO it's beneficial to keep anyway (as would adding a non-wrapped |
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 |
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.
Personally I don't think it's needed, since the same effect can be achieved by doing a modulo operation from user code. Anyway, I left some comments.
@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. |
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. |
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 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 #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 Taking all of this into account. I see a few possibility.
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 |
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. |
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? |
I'm okay with putting it on You should be able to run a system like this right after the system that updates #[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 But yeah, I'm cool with putting this inside |
I updated the PR to use option 2. until we have more feedback. |
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 We can also make the wrapping period more friendly towards trigonometric functions (which is the most common case) by wrapping every |
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). |
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. |
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. |
bors r+ |
# 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 #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. Co-authored-by: Charles <IceSentry@users.noreply.github.com>
Pull request successfully merged into main. Build succeeded: |
# 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 bevyengine#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. Co-authored-by: Charles <IceSentry@users.noreply.github.com>
# 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 bevyengine#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. Co-authored-by: Charles <IceSentry@users.noreply.github.com>
# 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 bevyengine#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. Co-authored-by: Charles <IceSentry@users.noreply.github.com>
Objective
f32
. Unfortunately this suffers from floating precision issues pretty quickly. The standard approach to this problem is to wrap the time after a given periodSolution
seconds_since_last_wrapping_period
method onTime
that returns af32
that is theseconds_since_startup
modulo themax_wrapping_period
Changelog
Added
seconds_since_last_wrapping_period
toTime
Additional info
I'm very opened to hearing better names. I don't really like the current naming, I just went with something descriptive.