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

Make initial StateTransition run before PreStartup #14208

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Jul 7, 2024

Objective

Solution

  • Run initial StateTransition as a startup schedule before PreStartup, instead of running it inside Startup as an exclusive system.

Related discord discussion:
https://discord.com/channels/691052431525675048/692572690833473578/1259543775668207678

Testing

Reproduction now works correctly:

use bevy::prelude::*;

#[derive(Debug, Clone, Copy, Default, Eq, PartialEq, Hash, States)]
enum AppState {
    #[default]
    Menu,
    InGame,
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .init_state::<AppState>()
        .add_systems(Startup, setup)
        .add_systems(OnEnter(AppState::Menu), enter_menu_state)
        .run();
}

fn setup(mut next_state: ResMut<NextState<AppState>>) {
    next_state.set(AppState::Menu);
}

fn enter_menu_state() {
    println!("Entered menu state");
}

image


Changelog

  • Initial StateTransition runs before PreStartup instead of inside Startup.

@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 S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 7, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14.1 milestone Jul 7, 2024
@CooCooCaCha
Copy link

I just tested this and it looks like it works 👍🏻

@benfrankel
Copy link
Contributor

I haven't looked at the code, but: IMO this approach is fine for 0.14.1, but a state disabling / enabling approach would be preferable for 0.15.

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Jul 8, 2024

I've thought about it, a little bit

If we do introduce NextState::Disable, running the initial transition as the first frame transition would be much simpler, because when initializing states we simply set the initial NextState.
Right now we need to do some events magic which isn't ideal, but NextState doesn't work with optionals yet.

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.

Running a schedule within another (commonly-used) bevy-provided schedule, without even exposing a system set or the system itself to order around, is problematic -- this is a clear improvement over the current behavior, and makes the code simpler.

No reason not to merge afaic. Better solution can be implemented later.

world: &mut World,
startup_label: Option<InternedScheduleLabel>,
) {
pub fn setup_state_transitions_in_world(world: &mut World) {
Copy link
Member

Choose a reason for hiding this comment

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

This function signature change is a semver violation by Rust's rules.

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 agree with this design, and I think that the technical semver violation isn't important enough to worry about. I would be very surprised to see users calling that method themselves, even though it is pub.

@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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 15, 2024
Merged via the queue into bevyengine:main with commit bc72ced Jul 15, 2024
28 checks passed
mockersf pushed a commit that referenced this pull request Aug 2, 2024
# Objective

- Fixes #14206 

## Solution

- Run initial `StateTransition` as a startup schedule before
`PreStartup`, instead of running it inside `Startup` as an exclusive
system.

Related discord discussion:

https://discord.com/channels/691052431525675048/692572690833473578/1259543775668207678

## Testing

Reproduction now works correctly:

```rs
use bevy::prelude::*;

#[derive(Debug, Clone, Copy, Default, Eq, PartialEq, Hash, States)]
enum AppState {
    #[default]
    Menu,
    InGame,
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .init_state::<AppState>()
        .add_systems(Startup, setup)
        .add_systems(OnEnter(AppState::Menu), enter_menu_state)
        .run();
}

fn setup(mut next_state: ResMut<NextState<AppState>>) {
    next_state.set(AppState::Menu);
}

fn enter_menu_state() {
    println!("Entered menu state");
}
```


![image](https://github.com/bevyengine/bevy/assets/13040204/96d7a533-c439-4c0b-8f15-49f620903ce1)


---

## Changelog

- Initial `StateTransition` runs before `PreStartup` instead of inside
`Startup`.
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-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
Development

Successfully merging this pull request may close these issues.

OnEnter schedule is not ran when calling next_state.set() in Startup system
4 participants