Add a global time scale for all effects #267

jannik4 · 2024-01-14T17:01:51Z

~~Add a global time scale for all effects. Currently very WIP. Probably needs:~~

~~Some tests/examples~~
~~More docs~~

Now reimplemented (and documented) using the Time<T> resource from bevy.

(Could still need some examples/tests when we have settled on the design)

Closes #239. Pausing should be possible by setting EffectTimeScale to zero.

djeedai · 2024-01-15T18:44:43Z

src/render/mod.rs

+    sim_params.time += ***time_scale * time.delta_seconds_f64();
+    sim_params.delta_time = (***time_scale * time.delta_seconds_f64()) as f32;


Isn't this essentially re-implementing partially Time<T>? Could we instead simply use Time<EffectSimulation>? See my comment on #239 for details.

Looks like yes, just had a proper look at Time<T> for the first time. Something like Time<EffectSimulation> seems like a good idea then.

I would be happy to try to implement this. I can then imagine a total of three "Time/Delta Time" values:

sim_params.real_time: From Time<Real>

sim_params.virtual_time: From Time<Virtual>

sim_params.time: From Time<EffectSimulation>

Where Time<EffectSimulation> is then a scaled version of Time<Virtual>. This would allow pausing/scaling time by both bevy's Time<Virtual> and by hanabi's Time<EffectSimulation>. All built in shader code would then use sim_params.time, but users can also use the other two versions.

This should be quite flexible. (But also adds a bit complexity)

Alternatively, you could of course leave out sim_params.real_time and sim_params.virtual_time for now and only add them if someone really needs them.

Implementation wise this seems pretty straight forward:

bevy provides virtual_time_system

So we can then just update Time<EffectSimulation> after that by Time<Virtual>::delta()

Sounds all good to me. SimParams is small and uploaded once per frame, not per effect, so I'm totally fine adding more stuffs to it (including real/virtual times). Chances are, with DMA copy alignment (256 bytes?) we're already copying zeros instead at the minute, so it will make no difference.

djeedai

This looks good overall but there's a number of small issues I think. Maybe some test would help validate the behavior here? Thanks!

src/time.rs

src/render/mod.rs

src/time.rs

djeedai · 2024-02-17T11:41:40Z

src/time.rs

+        // avoid rounding when at normal speed
+        virt_delta
+    };
+    effect_simulation.context_mut().effective_speed = effective_speed;


I don't understand, effective_speed was read directly from the simulation clock, and is re-written back into it, but doesn't seem to be affected in any way by the virtual clock. Is this missing some * virtual_speed somewhere?

effective_speed is a local variable a few lines above and is set depending on paused and relative_speed.

effective_speed is just zero when paused and relative_speed when running. It is and should be independent of the virtual time.

The only thing that is dependent on the virtual time is the resulting delta for that frame.

This is pretty much the same as here (without the extra clamping):

https://github.com/bevyengine/bevy/blob/abb8c353f49a6fe9e039e82adbe1040488ad910a/crates/bevy_time/src/virt.rs#L229-254

Yes but Virtual is a self-contained clock whereas this one is based on Virtual. What is strange to me is that we document "the final speed is the product of the two clock speeds" but then when we calculate we only store this clock speed in effective_speed, not the product. So it's "effective" because it takes into account pausing, but it's still not "effective" as in being the final value used in practice.

You can't compare with Bevy code, there's no clock in Bevy which is relative to another clock as far as I know.

Sure, Time<Virtual> is based on Time<Real>: https://github.com/bevyengine/bevy/blob/abb8c353f49a6fe9e039e82adbe1040488ad910a/crates/bevy_time/src/virt.rs#L271-279

Most of the time code is literally copy-pasted from Time<Virtual> and adapted (e.g. remove clamping).

Ah, I think I see your point: Because Time<Virtual> already has time scaling.

This should fix it: 1f9a23e

Edit: Also updated the docs now.

djeedai · 2024-02-17T11:42:47Z

src/spawn.rs

@@ -800,7 +800,7 @@ mod test {
        // app.add_plugins(DefaultPlugins);
        app.init_asset::<Mesh>();
        app.add_plugins(VisibilityPlugin);
-        app.init_resource::<Time>();
+        app.init_resource::<Time<EffectSimulation>>();


I'd assume this also needs init_resource::<Time>() since we need the base Virtual time. This is an emty app for testing, so won't have Time by default.

Time is not need as the time is advanced manually in the tests. (It was the same before, I didn't change anything there)

Ah ok and we don't insert the effect_simulation_time_system system so we don't need to read Time<Virtual>.

Multiply by effective speed of virtual clock

jannik4 · 2024-02-26T01:12:59Z

Rebased on main and added a test for the effect simulation time. Could you take another look at this, @djeedai?

djeedai

Looks all good, thanks for the update!

djeedai · 2024-03-03T09:50:33Z

src/spawn.rs

@@ -800,7 +800,7 @@ mod test {
        // app.add_plugins(DefaultPlugins);
        app.init_asset::<Mesh>();
        app.add_plugins(VisibilityPlugin);
-        app.init_resource::<Time>();
+        app.init_resource::<Time<EffectSimulation>>();


Ah ok and we don't insert the effect_simulation_time_system system so we don't need to read Time<Virtual>.

src/time.rs

djeedai reviewed Jan 15, 2024

View reviewed changes

jannik4 force-pushed the main branch from 5d62841 to 8314339 Compare January 16, 2024 22:37

jannik4 marked this pull request as ready for review January 16, 2024 23:01

jannik4 requested a review from djeedai January 17, 2024 00:58

djeedai requested changes Feb 17, 2024

View reviewed changes

jannik4 force-pushed the main branch from a77fded to 8acd81a Compare February 22, 2024 15:27

jannik4 added 10 commits February 26, 2024 01:24

EffectTimeScale

9527ede

Scale spawners delta time

f26ff37

Implement time scale by Time<T>

2d076de

Init resource Time<EffectSimulation>

b485f35

Improve docs

e9c44ee

Fix times in extract_effects

df0f7d3

Fix effective_speed

6c62dc4

Multiply by effective speed of virtual clock

Fix docs for EffectSimulationTime::effective_speed

9f09261

Remove unused import

5e5b00a

Add test for effect simulation time

5b2d3a8

jannik4 force-pushed the main branch from 8acd81a to 5b2d3a8 Compare February 26, 2024 01:10

Register Time<EffectSimulation> type

cbfd17c

jannik4 requested a review from djeedai February 27, 2024 19:39

djeedai approved these changes Mar 3, 2024

View reviewed changes

Fix docs

4f98ad0

djeedai approved these changes Mar 4, 2024

View reviewed changes

Fix docs test

025ff1f

djeedai added C - enhancement New feature or request C - breaking change A breaking API or behavior change labels Mar 5, 2024

djeedai merged commit 4933a35 into djeedai:main Mar 5, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a global time scale for all effects #267

Add a global time scale for all effects #267

jannik4 commented Jan 14, 2024 •

edited

Loading

djeedai Jan 15, 2024 •

edited

Loading

jannik4 Jan 15, 2024 •

edited

Loading

jannik4 Jan 15, 2024

djeedai Jan 16, 2024

djeedai left a comment

djeedai Feb 17, 2024

jannik4 Feb 17, 2024

jannik4 Feb 17, 2024

djeedai Feb 17, 2024

djeedai Feb 17, 2024

jannik4 Feb 17, 2024

jannik4 Feb 17, 2024

jannik4 Feb 17, 2024

jannik4 Feb 17, 2024 •

edited

Loading

djeedai Feb 17, 2024

jannik4 Feb 17, 2024

djeedai Mar 3, 2024

jannik4 commented Feb 26, 2024

djeedai left a comment

djeedai Mar 3, 2024

		sim_params.time += **time_scale time.delta_seconds_f64();
		sim_params.delta_time = (**time_scale time.delta_seconds_f64()) as f32;

Add a global time scale for all effects #267

Add a global time scale for all effects #267

Conversation

jannik4 commented Jan 14, 2024 • edited Loading

djeedai Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

jannik4 Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djeedai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannik4 Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannik4 commented Feb 26, 2024

djeedai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannik4 commented Jan 14, 2024 •

edited

Loading

djeedai Jan 15, 2024 •

edited

Loading

jannik4 Jan 15, 2024 •

edited

Loading

jannik4 Feb 17, 2024 •

edited

Loading