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 more granular system sets for state transition schedule ordering #13763

Merged
merged 8 commits into from
Jun 10, 2024
Merged

Add more granular system sets for state transition schedule ordering #13763

merged 8 commits into from
Jun 10, 2024

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Jun 9, 2024 •
edited
Loading

Objective

Fixes #13711

Solution

Introduce smaller, generic system sets for each schedule variant, which are ordered against other generic variants:

  • ExitSchedules<S> - For OnExit schedules, runs from leaf states to root states.
  • TransitionSchedules<S> - For OnTransition schedules, runs in arbitrary order.
  • EnterSchedules<S> - For OnEnter schedules, runs from root states to leaf states.

Also unified ApplyStateTransition<S> schedule which works in basically the same way, just for internals.

Testing

  • One test that tests schedule execution order

@MiniaczQ MiniaczQ mentioned this pull request Jun 9, 2024
Closed
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 9, 2024
@NthTensor NthTensor added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 9, 2024
@alice-i-cecile
Copy link
Member

We should collaborate on either this or #13724: they seem very similar :)

@alice-i-cecile alice-i-cecile mentioned this pull request Jun 9, 2024
Closed
lee-orr
lee-orr approved these changes Jun 9, 2024
View reviewed changes
Copy link
Contributor

@lee-orr lee-orr left a comment

Choose a reason for hiding this comment

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

Overall the PR is good - the main issue was in the test, so I'd recommend applying some changes there (I provided suggestions) - but once they're in I'll be happy with this.

crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/mod.rs Outdated Show resolved Hide resolved
MiniaczQ and others added 3 commits June 9, 2024 21:39
@MiniaczQ @lee-orr
Apply suggestions from code review
8a93662 
Co-authored-by: Lee-Orr <lee-orr@users.noreply.github.com>
@MiniaczQ
fixes
208421a
@MiniaczQ
fix clippy
34a8f70
benfrankel
benfrankel reviewed Jun 9, 2024
View reviewed changes
Copy link
Contributor

@benfrankel benfrankel left a comment

Choose a reason for hiding this comment

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

Some documentation nits & a question.

crates/bevy_state/src/state/transitions.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/transitions.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/transitions.rs Outdated Show resolved Hide resolved
crates/bevy_state/src/state/freely_mutable_state.rs Show resolved Hide resolved
@benfrankel
Copy link
Contributor

Replaced StateTransitionSteps in favor of ExitSchedules, TransitionSchedules, EnterSchedules.

This in the changelog seems not entirely accurate. StateTransitionSteps is still around, and the new system sets provide internal ordering.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 9, 2024
@alice-i-cecile alice-i-cecile 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 9, 2024
alice-i-cecile
alice-i-cecile approved these changes Jun 9, 2024
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.

Good tests, reasonable implementation. If you can get CI green I'll merge this tomorrow during the merge train.

benfrankel
benfrankel approved these changes Jun 9, 2024
View reviewed changes
Copy link
Contributor

@benfrankel benfrankel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I haven't reviewed the added test, but the implementation looks good.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 10, 2024
Merged via the queue into bevyengine:main with commit 6d0b750 Jun 10, 2024
27 checks passed
mockersf pushed a commit that referenced this pull request Jun 10, 2024
@MiniaczQ @lee-orr @mockersf
Add more granular system sets for state transition schedule ordering (#…
854983d 
…13763)

# Objective

Fixes #13711 

## Solution

Introduce smaller, generic system sets for each schedule variant, which
are ordered against other generic variants:
- `ExitSchedules<S>` - For `OnExit` schedules, runs from leaf states to
root states.
- `TransitionSchedules<S>` - For `OnTransition` schedules, runs in
arbitrary order.
- `EnterSchedules<S>` - For `OnEnter` schedules, runs from root states
to leaf states.

Also unified `ApplyStateTransition<S>` schedule which works in basically
the same way, just for internals.

## Testing

- One test that tests schedule execution order

---------

Co-authored-by: Lee-Orr <lee-orr@users.noreply.github.com>
@MiniaczQ MiniaczQ deleted the state-schedule-ordering branch September 7, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benfrankel benfrankel benfrankel approved these changes

@lee-orr lee-orr lee-orr approved these changes

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

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
0.14
Development

Successfully merging this pull request may close these issues.

Ordered OnExit and OnEnter
5 participants
@MiniaczQ @alice-i-cecile @benfrankel @lee-orr @NthTensor