From 4fc076325c765f52f60ac7e7616bc383c9f6039f Mon Sep 17 00:00:00 2001 From: Lynix Date: Sun, 19 Nov 2017 17:09:56 +0100 Subject: [PATCH] Sdk/StateMachine: Fix instantaneous state change --- ChangeLog.md | 1 + SDK/include/NDK/StateMachine.hpp | 22 ++++- SDK/include/NDK/StateMachine.inl | 151 ++++++++++++++++++------------- tests/SDK/NDK/StateMachine.cpp | 32 +++---- 4 files changed, 121 insertions(+), 85 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index c424c02a9..5c5b1491a 100644 --- a/ChangeLog.md +++ b/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: diff --git a/SDK/include/NDK/StateMachine.hpp b/SDK/include/NDK/StateMachine.hpp index e9e665c44..830391e55 100644 --- a/SDK/include/NDK/StateMachine.hpp +++ b/SDK/include/NDK/StateMachine.hpp @@ -24,15 +24,13 @@ namespace Ndk inline void ChangeState(std::shared_ptr state); - inline const std::shared_ptr& GetCurrentState() const; - inline bool IsTopState(const State* state) const; - inline std::shared_ptr PopState(); - inline bool PopStatesUntil(std::shared_ptr state); + inline void PopState(); + inline void PopStatesUntil(std::shared_ptr state); inline void PushState(std::shared_ptr state); - inline void SetState(std::shared_ptr state); + inline void ResetState(std::shared_ptr 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; + }; + std::vector> m_states; + std::vector m_transitions; }; } diff --git a/SDK/include/NDK/StateMachine.inl b/SDK/include/NDK/StateMachine.inl index 1086df54d..cfc021233 100644 --- a/SDK/include/NDK/StateMachine.inl +++ b/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 #include #include @@ -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 - * - * \remark Calls "Enter" on the state - * \remark Produces a NazaraAssert if nullptr is given + * \param originalState State which is the entry point of the application, a nullptr will create an empty state machine */ - inline StateMachine::StateMachine(std::shared_ptr originalState) { - NazaraAssert(originalState, "StateMachine must have a state to begin with"); - PushState(std::move(originalState)); + if (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 : m_states) - state->Leave(*this); + // Leave state from top to bottom (as if states were popped out) + 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) { if (state) { - PopState(); - PushState(std::move(state)); + // Change state is just a pop followed by a push + 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& 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 std::shared_ptr StateMachine::PopState() + inline void StateMachine::PopState() { - if (m_states.empty()) - return nullptr; + StateTransition transition; + transition.type = TransitionType::Pop; - m_states.back()->Leave(*this); - std::shared_ptr oldTopState = std::move(m_states.back()); - m_states.pop_back(); - return oldTopState; + m_transitions.emplace_back(std::move(transition)); } /*! - * \brief Pops all the states of the machine until a specific one is reached - * \return true If that specific state is on top, false if stack is empty + * \brief Pops all states of the machine until a specific one is reached * - * \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 bool StateMachine::PopStatesUntil(std::shared_ptr state) + inline void StateMachine::PopStatesUntil(std::shared_ptr state) { - if (!state) - return false; + if (state) + { + StateTransition transition; + transition.state = std::move(state); + transition.type = TransitionType::PopUntil; - while (!m_states.empty() && !IsTopState(state.get())) - PopState(); - - return !m_states.empty(); + m_transitions.emplace_back(std::move(transition)); + } } /*! @@ -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) { 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_states.back()->Enter(*this); + m_transitions.emplace_back(std::move(transition)); } } /*! * \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::SetState(std::shared_ptr state) + inline void StateMachine::ResetState(std::shared_ptr 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()) - PopState(); - - PushState(std::move(state)); + transition.state = std::move(state); + transition.type = TransitionType::Push; + m_transitions.emplace_back(std::move(transition)); } } @@ -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& 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) { return state->Update(*this, elapsedTime); }); diff --git a/tests/SDK/NDK/StateMachine.cpp b/tests/SDK/NDK/StateMachine.cpp index 066a150d7..15502c5cf 100644 --- a/tests/SDK/NDK/StateMachine.cpp +++ b/tests/SDK/NDK/StateMachine.cpp @@ -64,8 +64,8 @@ SCENARIO("State & StateMachine", "[NDK][STATE]") std::shared_ptr secondTestState = std::make_shared(); Ndk::StateMachine stateMachine(secondTestState); stateMachine.PushState(testState); - REQUIRE(!testState->IsUpdated()); - REQUIRE(!secondTestState->IsUpdated()); + CHECK(!testState->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())); - REQUIRE(!stateMachine.IsTopState(secondTestState.get())); + CHECK(stateMachine.IsTopState(testState.get())); + CHECK(!stateMachine.IsTopState(secondTestState.get())); - REQUIRE(testState->IsUpdated()); - REQUIRE(!secondTestState->IsUpdated()); + CHECK(testState->IsUpdated()); + CHECK(!secondTestState->IsUpdated()); } } WHEN("We exchange the states' positions while emptying the stack") { - REQUIRE(stateMachine.PopStatesUntil(secondTestState)); - REQUIRE(stateMachine.IsTopState(secondTestState.get())); + stateMachine.PopStatesUntil(secondTestState); + stateMachine.Update(1.f); + CHECK(stateMachine.IsTopState(secondTestState.get())); - std::shared_ptr oldState = stateMachine.PopState(); - REQUIRE(stateMachine.PopState() == nullptr); - - stateMachine.SetState(testState); - stateMachine.PushState(oldState); + stateMachine.ResetState(testState); + stateMachine.PushState(secondTestState); stateMachine.Update(1.f); THEN("Both states should be updated") { - REQUIRE(!stateMachine.IsTopState(testState.get())); - REQUIRE(stateMachine.IsTopState(secondTestState.get())); + CHECK(!stateMachine.IsTopState(testState.get())); + CHECK(stateMachine.IsTopState(secondTestState.get())); - REQUIRE(testState->IsUpdated()); - REQUIRE(secondTestState->IsUpdated()); + CHECK(testState->IsUpdated()); + CHECK(secondTestState->IsUpdated()); } } }