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.

Sign up for GitHub

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

Jump to bottom

Add OnTransition schedule that is ran between OnExit and OnEnter #7936

Merged
merged 6 commits into from
Mar 10, 2023
Merged

Add OnTransition schedule that is ran between OnExit and OnEnter #7936

merged 6 commits into from
Mar 10, 2023

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Mar 7, 2023 •
edited
Loading

Objective

Fix #7647

Solution

Implements the solution described by @LiamGallagher737 in #7647.

Changelog

Added

  • OnTransition: Alternative to OnEnter and OnExit to enable more specific state transition logic.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Example alien_cake_addict failed to run, please try running it locally and check the result.

alice-i-cecile
alice-i-cecile requested changes Mar 7, 2023
View reviewed changes
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.

The schedule needs to be initialized via App::add_state.

Cool idea though, I like it!

Strong preference for named fields (from and to perhaps?) for the OnTransition type though.

@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 7, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 7, 2023
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Mar 7, 2023 •
edited
Loading

The schedule needs to be initialized via App::add_state.

Strong preference for named fields (from and to perhaps?) for the OnTransition type though.

Done 👍

Although since there isn't a way to lazily initialize a Schedule (AFAIK), this might incur a good amount of overhead for States that have a lot of variants. Might be worth a benchmark.

james7132
james7132 requested changes Mar 7, 2023
View reviewed changes
crates/bevy_app/src/app.rs Outdated
to: second,
},
Schedule::new(),
);
Copy link
Member

@james7132 james7132 Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This eagerly registers schedules that scales quadratically with the number of variants in a state set. Not all of them may be necessary, and will still use memory. Is there a way to do this lazily?

Copy link
Member

Choose a reason for hiding this comment

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

If we merge #7911 first we can actually remove all of the schedule registration logic from this method. Which I think I prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer this as well. We wait for #7911 then.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Mar 8, 2023
@james7132 james7132 removed the S-Blocked This cannot move forward until something else changes label Mar 9, 2023
@james7132
Copy link
Member

#7911 has been merged.

@ItsDoot ItsDoot force-pushed the feature/state-on-transition branch from ab109d1 to ae7383e Compare March 10, 2023 07:23
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Mar 10, 2023

Updated. Since schedules can be lazily initialized now, there shouldn't be a need to register OnTransition now right?

@ItsDoot ItsDoot requested a review from james7132 March 10, 2023 07:25
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@ItsDoot ItsDoot requested review from alice-i-cecile and james7132 and removed request for james7132 and alice-i-cecile March 10, 2023 07:36
james7132
james7132 approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Please rerun cargo fmt --all to fix CI

crates/bevy_ecs/src/schedule/state.rs
from: exited,
to: entered.clone(),
};
if world.resource::<Schedules>().contains(&transition_schedule) {
Copy link
Member

Choose a reason for hiding this comment

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

We should have a try_run_schedule. But that can be handled in a different PR.

@james7132 james7132 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 10, 2023
@james7132 james7132 enabled auto-merge March 10, 2023 08:57
@james7132 james7132 added this pull request to the merge queue Mar 10, 2023
@ItsDoot ItsDoot mentioned this pull request Mar 10, 2023
Closed
Merged via the queue into bevyengine:main with commit 8aa217c Mar 10, 2023
@ItsDoot ItsDoot deleted the feature/state-on-transition branch March 10, 2023 18:16
ItsDoot added a commit to ItsDoot/bevy that referenced this pull request Mar 10, 2023
@ItsDoot
Add OnTransition schedule that is ran between OnExit and OnEnter (bev…
55bb0df 
…yengine#7936)
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@ItsDoot @Shfty
Add OnTransition schedule that is ran between OnExit and OnEnter (bev…
4df64fa 
…yengine#7936)
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@ItsDoot @Shfty
Add OnTransition schedule that is ran between OnExit and OnEnter (bev…
868e8b4 
…yengine#7936)
This was referenced Jun 28, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
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
Milestone
0.11
Development

Successfully merging this pull request may close these issues.

Run conditions for specific state transitions
3 participants
@ItsDoot @james7132 @alice-i-cecile