Rename StateScoped to DespawnOnExitState and add DespawnOnEnterState

[mgi388]

Objective

• Alternative to and builds on top of Rename StateScoped to DespawnOnStateExit and add DespawnOnStateEnter #16284.
• Fixes Rename StateScoped to StateExitDespawn, add the inverse variant StateEntryDespawn #15849.

Solution

• Rename component StateScoped to DespawnOnExitState.
• Rename system clear_state_scoped_entities to despawn_entities_on_exit_state.
• Add DespawnOnEnterState and despawn_entities_on_enter_state which is the OnEnter equivalent.

Note

Compared to #16284, the main change is that I did the rename in such a way as to keep the terms OnExit and OnEnter together. In my own game, I was adding VisibleOnEnterState and HiddenOnExitState and when naming those, I kept the OnExit and OnEnter together. When I checked #16284 it stood out to me that the naming was a bit awkward. Putting the State in the middle and breaking up OnEnter and OnExit also breaks searching for those terms.

Open questions

1. Should we split enable_state_scoped_entities into two functions, one for the OnEnter and one for the OnExit? I personally have zero need thus far for the OnEnter version, so I'd be interested in not having this enabled unless I ask for it.
2. If yes to 1., should we follow my lead in my Visibility state components (see below) and name these app.enable_despawn_entities_on_enter_state() and app.enable_despawn_entities_on_exit_state(), which IMO says what it does on the tin?

Testing

Ran all changed examples.

Side note: VisibleOnEnterState and HiddenOnExitState

For reference to anyone else and to help with the open questions, I'm including the code I wrote for controlling entity visibility when a state is entered/exited.

visibility.rs

use bevy_app::prelude::*;
use bevy_ecs::prelude::*;
use bevy_reflect::prelude::*;
use bevy_render::prelude::*;
use bevy_state::{prelude::*, state::StateTransitionSteps};
use tracing::*;

pub trait AppExtStates {
    fn enable_visible_entities_on_enter_state<S: States>(&mut self) -> &mut Self;

    fn enable_hidden_entities_on_exit_state<S: States>(&mut self) -> &mut Self;
}

impl AppExtStates for App {
    fn enable_visible_entities_on_enter_state<S: States>(&mut self) -> &mut Self {
        self.main_mut()
            .enable_visible_entities_on_enter_state::<S>();
        self
    }

    fn enable_hidden_entities_on_exit_state<S: States>(&mut self) -> &mut Self {
        self.main_mut().enable_hidden_entities_on_exit_state::<S>();
        self
    }
}

impl AppExtStates for SubApp {
    fn enable_visible_entities_on_enter_state<S: States>(&mut self) -> &mut Self {
        if !self
            .world()
            .contains_resource::<Events<StateTransitionEvent<S>>>()
        {
            let name = core::any::type_name::<S>();
            warn!("Visible entities on enter state are enabled for state `{}`, but the state isn't installed in the app!", name);
        }
        // We work with [`StateTransition`] in set
        // [`StateTransitionSteps::ExitSchedules`] as opposed to [`OnExit`],
        // because [`OnExit`] only runs for one specific variant of the state.
        self.add_systems(
            StateTransition,
            update_to_visible_on_enter_state::<S>.in_set(StateTransitionSteps::ExitSchedules),
        )
    }

    fn enable_hidden_entities_on_exit_state<S: States>(&mut self) -> &mut Self {
        if !self
            .world()
            .contains_resource::<Events<StateTransitionEvent<S>>>()
        {
            let name = core::any::type_name::<S>();
            warn!("Hidden entities on exit state are enabled for state `{}`, but the state isn't installed in the app!", name);
        }
        // We work with [`StateTransition`] in set
        // [`StateTransitionSteps::ExitSchedules`] as opposed to [`OnExit`],
        // because [`OnExit`] only runs for one specific variant of the state.
        self.add_systems(
            StateTransition,
            update_to_hidden_on_exit_state::<S>.in_set(StateTransitionSteps::ExitSchedules),
        )
    }
}

#[derive(Clone, Component, Debug, Reflect)]
#[reflect(Component, Debug)]
pub struct VisibleOnEnterState<S: States>(pub S);

#[derive(Clone, Component, Debug, Reflect)]
#[reflect(Component, Debug)]
pub struct HiddenOnExitState<S: States>(pub S);

/// Makes entities marked with [`VisibleOnEnterState<S>`] visible when the state
/// `S` is entered.
pub fn update_to_visible_on_enter_state<S: States>(
    mut transitions: EventReader<StateTransitionEvent<S>>,
    mut query: Query<(&VisibleOnEnterState<S>, &mut Visibility)>,
) {
    // We use the latest event, because state machine internals generate at most
    // 1 transition event (per type) each frame. No event means no change
    // happened and we skip iterating all entities.
    let Some(transition) = transitions.read().last() else {
        return;
    };
    if transition.entered == transition.exited {
        return;
    }
    let Some(entered) = &transition.entered else {
        return;
    };
    for (binding, mut visibility) in query.iter_mut() {
        if binding.0 == *entered {
            visibility.set_if_neq(Visibility::Visible);
        }
    }
}

/// Makes entities marked with [`HiddenOnExitState<S>`] invisible when the state
/// `S` is exited.
pub fn update_to_hidden_on_exit_state<S: States>(
    mut transitions: EventReader<StateTransitionEvent<S>>,
    mut query: Query<(&HiddenOnExitState<S>, &mut Visibility)>,
) {
    // We use the latest event, because state machine internals generate at most
    // 1 transition event (per type) each frame. No event means no change
    // happened and we skip iterating all entities.
    let Some(transition) = transitions.read().last() else {
        return;
    };
    if transition.entered == transition.exited {
        return;
    }
    let Some(exited) = &transition.exited else {
        return;
    };
    for (binding, mut visibility) in query.iter_mut() {
        if binding.0 == *exited {
            visibility.set_if_neq(Visibility::Hidden);
        }
    }
}

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[benfrankel]

Leaving a few minor comments on the documentation, but the code looks good and I like the rename.

[mgi388]

@benfrankel thanks for the review ❤️. I'll apply the suggestions shortly (no rush due to 0.16 shenanigans).

In the meantime, if you have any thoughts on the open questions in the PR description, please feel free to add your opinion

[benfrankel]

I think it would be best to keep it to a single enable_ method for simplicity, since the performance cost per system is likely negligible. If it is to be split up though, I think the proposed naming sounds good.

Another naming option is apply_despawn_on_enter_state for the system instead of despawn_entities_on_enter_state, and enable_despawn_on_enter_state for the method instead of enable_despawn_entities_on_enter_state. But I think it's fine either way.

[Henauxg]

Although I'm also not interested in DespawnOnEnterState either, I think the performance cost of enabling both with 1 call is likely negligible as it is only scheduled to run once during transitions, whereas the verbosity of splitting the enabling calls seems cumbersome. It will still be possible to refine it later once more state-scope mechanisms are introduced.
Regarding the naming question for the common enabling call, it seems from the discussions in #16284 that "state scope" could still be used to name the feature (bikeshed: could also introduce something like "Entities Lifetime" which could potentially extend to more usages than just state scopes).

Regarding the internal systems name, I prefer despawn_entities_on_enter_state rather than apply_despawn_on_enter_state.

(As a more general note, I feel like it might have been easier to split the work as a straightforward renaming PR from StateScoped to DespawnOnExitState and then have a PR to propose an extension of state scoping mechanisms)

[mgi388]

(As a more general note, I feel like it might have been easier to split the work as a straightforward renaming PR from StateScoped to DespawnOnExitState and then have a PR to propose an extension of state scoping mechanisms)

@Henauxg in case you weren't aware, I adopted this PR and all the work and review was already done, so it probably would have been more effort to start from scratch, even if you're right.

[mgi388]

@benfrankel @Henauxg how about this rename for the enable_ funcs.

First, as a reminder, the goal of the rename (more than just pointless rename churn) is that enable_state_scoped_entities:

1. Is vague. In the context of adding the visibility-based one, it reads like a superset of both, and doesn't really say anything about how it only controls the despawning of entities.
2. Makes less sense once the original StateScoped component no longer exists. Users will see a component exists named DespawnOnExitState and it won't be obvious that to enable it you need to look for a method named enable_state_scoped_entities.
3. The entities part being plural is, arguably, awkward and unnecessary.

Proposal:

app.enable_state_scoped_entity_despawning::<GameState>();
app.enable_state_scoped_entity_visibility::<GameState>();

// The visibility one just to show that it _could_ exist, even in userland, 
// and also to show how the naming would sit with the despawning version

I went back and forth a bit coming up with this. Rationale/thinking:

1. enable_ - shows the user is turning on a feature.
2. state_scoped_ - shows what we're really enabling is state-based control over entities.
3. entity_ - clearly shows we're dealing with entities. Without this, it's not obvious that this is for entities.
4. entity_ not entities_—the plural is not necessary and just makes the whole thing harder to parse.
5. despawning - tells users that this is specifically about despawning. I thought about lifecycle here, but it's a bit misleading. This feature can only despawn. I also went with despawning not despawn because that feels like it matches better with visibility but I could be convinced that despawn is OK.
6. visibility - tells users that this is specifically about visibility. Note: Compared to my OP, I'd imagine that the user calls this function and underneath, it enables systems for all of On/Enter/Visibible/Hidden combos.

If we wanted, we could also use the term bound instead of scoped, as follows. I'm probably impartial to bound vs scoped even if "scoped" has always stuck out as an awkward term personally.

app.enable_state_bound_entity_despawning::<GameState>();
app.enable_state_bound_entity_visibility::<GameState>();

WDYT?

[Henauxg]

I thought that I would prefer scope to bound as scope is often used in this same context in CS: scope of a variable, function, ... But after giving it a go, with the addition of the OnEnter despawning, this really goes against what we assume when we talk about the "scope" of some object. You usually disappear when going "out of scope", not when coming into it.

Here's my attempt at a more accurate name:

app.enable_entities_despawning_on_state_transitions::<GameState>();

The plural does not seem like a bad thing to me. It manages entities, not an entity. We have plural on App methods such as add_plugins, add_systems, but I'm not bent on it.
I'd like something shorter using Schedule labels directly to convey the information such as an hypothetical app.enable_entities_despawning(OnExit(GameState)); but I'm sure it would be much messier to implement.

EDIT: the use of enable was fine when we didn't have a Disabled component. Now that we have it, I think this is a bit confusing (especially if we introduce a new state scoped enable/disable mechanic: enabled_state_enabled_entities...). Could be replaced with something similar: activate_entities_despawning_on..., schedule_entities_despawning_on..., ...

[alice-i-cecile]

I'm pretty sure I have opinions here on what an ideal naming / setup looks like, but I can tackle that during final review. Other maintainers: please let me handle this one?

[benfrankel]

FWIW in my own crate pyri_state I call this feature react, or "state reaction components". So another option for the bikeshed is app.enable_state_reaction_components.

[NthTensor]

Migration guide looks good.

[alice-i-cecile]

I've come around and quite like the current naming. Let's merge this, and then make an entity-disabling form in follow-up.