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

Ordered OnExit and OnEnter #13711

Closed
MiniaczQ opened this issue Jun 6, 2024 · 2 comments · Fixed by #13763
Closed

Ordered OnExit and OnEnter #13711

MiniaczQ opened this issue Jun 6, 2024 · 2 comments · Fixed by #13763
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior X-Contentious There are nontrivial implications that should be thought through

Comments

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jun 6, 2024

What problem does this solve or what need does it fill?

Currently OnExit and OnEnter aren't ordered in relation to the state dependency graph.
This is a problem when parent state transition prepares e.g. resources for substates, but the OnEnter(Substate) runs first.

What solution would you like?

  • Order OnExit to run from graph leaf-states to root-states.
  • Keep OnTransition in arbitrary order.
  • Order OnEnter to run from graph root-states to leaf-states.

This can either be done by adding after conditions during state installation or as part of StateRegistry, a separate feature.

What alternative(s) have you considered?

None that seem reasonable.

Additional context

This could be considered a bug to some degree.

Working example, entering Settings will crash or not, depending on the system order that was decided during startup.
@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 6, 2024
@alice-i-cecile alice-i-cecile 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 and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 6, 2024
@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jun 6, 2024 •
edited
Loading

Note that the fix with after will backfire on custom schedule runners, which
while still ordered AFTER their parent states, won't have the ordering to run BEFORE dependent states
(since those too use after and will only register them after built-in runners)

@lee-orr
Copy link
Contributor

lee-orr commented Jun 6, 2024

@MiniaczQ @alice-i-cecile - working on a potential solution

@lee-orr lee-orr mentioned this issue Jun 6, 2024
Closed
This was referenced Jun 8, 2024
Closed
Merged
github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
@MiniaczQ @lee-orr
Add more granular system sets for state transition schedule ordering (#…
6d0b750 
…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>
mockersf pushed a commit that referenced this issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
3 participants
@alice-i-cecile @lee-orr @MiniaczQ