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

[Merged by Bors] - Redo State architecture #1424

Closed
wants to merge 26 commits into from

Conversation

TheRawMeatball
Copy link
Member

An alternative to StateStages that uses SystemSets. Also includes pop and push operations since this was originally developed for my personal project which needed them.

@Ratysz
Copy link
Contributor

Ratysz commented Feb 10, 2021

Suggested changes from Discord earlier: SetState -> State (assuming StateStage is removed); StateSetBuilder -> StateSet; remove ScheduledOperation, split schedule_operation() into three methods.

Speaking of StateStage: since this is more or less a replacement, might be a good idea to try removing it now as well.

I'm not sure I like the admittedly clever hack with state_cleaner: it hides from the user what's actually going on. If I were to use this, I would definitely try it via regular system sets with state-related run criteria and assume the state would be driven... somehow. This is why I keep suggesting that state-driving system should be required to be added explicitly, by the user; there is just too many edge cases to try and cover for all of them automatically without being implicit somewhere.

@TheRawMeatball
Copy link
Member Author

Hmm, I agree with most of your points but I'd argue keeping state_cleaner a hidden implementation detail is the better choice: I feel like adding a state should automatically add all necessary parts, including the driver. As an improvement I might make soon, this impl can probably be improved by automatically adding in relations between the driver system and all systems in the driven sets to remove the need for the hard sync point. If the state driver was to be manual, this burden would be on the user, and I don't think thats very elegant.

@Ratysz
Copy link
Contributor

Ratysz commented Feb 10, 2021

I am insinuating that this approach relies on its own abstraction, rather than integrating well with existing ones. How will this interoperate with plain system sets (i.e. what if something uses the provided run criteria without going through StateSet)? With states being used across stages? How will this work with #1375?

That said, the improvement you're planning will, in my opinion, make this a very good direct replacement to StateStage until at least the time we figure out the questions above.

As for epithets, "elegant" is when things work out beautifully and with perfect clarity; this idea strikes me as "sneaky" at best - which doesn't mean it's bad.

@TheRawMeatball
Copy link
Member Author

Alright, I cleaned up the API and made it more aligned with the rest of the ECS. I'll update the example when #1428 is merged.

@TheRawMeatball
Copy link
Member Author

Ok, the PR is ready except for the examples, which I'll fix after #1453

crates/bevy_ecs/src/schedule/state.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/state.rs Outdated Show resolved Hide resolved
@@ -61,6 +67,9 @@ impl<T: Clone + Resource> State<T> {
Wrapper::<T, OnExit>::new(discriminant(&d))
}

/// Creates a driver set for the State.
///
/// Important note: this set must be inserted **before** all other state-dependant sets to work properly!
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate, but I don't think this can be avoided until #1375 or some stop-gap measure that lets us order run criteria evaluation.

Base automatically changed from master to main February 19, 2021 20:44
@TheRawMeatball TheRawMeatball changed the title Implement State sets Redo State architecture Mar 5, 2021
@TheRawMeatball TheRawMeatball marked this pull request as ready for review March 5, 2021 16:32
Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
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.

This seems like a very solid improvement to StateStages: the way it's decoupled makes much more sense and the general API seems sensible. Are there any good patterns for working with compound states with this API? If so, they likely deserve their own example.

})
pub fn add_state<T: Component + Clone + Eq>(&mut self, initial: T) -> &mut Self {
self.insert_resource(State::new(initial))
.add_system_set(State::<T>::make_driver())
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add these system sets (and hence states) to stages other than Update?

Copy link
Member Author

Choose a reason for hiding this comment

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

add_state_to_stage is the solution for this.

scoreboard_system.system(),
.add_system_set(State::on_enter_set(GameState::Playing).with_system(setup.system()))
.add_system_set(
State::on_update_set(GameState::Playing)
Copy link
Member

Choose a reason for hiding this comment

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

on_enter_set and friends is a bit of a confusing name in English: "set" could either be a verb (to set a property on update...) or a "noun" (a collection of systems). I think it's the latter here from context.

IMO State::on_enter (etc.) would be clearer in typical usage, since you're seeing the system set call just above it already.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, those names are reserved for the transition functions...

What about on_enter_systems?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't feel strongly either way, so sure.

Copy link
Member

Choose a reason for hiding this comment

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

I think I like this:

.add_system_set(SystemSet::on_enter(GameState::Playing).with_system(setup.system()))

more than this:

.add_system_set(State::on_enter_set(GameState::Playing).with_system(setup.system()))

Normally I would prefer the current impl as it's a clean "layer" on top of system sets, but SystemSet::on_enter reads better and is clearer about the type. The fact that they are in the same module also makes me feel better about it.

let stages = self.state_stages(state);
stages.enter = Box::new(stage);
self
pub fn on_inactive_update(s: T) -> impl System<In = (), Out = ShouldRun> {
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a doc string describing this function? It's not clear from either the name or a quick look at the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, documentation on the various on_*_set methods is a good idea, though I'd probably put the actual docs on on_* and link.

scoreboard_system.system(),
.add_system_set(State::on_enter_set(GameState::Playing).with_system(setup.system()))
.add_system_set(
State::on_update_set(GameState::Playing)
Copy link
Member

Choose a reason for hiding this comment

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

I think I like this:

.add_system_set(SystemSet::on_enter(GameState::Playing).with_system(setup.system()))

more than this:

.add_system_set(State::on_enter_set(GameState::Playing).with_system(setup.system()))

Normally I would prefer the current impl as it's a clean "layer" on top of system sets, but SystemSet::on_enter reads better and is clearer about the type. The fact that they are in the same module also makes me feel better about it.

crates/bevy_ecs/src/schedule/state.rs Outdated Show resolved Hide resolved
@cart cart added this to the Bevy 0.5 milestone Mar 15, 2021
@cart
Copy link
Member

cart commented Mar 15, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 15, 2021
An alternative to StateStages that uses SystemSets. Also includes pop and push operations since this was originally developed for my personal project which needed them.
@bors bors bot changed the title Redo State architecture [Merged by Bors] - Redo State architecture Mar 15, 2021
@bors bors bot closed this Mar 15, 2021
@TheRawMeatball TheRawMeatball deleted the state-set-v2 branch March 15, 2021 22:39
@zicklag
Copy link
Member

zicklag commented Mar 16, 2021

Thank you for this @TheRawMeatball !! Me and my brother were literally just talking about how we felt like state could be simpler and then a couple hours later this is merged. It's great!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants