feat(time): allow cancelling delayed commands#24179
Conversation
|
@Runi-c, I would value your review here. |
| delayed.secs(1.5).entity(entity).insert(DummyComponent); | ||
| } | ||
|
|
||
| fn cancellable_delayed_spawn(mut commands: Commands) { |
There was a problem hiding this comment.
This example is not realistic enough to teach the pattern properly: we want to gesture at some sort of "conditionally cancel" pattern.
| @@ -18,6 +18,17 @@ fn delayed_spawn_then_insert(mut commands: Commands) { | |||
| let entity = delayed.secs(0.5).spawn_empty().id(); | |||
| delayed.secs(1.5).entity(entity).insert(DummyComponent); | |||
| } | |||
There was a problem hiding this comment.
Note: I'd be happy to land this in 0.19, but won't block on it. You may need to rewrite or cut the release notes later if it misses the window.
We can add this in a later release candidate though: it's additive and minor.
| #[doc(hidden)] | ||
| pub use crate::{DelayedCommandsExt, Fixed, Real, Time, Timer, TimerMode, Virtual}; | ||
| pub use crate::{ | ||
| DelayedCommandHandle, DelayedCommandsExt, Fixed, Real, Time, Timer, TimerMode, Virtual, |
There was a problem hiding this comment.
DelayedCommandHandle is definitely not common enough that it should be in the prelude :)
| queue: CommandQueue, | ||
| } | ||
|
|
||
| /// A handle that can be used to cancel delayed commands. |
There was a problem hiding this comment.
I think we might want to steer clear of the term "handle" specifically here. It's common in async contexts, but it means something totally different in the context of Bevy because of asset handles. Is there a synonym we can use?
| /// Return a [`Commands`] whose commands will be delayed by `duration`, and | ||
| /// a handle that can be used to cancel them. | ||
| /// | ||
| /// Commands queued for the same `duration` share a queue and will return |
There was a problem hiding this comment.
This is extremely surprising, and IMO a bug. It would be good to avoid this, but if not, we should at least document how to avoid working around it (I think by generating two instances of DelayedCommands?).
| /// Commands queued for the same `duration` share a queue and will return | ||
| /// the same handle. | ||
| #[must_use = "The returned Commands must be used to submit commands with this delay."] | ||
| pub fn duration_with_handle( |
There was a problem hiding this comment.
This duplication is a code smell that will lead to a combinatorial explosion of methods. We should instead have one with_handle method or the like that gets chained on.
The pattern we want to copy is EntityCommands::id()
There was a problem hiding this comment.
See above for notes on naming. Handle is particularly confusing as a term here.
|
|
||
| /// A handle that can be used to cancel delayed commands. | ||
| /// | ||
| /// This handle represents a [`DelayedCommandQueue`] entity. Despawning the |
There was a problem hiding this comment.
These docs would benefit from some grounded advice on why you might want to cancel delayed commands in the context of game dev, and a dedicated doc test showing the end-to-end pattern.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Nice! I quite like this, but there are some things to clean up before we merge.
Easier than I expected overall; good work :)
| } | ||
|
|
||
| #[test] | ||
| fn delayed_queues_can_be_cancelled() { |
There was a problem hiding this comment.
It would be nice to test the idempotency of repeated cancellations, and the cancellation of commands that have already fired. Those should already pass, but they're tricky race condition bugs that we should protect against defensively.
| let handle = { | ||
| let mut delayed_cmds = commands.delayed(); | ||
| let (mut delayed, handle) = delayed_cmds.secs_with_handle(0.1); | ||
| delayed.spawn(DummyComponent); |
There was a problem hiding this comment.
Note that the entity for this spawn command is allocated immediately. If the delayed command is then canceled, the allocated entity is never spawned nor freed for reuse. This was what stopped me from adding cancelling to the original PR and I don't see a solution here.
There was a problem hiding this comment.
Could you track this in the handle, and then free it? You'd need a Vec, since there could be multiple entities queued up.
There was a problem hiding this comment.
This is quite complex: see #18670 and the code added there for more context on exactly how Bevy's entity lifecycle works :)
There was a problem hiding this comment.
The key API you'll need to clean up the memory leak of user entities is Entities::free_many :)
There was a problem hiding this comment.
Tracking it in the handle might be possible but there needs to be a proxy or facade for Commands::spawn and Commands::spawn_empty in order to "catch" allocations and somehow send/store them in the handle. As far as I can tell no other commands have this pre-allocation behavior (right now anyway! this would need to be carefully documented so future additions don't leak entities)
|
This is the Github profile of NewN on Discord, who was previously warned for submitting AI generated code in opposition to our AI Policy. They were banned from Discord due to hostility and pushiness when confronted on this. They have been blocked from this repo and reported to Github as a result. The discussion here is still useful here: I would welcome attempts to do it properly, but you will need to manually reconstruct this work from scratch to be compliant with our policy. |
Objective
Solution
DelayedCommandHandle, which wraps the entity tracking a delayed command queue and can cancel it viatry_despawn.duration_with_handleandsecs_with_handlewhile keeping the existingdurationandsecsAPIs unchanged.DelayedCommandQueueinto that entity when delayed commands are submitted.Testing
delayed_queues_can_be_cancelled, which queues a delayed spawn, cancels the handle, advances time, and confirms the spawn never runs.git diff --check.cargo test -p bevy_time delayed_queues_can_be_cancelledlocally because this environment does not havecargoorrustfmtinstalled.Showcase