From 46159099bbd46fbf64ba55d02b70a3a4389001d5 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 21 Dec 2020 22:33:27 +0000 Subject: [PATCH 1/3] Make current state a property of StateStage Extends #1021 Fixes #1117 This also allows avoiding the Clone bound on state Possible future work: - Make state use Eq instead --- crates/bevy_ecs/src/schedule/state.rs | 69 ++++++++++++--------------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/state.rs b/crates/bevy_ecs/src/schedule/state.rs index f54a5f0ebaf1c..0becb83bb2c82 100644 --- a/crates/bevy_ecs/src/schedule/state.rs +++ b/crates/bevy_ecs/src/schedule/state.rs @@ -1,6 +1,9 @@ use crate::{Resource, Resources, Stage, System, SystemStage, World}; use bevy_utils::HashMap; -use std::{mem::Discriminant, ops::Deref}; +use std::{ + mem::{self, Discriminant}, + ops::Deref, +}; use thiserror::Error; pub(crate) struct StateStages { @@ -21,12 +24,14 @@ impl Default for StateStages { pub struct StateStage { stages: HashMap, StateStages>, + current_stage: Option>, } impl Default for StateStage { fn default() -> Self { Self { stages: Default::default(), + current_stage: None, } } } @@ -142,14 +147,12 @@ impl StateStage { } fn state_stages(&mut self, state: T) -> &mut StateStages { - self.stages - .entry(std::mem::discriminant(&state)) - .or_default() + self.stages.entry(mem::discriminant(&state)).or_default() } } #[allow(clippy::mem_discriminant_non_enum)] -impl Stage for StateStage { +impl Stage for StateStage { fn initialize(&mut self, world: &mut World, resources: &mut Resources) { for state_stages in self.stages.values_mut() { state_stages.enter.initialize(world, resources); @@ -160,33 +163,25 @@ impl Stage for StateStage { fn run(&mut self, world: &mut World, resources: &mut Resources) { let current_stage = loop { - let (next_stage, current_stage) = { + let next = { let mut state = resources .get_mut::>() .expect("Missing state resource"); - let result = ( - state.next.as_ref().map(|next| std::mem::discriminant(next)), - std::mem::discriminant(&state.current), - ); - - state.apply_next(); - - result + state.previous = state.apply_next().or(state.previous.take()); + mem::discriminant(&state.current) }; - - // if next_stage is Some, we just applied a new state - if let Some(next_stage) = next_stage { - if next_stage != current_stage { - if let Some(current_state_stages) = self.stages.get_mut(¤t_stage) { - current_state_stages.exit.run(world, resources); - } + if self.current_stage == Some(next) { + break next; + } else { + if let Some(current_state_stages) = + self.current_stage.and_then(|it| self.stages.get_mut(&it)) + { + current_state_stages.exit.run(world, resources); } - - if let Some(next_state_stages) = self.stages.get_mut(&next_stage) { + self.current_stage = Some(next); + if let Some(next_state_stages) = self.stages.get_mut(&next) { next_state_stages.enter.run(world, resources); } - } else { - break current_stage; } }; @@ -204,20 +199,19 @@ pub enum StateError { } #[derive(Debug)] -pub struct State { +pub struct State { previous: Option, current: T, next: Option, } #[allow(clippy::mem_discriminant_non_enum)] -impl State { +impl State { pub fn new(state: T) -> Self { Self { - current: state.clone(), + current: state, + next: None, previous: None, - // add value to queue so that we "enter" the state - next: Some(state), } } @@ -235,7 +229,7 @@ impl State { /// Queue a state change. This will fail if there is already a state in the queue, or if the given `state` matches the current state pub fn set_next(&mut self, state: T) -> Result<(), StateError> { - if std::mem::discriminant(&self.current) == std::mem::discriminant(&state) { + if mem::discriminant(&self.current) == mem::discriminant(&state) { return Err(StateError::AlreadyInState); } @@ -249,7 +243,7 @@ impl State { /// Same as [Self::queue], but there is already a next state, it will be overwritten instead of failing pub fn overwrite_next(&mut self, state: T) -> Result<(), StateError> { - if std::mem::discriminant(&self.current) == std::mem::discriminant(&state) { + if mem::discriminant(&self.current) == mem::discriminant(&state) { return Err(StateError::AlreadyInState); } @@ -257,17 +251,16 @@ impl State { Ok(()) } - fn apply_next(&mut self) { + fn apply_next(&mut self) -> Option { if let Some(next) = self.next.take() { - let previous = std::mem::replace(&mut self.current, next); - if std::mem::discriminant(&previous) != std::mem::discriminant(&self.current) { - self.previous = Some(previous) - } + Some(std::mem::replace(&mut self.current, next)) + } else { + None } } } -impl Deref for State { +impl Deref for State { type Target = T; fn deref(&self) -> &Self::Target { From 43b9245532fd29df65a7c95371c56c6bbea1e74c Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 21 Dec 2020 22:35:44 +0000 Subject: [PATCH 2/3] Fix example to not use clone on AppState Maybe this will seem a bit too much like magic to someone who doesn't know about mem::discriminant? Oh well! Also fix AppBuilder to not require otherwise unused clones --- crates/bevy_app/src/app_builder.rs | 6 +++--- examples/ecs/state.rs | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/bevy_app/src/app_builder.rs b/crates/bevy_app/src/app_builder.rs index fb69222b9b5ef..00dbc5c71a19b 100644 --- a/crates/bevy_app/src/app_builder.rs +++ b/crates/bevy_app/src/app_builder.rs @@ -129,7 +129,7 @@ impl AppBuilder { self.add_system_to_stage(stage::UPDATE, system) } - pub fn on_state_enter>( + pub fn on_state_enter>( &mut self, stage: &str, state: T, @@ -140,7 +140,7 @@ impl AppBuilder { }) } - pub fn on_state_update>( + pub fn on_state_update>( &mut self, stage: &str, state: T, @@ -151,7 +151,7 @@ impl AppBuilder { }) } - pub fn on_state_exit>( + pub fn on_state_exit>( &mut self, stage: &str, state: T, diff --git a/examples/ecs/state.rs b/examples/ecs/state.rs index 5d7c31d779eb0..5c89607525e9f 100644 --- a/examples/ecs/state.rs +++ b/examples/ecs/state.rs @@ -18,7 +18,6 @@ fn main() { const STAGE: &str = "app_state"; -#[derive(Clone)] enum AppState { Menu, InGame, @@ -101,6 +100,7 @@ fn setup_game( commands: &mut Commands, asset_server: Res, mut materials: ResMut>, + state: Res>, ) { let texture_handle = asset_server.load("branding/icon.png"); commands @@ -109,6 +109,11 @@ fn setup_game( material: materials.add(texture_handle.into()), ..Default::default() }); + match state.previous() { + Some(AppState::Menu) => println!("Called setup_game from leaving the menu"), + Some(AppState::InGame) => unreachable!("Called setup_game from leaving InGame"), + None => unreachable!("Called setup_game as the first state"), + } } const SPEED: f32 = 100.0; From e70857b79a0868e0c52be44ce72bdd736251a302 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 21 Dec 2020 22:53:05 +0000 Subject: [PATCH 3/3] Fix error given by clippy --- crates/bevy_ecs/src/schedule/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/state.rs b/crates/bevy_ecs/src/schedule/state.rs index 0becb83bb2c82..8c266d2455271 100644 --- a/crates/bevy_ecs/src/schedule/state.rs +++ b/crates/bevy_ecs/src/schedule/state.rs @@ -167,7 +167,7 @@ impl Stage for StateStage { let mut state = resources .get_mut::>() .expect("Missing state resource"); - state.previous = state.apply_next().or(state.previous.take()); + state.previous = state.apply_next().or_else(|| state.previous.take()); mem::discriminant(&state.current) }; if self.current_stage == Some(next) {