Add a DelayedCommands helper to support arbitrary delayed commands#23090
Add a DelayedCommands helper to support arbitrary delayed commands#23090alice-i-cecile merged 15 commits intobevyengine:mainfrom
DelayedCommands helper to support arbitrary delayed commands#23090Conversation
kfc35
left a comment
There was a problem hiding this comment.
The API is quite nice IMO. I don’t see any issues with the code. Pretty straightforward to understand as well.
I think maybe one thing you can add to your test is that you can still use the same original commands to spawn some things to take effect immediately after using commands.delayed() to spawn some delayed things, just to show that it’s can still be used after doing some delayed shenanigans.
I also am not too familiar with doing weird things with Drop, but I don’t think what you wrote is problematic.
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
| } | ||
| } | ||
|
|
||
| /// Returns a [`Commands`] that pushes commands to the provided queue instead of the one from `self`. |
There was a problem hiding this comment.
Fancy. This needs more docs! We need to cover:
- Why you might want to do this.
- What the semantic meaning of the returned
Commandsobject is. - The fact that you don't seem to need to do anything with the returned
Commandsobject: the magic seems to be done viaDrop.
There was a problem hiding this comment.
Just to clarify, there's only Drop magic on the DelayedCommands, the Commands returned by this rebind function should be used to write commands or else there's no point in calling it (the queue will remain empty)
crates/bevy_time/src/lib.rs
Outdated
| .in_set(TimeSystems) | ||
| .ambiguous_with(message_update_system), | ||
| ) | ||
| .add_systems(PreUpdate, delayed_queues_system) |
There was a problem hiding this comment.
This will only operate on Time<Virtual> because of the location of this system.
I'm fine with that limitation for an initial PR, but I'm also happy to help advise you on how to lift it (DelayedCommands + DelayedCommandQueue should have a generic, copy the strategy from Time).
If we don't lift that limitation in this PR, we should document it clearly.
There was a problem hiding this comment.
As we talked about on Discord there's a few complications here that make this quite tricky:
- DelayedCommands doesn't know what schedule it's running in or what the current default clock (
Time<()>) is set to. - We'd rather not have
commands.delayedtake a generic as it can't have a default and would hurt ergonomics. - There doesn't seem to be a reasonable way to inspect the current clock that accounts for custom clocks. (e.g. we could have an enum, but that doesn't account for custom clocks).
So unfortunately this looks like it'll have to be followup work. I added a note to document the limitation.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Incredibly slick: I really like this approach, and I think that the impact to users is high.
That said, I have a few cleanup items before we merge this:
- This code is generally very clever (mixed connotations!). We need more docs and comments throughout to make this extremely clear to future readers.
- There's a few code organization nits.
- Usage examples are mandatory here. Doc tests at least, but a tiny example in the
ecsfolder would be nice for discoverability. - I think this is important enough that it deserves a release note :)
|
@Runi-c and I tried to hash out designs for "make this generic over the clock used in Conversation Details
We should make |
| mut commands: Commands, | ||
| ) { | ||
| for (e, mut queue) in queues { | ||
| if queue.timer.tick(time.delta()).just_finished() { |
There was a problem hiding this comment.
Consider my comment on the Alice's original MR, #20155 (comment)
There was a problem hiding this comment.
Good callout, I switched it over.
I also considered @chescock's suggestion in that thread of using a priority queue index to check only the next queue that should be submitted per frame, but I'm unfamiliar with engine performance work. So long as there's no major performance issues, I'd prefer to save that kind of complexity for when we have a benchmark and real-world usage patterns to work from.
|
Damn, I really wish my phone dind't break a few days ago, causing me to miss this (apparently, Thunderbird does not deal with subdirectories used as inboxes well). Turns out I really ended up loving the callback-style API in my original snippet - not because its particularily more useful, but because of the very clear conceptual and visual distinction between delayed and non-delayed code. The simple act of having it in a block, to me, feels much less immediately confusing and prone to things like bad naming of the delayed commands than this style. Not that i dont see the advantages of this style as well. its far easier to delay a few commands interspersed in more complex non-delayed code - but thats also the downside imo. Ah well, its not a big enough deal to me to make me submit a PR to change it. |
|
@laundmo Darn, I was hoping to get your thoughts too! For what it's worth, I used your exact snippet in my submission to Bevy Jam 7, which ended up containing a significant number of delayed commands, and it worked great! The main thing that bothered me about the callback style was having to move things into it, meaning any data borrowed from components or resources in the system had to be pre-emptively cloned or copied into the callback to work correctly. By all means manageable, but it was a rough edge I really wanted to figure out how to get rid of by the end there just to cut down on ownership boilerplate. |
…evyengine#23090) # Objective - A generalized mechanism for "doing something later" is desirable for many games, especially when it comes to gameplay logic and VFX. - Fixes bevyengine#15129 - Closes bevyengine#20155 ## Solution - Build off the work in bevyengine#20155 (comment), especially @laundmo's comment. - Add a `DelayedCommands` helper obtainable via `commands.delayed()` that owns `CommandQueue`s and hands out new `Commands` bound to them. - When the `DelayedCommands` helper is dropped, push spawn commands onto the host `Commands` to spawn the queues as `DelayedCommandQueue` entities. - The entities are ticked by a new system added by `TimePlugin`. When the timer fires, the queue is submitted onto that system's `Commands`. ## Testing - Added a new test in `bevy_time` and it seems to work. - I'm not very familiar with doing hacky things like using `Drop` like this and would therefore appreciate careful review and guidance if changes are requested. --- ## Showcase ```rust fn my_cool_system(mut commands: Commands) { // fairly unobtrusive one-line delayed spawn commands.delayed().secs(0.1).spawn(DummyComponent); // the DelayedCommands can be stored to reuse more tersely let mut delayed = commands.delayed(); // allocation happens immediately so you can even queue // further operations on entities that aren't spawned yet let entity = delayed.secs(0.5).spawn_empty().id(); delayed.secs(0.7).entity(entity).insert(DummyComponent); // `delayed.secs` and `delayed.duration` both simply return a // `Commands` rebound to the stored `CommandQueue`, so you can additionally // just store that and reuse it to queue multiple commands with the same delay let mut in_1_sec = delayed.duration(Duration::from_secs_f32(1.0)); in_1_sec.spawn(DummyComponent); in_1_sec.spawn(DummyComponent); in_1_sec.spawn(DummyComponent); } ``` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
…evyengine#23090) # Objective - A generalized mechanism for "doing something later" is desirable for many games, especially when it comes to gameplay logic and VFX. - Fixes bevyengine#15129 - Closes bevyengine#20155 ## Solution - Build off the work in bevyengine#20155 (comment), especially @laundmo's comment. - Add a `DelayedCommands` helper obtainable via `commands.delayed()` that owns `CommandQueue`s and hands out new `Commands` bound to them. - When the `DelayedCommands` helper is dropped, push spawn commands onto the host `Commands` to spawn the queues as `DelayedCommandQueue` entities. - The entities are ticked by a new system added by `TimePlugin`. When the timer fires, the queue is submitted onto that system's `Commands`. ## Testing - Added a new test in `bevy_time` and it seems to work. - I'm not very familiar with doing hacky things like using `Drop` like this and would therefore appreciate careful review and guidance if changes are requested. --- ## Showcase ```rust fn my_cool_system(mut commands: Commands) { // fairly unobtrusive one-line delayed spawn commands.delayed().secs(0.1).spawn(DummyComponent); // the DelayedCommands can be stored to reuse more tersely let mut delayed = commands.delayed(); // allocation happens immediately so you can even queue // further operations on entities that aren't spawned yet let entity = delayed.secs(0.5).spawn_empty().id(); delayed.secs(0.7).entity(entity).insert(DummyComponent); // `delayed.secs` and `delayed.duration` both simply return a // `Commands` rebound to the stored `CommandQueue`, so you can additionally // just store that and reuse it to queue multiple commands with the same delay let mut in_1_sec = delayed.duration(Duration::from_secs_f32(1.0)); in_1_sec.spawn(DummyComponent); in_1_sec.spawn(DummyComponent); in_1_sec.spawn(DummyComponent); } ``` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Objective
TimedCommandsSystemParamfor easier delayed operations #15129DelayedCommandfor delayed ECS actions #20155Solution
DelayedCommandfor delayed ECS actions #20155 (comment), especially @laundmo's comment.DelayedCommandshelper obtainable viacommands.delayed()that ownsCommandQueues and hands out newCommandsbound to them.DelayedCommandshelper is dropped, push spawn commands onto the hostCommandsto spawn the queues asDelayedCommandQueueentities.TimePlugin. When the timer fires, the queue is submitted onto that system'sCommands.Testing
bevy_timeand it seems to work.Droplike this and would therefore appreciate careful review and guidance if changes are requested.Showcase