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 World::try_run_schedule #8028

Merged
merged 10 commits into from
Mar 17, 2023

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Mar 10, 2023

Objective

Fix #8018

Solution

Added a new method World::try_run_schedule that returns a Result instead of panicking if the schedule doesn't exist.


Changelog

Added

  • World::try_run_schedule: Attempt to run a schedule and return a Result rather than panicking if the schedule doesn't exist.

@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 labels Mar 10, 2023
@alice-i-cecile
Copy link
Member

The diff / commit history looks wrong, so this is quite hard to review. Are you able to clean that up?

@ItsDoot ItsDoot marked this pull request as draft March 10, 2023 20:03
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Mar 10, 2023

Yea sorry about that @alice-i-cecile, messed up my history trying to rebase, fixing it now. 😅

@ItsDoot ItsDoot marked this pull request as ready for review March 10, 2023 20:12
@alice-i-cecile
Copy link
Member

Code itself LGTM. Can we use this internally anywhere? I know that state transitions wanted this: do enter / exit want it too already?

@ItsDoot
Copy link
Contributor Author

ItsDoot commented Mar 10, 2023

We could certainly move towards full lazy initialization for State schedules by removing these:

for variant in S::variants() {
if self.get_schedule(OnEnter(variant.clone())).is_none() {
self.add_schedule(OnEnter(variant.clone()), Schedule::new());
}
if self.get_schedule(OnExit(variant.clone())).is_none() {
self.add_schedule(OnExit(variant), Schedule::new());
}
}

And using try_run_schedule in these places:

pub fn run_enter_schedule<S: States>(world: &mut World) {
world.run_schedule(OnEnter(world.resource::<State<S>>().0.clone()));
}

world.run_schedule(OnExit(exited.clone()));
// If the schedule doesn't exist, we don't care.
let _ = world.try_run_schedule(OnTransition {
from: exited,
to: entered.clone(),
});
world.run_schedule(OnEnter(entered));

@james7132 @alice-i-cecile Thoughts?

@alice-i-cecile
Copy link
Member

I like that much better!

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/state.rs Outdated Show resolved Hide resolved
Co-authored-by: James Liu <contact@jamessliu.com>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2023
@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 Mar 17, 2023
Merged via the queue into bevyengine:main with commit 609b099 Mar 17, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
Co-authored-by: James Liu <contact@jamessliu.com>
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
Co-authored-by: James Liu <contact@jamessliu.com>
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 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.

Panic-less version of World::run_schedule
3 participants