Skip to content
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

Add run conditions for executing a system after a delay #11095

Merged
merged 7 commits into from
Jan 9, 2024

Conversation

TimJentzsch
Copy link
Contributor

@TimJentzsch TimJentzsch commented Dec 26, 2023

Objective

I want to run a system once after a given delay.

  • First, I tried using the on_timer run condition, but it uses a repeating timer, causing the system to run multiple times.
  • Next, I tried combining the on_timer with the run_once run condition. However, this causes the timer to tick only once, so the system is never executed.

Solution

  • Replace on_timer by on_time_interval and on_real_timer by on_real_time_interval to clarify the meaning (the old ones are deprecated to avoid a breaking change). (Reverted according to feedback)
  • Add once_after_delay and once_after_real_delay to run the system exactly once after the delay, using TimerMode::Once.
  • Add repeating_after_delay and repeating_after_real_delay to run the system indefinitely after the delay, using Timer::finished instead of Timer::just_finished.

Changelog

Added

  • once_after_delay and once_after_real_delay run conditions to run the system exactly once after the delay, using TimerMode::Once.
  • repeating_after_delay and repeating_after_real_delay run conditions to run the system indefinitely after the delay, using Timer::finished instead of Timer::just_finished.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Time Involves time keeping and reporting labels Dec 26, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this change (it's flexible and intuitively matches the name), but I think it's a usability regression in most cases.

Instead, I suggest we keep this change, but also:

  1. Add a periodically (or whatever) run condition that works the same as the old on_timer condition.
  2. Doc link between the two methods, explaining why you might want the more verbose form.
  3. Replicate this for the on_real_timer variant.

@TimJentzsch
Copy link
Contributor Author

TimJentzsch commented Dec 26, 2023

I really like this change (it's flexible and intuitively matches the name), but I think it's a usability regression in most cases.

Instead, I suggest we keep this change, but also:

1. Add a `periodically` (or whatever) run condition that works the same as the old on_timer condition.

2. Doc link between the two methods, explaining why you might want the more verbose form.

3. Replicate this for the `on_real_timer` variant.

At this point I wonder if it's worth it to have the more verbose version, or if we have periodically and after_delay (or something similar) which both just take a Duration and handle the TimerMode internally.

Or maybe on_timer can stay as it is and after_delay is just an extra run condition, then the change isn't breaking anymore.

@alice-i-cecile
Copy link
Member

Yeah, it might make sense just to have the two forms and don't ever expose the Timer form. At the same time, I think we should move away from on_timer as a name (use a deprecation if you do), just to avoid the same confusion you had. It's weird that it doesn't take a Timer argument!

@TimJentzsch
Copy link
Contributor Author

Alright, I have deprecated the on_timer and on_real_timer names and added variants with TimerMode::Once.

@TimJentzsch TimJentzsch changed the title Take Timer instead of Duration in on_timer run condition Add run conditions for executing a system after a delay Dec 28, 2023
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Nice docs.

@TimJentzsch TimJentzsch added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 7, 2024
@TimJentzsch
Copy link
Contributor Author

As per https://discordapp.com/channels/691052431525675048/692572690833473578/1193685432442036235, I reverted the rename of the old run condition.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 8, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 8, 2024
Merged via the queue into bevyengine:main with commit cf3105a Jan 9, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Time Involves time keeping and reporting C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants