Sdk/StateMachine: Fix instantaneous state change

ChangeLog.md

@@ -25,6 +25,7 @@ Nazara Development Kit:
 - Added BaseWidget::ClearFocus method and OnFocus[Lost|Received] virtual methods
 - TextAreaWidget will now show a cursor as long as it has focus
 - Fix BaseWidget linking error on Linux
+- ⚠️ Rewrite StateMachine to fix instantaneous state changing (state change is no longer effective until the next update call)
 
 # 0.4:
 

SDK/include/NDK/StateMachine.hpp

@@ -24,15 +24,13 @@ namespace Ndk
 
 			inline void ChangeState(std::shared_ptr<State> state);
 
-			inline const std::shared_ptr<State>& GetCurrentState() const;
-
 			inline bool IsTopState(const State* state) const;
 
-			inline std::shared_ptr<State> PopState();
+			inline void PopState();
-			inline bool PopStatesUntil(std::shared_ptr<State> state);
+			inline void PopStatesUntil(std::shared_ptr<State> state);
 			inline void PushState(std::shared_ptr<State> state);
 
-			inline void SetState(std::shared_ptr<State> state);
+			inline void ResetState(std::shared_ptr<State> state);
 
 			inline bool Update(float elapsedTime);
 
@@ -40,7 +38,21 @@ namespace Ndk
 			StateMachine& operator=(const StateMachine&) = delete;
 
 		private:
+			enum class TransitionType
+			{
+				Pop,
+				PopUntil,
+				Push,
+			};
+
+			struct StateTransition
+			{
+				TransitionType type;
+				std::shared_ptr<State> state;
+			};
+
 			std::vector<std::shared_ptr<State>> m_states;
+			std::vector<StateTransition> m_transitions;
 	};
 }
 

SDK/include/NDK/StateMachine.inl

@@ -2,6 +2,7 @@
 // This file is part of the "Nazara Development Kit"
 // For conditions of distribution and use, see copyright notice in Prerequesites.hpp
 
+#include <NDK/StateMachine.hpp>
 #include <Nazara/Core/Error.hpp>
 #include <utility>
 
@@ -16,66 +17,56 @@ namespace Ndk
 	/*!
 	* \brief Constructs a StateMachine object with an original state
 	*
-	* \param originalState State which is the entry point of the application
+	* \param originalState State which is the entry point of the application, a nullptr will create an empty state machine
-	*
-	* \remark Calls "Enter" on the state
-	* \remark Produces a NazaraAssert if nullptr is given
 	*/
-
 	inline StateMachine::StateMachine(std::shared_ptr<State> originalState)
 	{
-		NazaraAssert(originalState, "StateMachine must have a state to begin with");
+		if (originalState)
-		PushState(std::move(originalState));
+			PushState(std::move(originalState));
 	}
 
 	/*!
 	* \brief Destructs the object
 	*
-	* \remark Calls "Leave" on all the states
+	* \remark Calls "Leave" on all the states from top to bottom
 	*/
-
 	inline StateMachine::~StateMachine()
 	{
-		for (std::shared_ptr<State>& state : m_states)
+		// Leave state from top to bottom (as if states were popped out)
-			state->Leave(*this);
+		for (auto it = m_states.rbegin(); it != m_states.rend(); ++it)
+			(*it)->Leave(*this);
 	}
 
 	/*!
 	* \brief Replaces the current state on the top of the machine
 	*
 	* \param state State to replace the top one if it is nullptr, no action is performed
+	*
+	* \remark It is forbidden for a state machine to have (at any moment) the same state in its list multiple times
+	* \remark Like all actions popping or pushing a state, this is not immediate and will only take effect when state machine is updated
 	*/
-
 	inline void StateMachine::ChangeState(std::shared_ptr<State> state)
 	{
 		if (state)
 		{
-			PopState();
+			// Change state is just a pop followed by a push
-			PushState(std::move(state));
+			StateTransition transition;
+			transition.type = TransitionType::Pop;
+			m_transitions.emplace_back(std::move(transition));
+
+			transition.state = std::move(state);
+			transition.type = TransitionType::Push;
+			m_transitions.emplace_back(std::move(transition));
 		}
 	}
 
-	/*!
-	* \brief Gets the current state on the top of the machine
-	* \return A constant reference to the state
-	*
-	* \remark The stack is supposed to be non empty, otherwise it is undefined behaviour
-	*
-	* \see PopStatesUntil
-	*/
-
-	inline const std::shared_ptr<State>& StateMachine::GetCurrentState() const
-	{
-		return m_states.back();
-	}
-
 	/*!
 	* \brief Checks whether the state is on the top of the machine
 	* \return true If it is the case
 	*
 	* \param state State to compare the top with
+	* \remark Because all actions popping or pushing a state don't take effect until next state machine update, this can return false on a just pushed state
 	*/
-
 	inline bool StateMachine::IsTopState(const State* state) const
 	{
 		if (m_states.empty())
@@ -86,40 +77,36 @@ namespace Ndk
 
 	/*!
 	* \brief Pops the state on the top of the machine
-	* \return Old state on the top, nullptr if stack was empty
 	*
 	* \remark This method can completely empty the stack
+	* \remark Like all actions popping or pushing a state, this is not immediate and will only take effect when state machine is updated
 	*/
-
+	inline void StateMachine::PopState()
-	inline std::shared_ptr<State> StateMachine::PopState()
 	{
-		if (m_states.empty())
+		StateTransition transition;
-			return nullptr;
+		transition.type = TransitionType::Pop;
 
-		m_states.back()->Leave(*this);
+		m_transitions.emplace_back(std::move(transition));
-		std::shared_ptr<State> oldTopState = std::move(m_states.back());
-		m_states.pop_back();
-		return oldTopState;
 	}
 
 	/*!
-	* \brief Pops all the states of the machine until a specific one is reached
+	* \brief Pops all states of the machine until a specific one is reached
-	* \return true If that specific state is on top, false if stack is empty
 	*
-	* \param state State to find on the stack if it is nullptr, no action is performed
+	* \param state State to find on the stack. If nullptr is passed, no action is performed
 	*
-	* \remark This method can completely empty the stack
+	* \remark This method will completely empty the stack if state is not present
+	* \remark Like all actions popping or pushing a state, this is not immediate and will only take effect when state machine is updated
 	*/
-
+	inline void StateMachine::PopStatesUntil(std::shared_ptr<State> state)
-	inline bool StateMachine::PopStatesUntil(std::shared_ptr<State> state)
 	{
-		if (!state)
+		if (state)
-			return false;
+		{
+			StateTransition transition;
+			transition.state = std::move(state);
+			transition.type = TransitionType::PopUntil;
 
-		while (!m_states.empty() && !IsTopState(state.get()))
+			m_transitions.emplace_back(std::move(transition));
-			PopState();
+		}
-
-		return !m_states.empty();
 	}
 
 	/*!
@@ -127,34 +114,40 @@ namespace Ndk
 	*
 	* \param state Next state to represent if it is nullptr, it performs no action
 	*
-	* \remark Produces a NazaraAssert if the same state is pushed two times on the stack
+	* \remark It is forbidden for a state machine to have (at any moment) the same state in its list multiple times
+	* \remark Like all actions popping or pushing a state, this is not immediate and will only take effect when state machine is updated
 	*/
-
 	inline void StateMachine::PushState(std::shared_ptr<State> state)
 	{
 		if (state)
 		{
-			NazaraAssert(std::find(m_states.begin(), m_states.end(), state) == m_states.end(), "The same state was pushed two times");
+			StateTransition transition;
+			transition.state = std::move(state);
+			transition.type = TransitionType::Push;
 
-			m_states.push_back(std::move(state));
+			m_transitions.emplace_back(std::move(transition));
-			m_states.back()->Enter(*this);
 		}
 	}
 
 	/*!
 	* \brief Pops every states of the machine to put a new one
 	*
-	* \param state State to reset the stack with if it is nullptr, no action is performed
+	* \param state State to reset the stack with. If state is invalid, this will clear the state machine
+	*
+	* \remark It is forbidden for a state machine to have (at any moment) the same state in its list multiple times
+	* \remark Like all actions popping or pushing a state, this is not immediate and will only take effect when state machine is updated
 	*/
-
+	inline void StateMachine::ResetState(std::shared_ptr<State> state)
-	inline void StateMachine::SetState(std::shared_ptr<State> state)
 	{
+		StateTransition transition;
+		transition.type = TransitionType::PopUntil; //< Pop until nullptr, which basically clears the state machine
+		m_transitions.emplace_back(std::move(transition));
+
 		if (state)
 		{
-			while (!m_states.empty())
+			transition.state = std::move(state);
-				PopState();
+			transition.type = TransitionType::Push;
-
+			m_transitions.emplace_back(std::move(transition));
-			PushState(std::move(state));
 		}
 	}
 
@@ -164,9 +157,41 @@ namespace Ndk
 	*
 	* \param elapsedTime Delta time used for the update
 	*/
-
 	inline bool StateMachine::Update(float elapsedTime)
 	{
+		for (StateTransition& transition : m_transitions)
+		{
+			switch (transition.type)
+			{
+				case TransitionType::Pop:
+				{
+					std::shared_ptr<State>& topState = m_states.back();
+					topState->Leave(*this); //< Call leave before popping to ensure consistent IsTopState behavior
+
+					m_states.pop_back();
+					break;
+				}
+
+				case TransitionType::PopUntil:
+				{
+					while (!m_states.empty() && m_states.back() != transition.state)
+					{
+						m_states.back()->Leave(*this);
+						m_states.pop_back();
+					}
+					break;
+				}
+
+				case TransitionType::Push:
+				{
+					m_states.emplace_back(std::move(transition.state));
+					m_states.back()->Enter(*this);
+					break;
+				}
+			}
+		}
+		m_transitions.clear();
+
 		return std::all_of(m_states.begin(), m_states.end(), [=](std::shared_ptr<State>& state) {
 			return state->Update(*this, elapsedTime);
 		});

tests/SDK/NDK/StateMachine.cpp

@@ -64,8 +64,8 @@ SCENARIO("State & StateMachine", "[NDK][STATE]")
 		std::shared_ptr<SecondTestState> secondTestState = std::make_shared<SecondTestState>();
 		Ndk::StateMachine stateMachine(secondTestState);
 		stateMachine.PushState(testState);
-		REQUIRE(!testState->IsUpdated());
+		CHECK(!testState->IsUpdated());
-		REQUIRE(!secondTestState->IsUpdated());
+		CHECK(!secondTestState->IsUpdated());
 
 		WHEN("We update our machine")
 		{
@@ -73,34 +73,32 @@ SCENARIO("State & StateMachine", "[NDK][STATE]")
 
 			THEN("Our state on the top has been updated but not the bottom one")
 			{
-				REQUIRE(stateMachine.IsTopState(testState.get()));
+				CHECK(stateMachine.IsTopState(testState.get()));
-				REQUIRE(!stateMachine.IsTopState(secondTestState.get()));
+				CHECK(!stateMachine.IsTopState(secondTestState.get()));
 
-				REQUIRE(testState->IsUpdated());
+				CHECK(testState->IsUpdated());
-				REQUIRE(!secondTestState->IsUpdated());
+				CHECK(!secondTestState->IsUpdated());
 			}
 		}
 
 		WHEN("We exchange the states' positions while emptying the stack")
 		{
-			REQUIRE(stateMachine.PopStatesUntil(secondTestState));
+			stateMachine.PopStatesUntil(secondTestState);
-			REQUIRE(stateMachine.IsTopState(secondTestState.get()));
+			stateMachine.Update(1.f);
+			CHECK(stateMachine.IsTopState(secondTestState.get()));
 
-			std::shared_ptr<Ndk::State> oldState = stateMachine.PopState();
+			stateMachine.ResetState(testState);
-			REQUIRE(stateMachine.PopState() == nullptr);
+			stateMachine.PushState(secondTestState);
-
-			stateMachine.SetState(testState);
-			stateMachine.PushState(oldState);
 
 			stateMachine.Update(1.f);
 
 			THEN("Both states should be updated")
 			{
-				REQUIRE(!stateMachine.IsTopState(testState.get()));
+				CHECK(!stateMachine.IsTopState(testState.get()));
-				REQUIRE(stateMachine.IsTopState(secondTestState.get()));
+				CHECK(stateMachine.IsTopState(secondTestState.get()));
 
-				REQUIRE(testState->IsUpdated());
+				CHECK(testState->IsUpdated());
-				REQUIRE(secondTestState->IsUpdated());
+				CHECK(secondTestState->IsUpdated());
 			}
 		}
 	}