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

Do not add OnUpdate system set to CoreSet::ApplyStateTransitions #7634

Closed
wants to merge 1 commit into from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Feb 11, 2023

Objective

The inclusion of timing information in on_update is surprising to users, and leads to fairly tricky to debug errors whenever they attempt to schedule it.

Fixes #7632.

Solution

The OnUpdate system set is no longer added to CoreSet::ApplyStateTransitions. This lets users change the base set, reduces the number of concerns and allows the CoreSet::Update base set get applied by default. This is closest to 0.9 behavior.

This can result in a set which can span multiple base sets, which can be tricky to visualize.

Alternatives

  1. Let this error continue to bite users, but document what's going wrong better.
  2. Removed the OnUpdate set (which stuck the system in StateTransitions, in favor of simply having system.on_update(state) as sugar for run_if(state_equals(state)).
  3. Remove the on_update sugar completely and let people use run conditions themselves.
  4. Remove the OnUpdate set, and add an OnUpdate schedule, which is run via a system which is by default run immediately after apply_state_transition.

2 doesn't work well, since I can't easily implement .run_if for SystemConfigs, as it seems to require cloning Conditions.

Changelog

The OnUpdate set is no longer configured to run in CoreSet::ApplyStateTransitions.

Migration Guide

If you needed the timing information imposed by OnUpdate, also add in_base_set(CoreSet::StateTransitions)).

@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 X-Controversial There is active debate or serious implications around merging this PR labels Feb 11, 2023
@alice-i-cecile
Copy link
Member Author

Also added a missing run_if method to SystemConfigs.

I remember running into this in the initial migration, and I don't see a good way around it. To me, that suggests that alternative 2 is probably the right choice?

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 11, 2023
@alice-i-cecile alice-i-cecile changed the title Remove the OnUpdate system set Do not add OnUpdate system set to CoreSet::ApplyStateTransitions Feb 11, 2023
@alice-i-cecile alice-i-cecile marked this pull request as ready for review February 11, 2023 22:56
@maniwani
Copy link
Contributor

maniwani commented Feb 11, 2023

For (2), SystemConfigs won't support run_if without an additional Clone bound, but systems.on_update(X) doesn't have to literally be an alias for systems.run_if(state_equals(X)). States: Clone, so you can do something like:

impl IntoSystemConfigs for SystemConfigs {
    fn on_update<S: States>(self, state: S) -> Self {
        for system in &mut self.systems {
            system.conditions.push(new_condition(state_equals(state.clone()));
        }

        self
    }
}

I argued for (3) when we were trying to get the RFC merged. Users can't get OnUpdate and state_equals mixed up if one doesn't exist. 🙂 But turning OnUpdate into a schedule (4) is also valid. It'll probably add to the confusion about its difference from state_equals, but it would match enter/exit at least.

Also, that after(apply_state_transition::<S>) can easily turn into a SystemTypeSetAmbiguity error if a user just adds another instance of apply_state_transition::<S>. If we're keeping that, we should add it to a set (probably another base set in CoreSet) and order on that.

@cart
Copy link
Member

cart commented Feb 13, 2023

I was also planning on campaigning against on_update as that felt too "special cased". It wasn't in the RFC at all so I was surprised it was added. I just didn't want to start that campaign in the migration pr. Imo people should think about the OnUpdate set pretty much exactly the same way they think about CoreSet::Update (the name overlap is intentional from my perspective). Trying to add things to that set and the PreUpdate set should be invalid. Abstracting it out with the on_update builder makes it harder to think about it in terms of set compatibility / position in the schedule.

I'm strongly against removing a strict ordering from OnUpdate that makes it behave like CoreSet::Update. It should work "as expected" (as in ... like an Update system, designed under the assumptions that it will run after necessary engine feature prep and before engine "responses" to user code) without requiring users to supply their own orderings. It exists only for this purpose. If we remove this, we should should remove the abstraction entirely.

We already agreed that run conditions were the way to support "non update" scenarios like "adding state_equals(X) systems to PreUpdate". I think that design is the right approach. Compromising the OnUpdate abstraction because adding run conditions to SystemConfigs isn't currently supported seems wrong. Even the current situation where repeating the condition per-system (or using an the organizational set) is preferable. In these cases, organizations sets are probably the right move in most cases anyway?

It seems like we could support run_if() on SystemConfigs by just adding a clone bound / cloning it into all of the systems? We could also consider adding "implicit hidden organizational sets" for sharing run conditions defined on many systems in a SystemConfigs (but that feels a bit messy).

In short I think we should:

  1. Remove the on_update builder. Encourage people to use in_set(OnUpdate(State::X)) normally and run_if(state_equals(State::X)) in non-update situations.
  2. Try to improve the "run_if on SystemConfigs" situation.

@alice-i-cecile
Copy link
Member Author

Awesome, I'm quite happy with that conclusion.

@maniwani
Copy link
Contributor

maniwani commented Feb 13, 2023

Trying to add things to that set and the PreUpdate set should be invalid. Abstracting it out with the on_update builder makes it harder to think about it in terms of set compatibility / position in the schedule.

Compromising the OnUpdate abstraction because adding run conditions to SystemConfigs isn't currently supported seems wrong.

#7632 shows a user expecting on_update(X) to be sugar for run_if(state_equals(X)) and being surprised that isn't the case. There was no specific request to change the properties of the OnUpdate system set.

What is clear is that the 0.10 blog post / book / API docs needs to specifically spell out how run_if(state_equals(X)) and in_set(OnUpdate(X)) are different. The former is simply "if state is active" while the latter is a location in the base schedule that is tied to the Update base set and also has that condition attached.


Also, I still think that run_if(state_equals(X)) is the "normal" one / what most people will end up using. I just don't want users thinking it's "wrong" to attempt state transitions or put "if state is active" logic in other places. Our defaults may be internally consistent, but they're still just fallbacks / assumptions.

@cart
Copy link
Member

cart commented Feb 13, 2023

Given that we have "default base sets that default to Update", I think theres a world where we can reasonably remove OnUpdate in favor of run_if everywhere, but for me that hinges on a number of things:

  1. Right now the only way to avoid re-running run conditions for every system is to use a set and add the condition there. If we aren't going to provide a set like OnUpdate (which runs the condition exactly once at the appropriate point in the schedule), then we need to make a app.add_systems((foo, bar).run_if(state_equals(SomeState::X)) equivalent that only runs the criteria once (ex: the "anonymous organizational sets" i outlined above)
  2. . It would be nice if it was a bit less of a mouth full to type + read. If we implement Critera (via IntoSystem) for all states, then we could do app.add_systems((foo, bar).run_if(SomeState::X). Alternatively, I think .run_if(in_state(SomeState::X)) reads a bit better.

Also not something I'd want to do for 0.10, given the time constraints. Removing OnUpdate is something we should consider later.

@maniwani
Copy link
Contributor

Sure, I'm not saying we need to get rid of OnUpdate, just that we need to explain how it's different (and that it'll be common for users to put state logic in places other than ApplyStateTransitions and OnUpdate).

I do like in_state more than state_equals.

@alice-i-cecile
Copy link
Member Author

Closed in favor of #7667, as discussed above.

bors bot pushed a commit that referenced this pull request Feb 14, 2023
# Objective

Fixes #7632.

As discussed in #7634, it can be quite challenging for users to intuit the mental model of how states now work.

## Solution

Rather than change the behavior of the `OnUpdate` system set, instead work on making sure it's easy to understand what's going on.

Two things have been done:

1. Remove the `.on_update` method from our bevy of system building traits. This was special-cased and made states feel much more magical than they need to.
2. Improve the docs for the `OnUpdate` system set.
bors bot pushed a commit that referenced this pull request Feb 14, 2023
# Objective

- Improve readability of the run condition for systems only running in a certain state

## Solution

- Rename `state_equals` to `in_state` (see [comment by cart](#7634 (comment)) in #7634 )
- `.run_if(state_equals(variant))` now is `.run_if(in_state(variant))`

This breaks the naming pattern a bit with the related conditions `state_exists` and `state_exists_and_equals` but I could not think of better names for those and think the improved readability of `in_state` is worth it.
bors bot pushed a commit that referenced this pull request Feb 14, 2023
# Objective

- Improve readability of the run condition for systems only running in a certain state

## Solution

- Rename `state_equals` to `in_state` (see [comment by cart](#7634 (comment)) in #7634 )
- `.run_if(state_equals(variant))` now is `.run_if(in_state(variant))`

This breaks the naming pattern a bit with the related conditions `state_exists` and `state_exists_and_equals` but I could not think of better names for those and think the improved readability of `in_state` is worth it.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
…engine#7667)

# Objective

Fixes bevyengine#7632.

As discussed in bevyengine#7634, it can be quite challenging for users to intuit the mental model of how states now work.

## Solution

Rather than change the behavior of the `OnUpdate` system set, instead work on making sure it's easy to understand what's going on.

Two things have been done:

1. Remove the `.on_update` method from our bevy of system building traits. This was special-cased and made states feel much more magical than they need to.
2. Improve the docs for the `OnUpdate` system set.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
# Objective

- Improve readability of the run condition for systems only running in a certain state

## Solution

- Rename `state_equals` to `in_state` (see [comment by cart](bevyengine#7634 (comment)) in bevyengine#7634 )
- `.run_if(state_equals(variant))` now is `.run_if(in_state(variant))`

This breaks the naming pattern a bit with the related conditions `state_exists` and `state_exists_and_equals` but I could not think of better names for those and think the improved readability of `in_state` is worth it.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
…engine#7667)

# Objective

Fixes bevyengine#7632.

As discussed in bevyengine#7634, it can be quite challenging for users to intuit the mental model of how states now work.

## Solution

Rather than change the behavior of the `OnUpdate` system set, instead work on making sure it's easy to understand what's going on.

Two things have been done:

1. Remove the `.on_update` method from our bevy of system building traits. This was special-cased and made states feel much more magical than they need to.
2. Improve the docs for the `OnUpdate` system set.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
# Objective

- Improve readability of the run condition for systems only running in a certain state

## Solution

- Rename `state_equals` to `in_state` (see [comment by cart](bevyengine#7634 (comment)) in bevyengine#7634 )
- `.run_if(state_equals(variant))` now is `.run_if(in_state(variant))`

This breaks the naming pattern a bit with the related conditions `state_exists` and `state_exists_and_equals` but I could not think of better names for those and think the improved readability of `in_state` is worth it.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
…engine#7667)

# Objective

Fixes bevyengine#7632.

As discussed in bevyengine#7634, it can be quite challenging for users to intuit the mental model of how states now work.

## Solution

Rather than change the behavior of the `OnUpdate` system set, instead work on making sure it's easy to understand what's going on.

Two things have been done:

1. Remove the `.on_update` method from our bevy of system building traits. This was special-cased and made states feel much more magical than they need to.
2. Improve the docs for the `OnUpdate` system set.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
# Objective

- Improve readability of the run condition for systems only running in a certain state

## Solution

- Rename `state_equals` to `in_state` (see [comment by cart](bevyengine#7634 (comment)) in bevyengine#7634 )
- `.run_if(state_equals(variant))` now is `.run_if(in_state(variant))`

This breaks the naming pattern a bit with the related conditions `state_exists` and `state_exists_and_equals` but I could not think of better names for those and think the improved readability of `in_state` is worth it.
cart added a commit that referenced this pull request Mar 30, 2023
# Objective

- Fixes #7659

## Solution

The idea of anonymous system sets or "implicit hidden organizational
sets" was briefly mentioned by @cart here:
#7634 (comment).

- `Schedule::add_systems` creates an implicit, anonymous system set of
all systems in `SystemConfigs`.
- All dependencies and conditions from the `SystemConfigs` are now
applied to the implicit system set, instead of being applied to each
individual system. This should not change the behavior, AFAIU, because
`before`, `after`, `run_if` and `ambiguous_with` are transitive
properties from a set to its members.
- The newly added `AnonymousSystemSet` stores the names of its members
to provide better error messages.
- The names are stored in a reference counted slice, allowing fast
clones of the `AnonymousSystemSet`.
- However, only the pointer of the slice is used for hash and equality
operations
- This ensures that two `AnonymousSystemSet` are not equal, even if they
have the same members / member names.
- So two identical `add_systems` calls will produce two different
`AnonymousSystemSet`s.
  - Clones of the same `AnonymousSystemSet` will be equal.

## Drawbacks
If my assumptions are correct, the observed behavior should stay the
same. But the number of system sets in the `Schedule` will increase with
each `add_systems` call. If this has negative performance implications,
`add_systems` could be changed to only create the implicit system set if
necessary / when a run condition was added.

---------

Co-authored-by: Carter Anderson <[email protected]>
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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected SystemInMultipleBaseSets panic due to on_update
3 participants