From 276d1a656e2c440954bab1f63fd138b501d7efc9 Mon Sep 17 00:00:00 2001 From: bodymovin Date: Fri, 24 Jan 2025 17:30:49 +0000 Subject: [PATCH] check whether state can change before evaluating conditions this is a regression from the new implementation to use triggers per state machine layer. Since canChangeState was being called after evaluating the conditions, the trigger would be marked as used but the state wouldn't change. This makes sure canChangeState is called before. Diffs= 7f3314f4f9 check whether state can change before evaluating conditions (#8917) Co-authored-by: hernan --- .rive_head | 1 - .../state_machine_input_instance.hpp | 13 ++- .../transition_trigger_condition.hpp | 2 + src/animation/state_machine_instance.cpp | 60 +++++++------ src/animation/state_transition.cpp | 9 ++ ...ansition_property_viewmodel_comparator.cpp | 4 +- .../transition_trigger_condition.cpp | 16 +++- .../assets/state_machine_triggers.riv | Bin 0 -> 292 bytes .../unit_tests/runtime/state_machine_test.cpp | 79 ++++++++++++++++++ 9 files changed, 151 insertions(+), 33 deletions(-) create mode 100644 tests/unit_tests/assets/state_machine_triggers.riv diff --git a/.rive_head b/.rive_head index 62d82ccb8..e69de29bb 100644 --- a/.rive_head +++ b/.rive_head @@ -1 +0,0 @@ -24d9162103a73712ba3c80cfc909fee6948a0089 diff --git a/include/rive/animation/state_machine_input_instance.hpp b/include/rive/animation/state_machine_input_instance.hpp index 6d947ed6d..23958f296 100644 --- a/include/rive/animation/state_machine_input_instance.hpp +++ b/include/rive/animation/state_machine_input_instance.hpp @@ -79,15 +79,22 @@ class Triggerable { public: - bool useInLayer(StateMachineLayerInstance* layer) const + bool isUsedInLayer(StateMachineLayerInstance* layer) const + { + auto it = std::find(m_usedLayers.begin(), m_usedLayers.end(), layer); + if (it == m_usedLayers.end()) + { + return false; + } + return true; + } + void useInLayer(StateMachineLayerInstance* layer) const { auto it = std::find(m_usedLayers.begin(), m_usedLayers.end(), layer); if (it == m_usedLayers.end()) { m_usedLayers.push_back(layer); - return true; } - return false; } protected: diff --git a/include/rive/animation/transition_trigger_condition.hpp b/include/rive/animation/transition_trigger_condition.hpp index f08479323..8186434d6 100644 --- a/include/rive/animation/transition_trigger_condition.hpp +++ b/include/rive/animation/transition_trigger_condition.hpp @@ -9,6 +9,8 @@ class TransitionTriggerCondition : public TransitionTriggerConditionBase public: bool evaluate(const StateMachineInstance* stateMachineInstance, StateMachineLayerInstance* layerInstance) const override; + void useInLayer(const StateMachineInstance* stateMachineInstance, + StateMachineLayerInstance* layerInstance) const; protected: bool validateInputType(const StateMachineInput* input) const override; diff --git a/src/animation/state_machine_instance.cpp b/src/animation/state_machine_instance.cpp index 8d02ee34b..114e9b1a9 100644 --- a/src/animation/state_machine_instance.cpp +++ b/src/animation/state_machine_instance.cpp @@ -217,21 +217,25 @@ class StateMachineLayerInstance i++) { auto transition = stateFrom->transition(i); - auto allowed = transition->allowed(stateFromInstance, - m_stateMachineInstance, - this); - if (allowed == AllowTransition::yes && - canChangeState(transition->stateTo())) + if (canChangeState(transition->stateTo())) { - transition->evaluatedRandomWeight(transition->randomWeight()); - totalWeight += transition->randomWeight(); - } - else - { - transition->evaluatedRandomWeight(0); - if (allowed == AllowTransition::waitingForExit) + + auto allowed = transition->allowed(stateFromInstance, + m_stateMachineInstance, + this); + if (allowed == AllowTransition::yes) + { + transition->evaluatedRandomWeight( + transition->randomWeight()); + totalWeight += transition->randomWeight(); + } + else { - m_waitingForExit = true; + transition->evaluatedRandomWeight(0); + if (allowed == AllowTransition::waitingForExit) + { + m_waitingForExit = true; + } } } } @@ -270,21 +274,25 @@ class StateMachineLayerInstance i++) { auto transition = stateFrom->transition(i); - auto allowed = transition->allowed(stateFromInstance, - m_stateMachineInstance, - this); - if (allowed == AllowTransition::yes && - canChangeState(transition->stateTo())) + if (canChangeState(transition->stateTo())) { - transition->evaluatedRandomWeight(transition->randomWeight()); - return transition; - } - else - { - transition->evaluatedRandomWeight(0); - if (allowed == AllowTransition::waitingForExit) + + auto allowed = transition->allowed(stateFromInstance, + m_stateMachineInstance, + this); + if (allowed == AllowTransition::yes) { - m_waitingForExit = true; + transition->evaluatedRandomWeight( + transition->randomWeight()); + return transition; + } + else + { + transition->evaluatedRandomWeight(0); + if (allowed == AllowTransition::waitingForExit) + { + m_waitingForExit = true; + } } } } diff --git a/src/animation/state_transition.cpp b/src/animation/state_transition.cpp index 59ef92958..8ebb202c0 100644 --- a/src/animation/state_transition.cpp +++ b/src/animation/state_transition.cpp @@ -202,6 +202,15 @@ AllowTransition StateTransition::allowed( } } } + for (auto condition : m_Conditions) + { + if (condition->is()) + { + condition->as()->useInLayer( + stateMachineInstance, + layerInstance); + } + } return AllowTransition::yes; } diff --git a/src/animation/transition_property_viewmodel_comparator.cpp b/src/animation/transition_property_viewmodel_comparator.cpp index ca95feb69..a28860181 100644 --- a/src/animation/transition_property_viewmodel_comparator.cpp +++ b/src/animation/transition_property_viewmodel_comparator.cpp @@ -177,8 +177,8 @@ bool TransitionPropertyViewModelComparator::compare( if (source != nullptr && source->is()) { - if (!source->as()->useInLayer( - layerInstance)) + if (source->as() + ->isUsedInLayer(layerInstance)) { return false; diff --git a/src/animation/transition_trigger_condition.cpp b/src/animation/transition_trigger_condition.cpp index 2d4a5702c..c60458c6d 100644 --- a/src/animation/transition_trigger_condition.cpp +++ b/src/animation/transition_trigger_condition.cpp @@ -26,9 +26,23 @@ bool TransitionTriggerCondition::evaluate( } auto triggerInput = static_cast(inputInstance); - if (triggerInput->m_fired && triggerInput->useInLayer(layerInstance)) + if (triggerInput->m_fired && !triggerInput->isUsedInLayer(layerInstance)) { return true; } return false; } + +void TransitionTriggerCondition::useInLayer( + const StateMachineInstance* stateMachineInstance, + StateMachineLayerInstance* layerInstance) const +{ + + auto inputInstance = stateMachineInstance->input(inputId()); + if (inputInstance == nullptr) + { + return; + } + auto triggerInput = static_cast(inputInstance); + triggerInput->useInLayer(layerInstance); +} diff --git a/tests/unit_tests/assets/state_machine_triggers.riv b/tests/unit_tests/assets/state_machine_triggers.riv new file mode 100644 index 0000000000000000000000000000000000000000..42ec9641b11be4b8e20822796c73d06da2f39c75 GIT binary patch literal 292 zcmZ9GJxjxI7{u?)#i&pPt%we_iyt6JDZalX3W6?L9EBjE25g7erCaCj;^68x@GJOD zO5bCl8C~?>cJqNd?%?;p-Co^aIQahL7h6m?vfApXtDgEAXsASErNaN)fZ^c)Kis#o$H`J&Br)<;~e~VVv;yZN3W(%?F6TY@zOH=4@I3qe*gdg literal 0 HcmV?d00001 diff --git a/tests/unit_tests/runtime/state_machine_test.cpp b/tests/unit_tests/runtime/state_machine_test.cpp index 7247ea198..61c7724ba 100644 --- a/tests/unit_tests/runtime/state_machine_test.cpp +++ b/tests/unit_tests/runtime/state_machine_test.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -423,3 +424,81 @@ TEST_CASE("Transitions with reset applied to them.", "[file]") delete stateMachineInstance; } + +TEST_CASE("Triggers will only be used on allowed state changes.", "[file]") +{ + auto file = ReadRiveFile("assets/state_machine_triggers.riv"); + + auto artboard = file->artboard("main"); + + // We empty all factory reset resources to start the test clean + rive::AnimationResetFactory::releaseResources(); + REQUIRE(rive::AnimationResetFactory::resourcesCount() == 0); + + REQUIRE(artboard != nullptr); + REQUIRE(artboard->animationCount() == 1); + REQUIRE(artboard->stateMachineCount() == 1); + + auto stateMachine = artboard->stateMachine("State Machine 1"); + REQUIRE(stateMachine->layerCount() == 1); + REQUIRE(stateMachine->inputCount() == 1); + + auto abi = artboard->instance(); + rive::StateMachineInstance* stateMachineInstance = + new rive::StateMachineInstance(stateMachine, abi.get()); + stateMachineInstance->advanceAndApply(0.1f); + abi->advance(0.1f); + + auto trigger = stateMachine->input("Trigger 1"); + REQUIRE(trigger != nullptr); + REQUIRE(trigger->is()); + + auto layer = stateMachine->layer(0); + REQUIRE(layer->stateCount() == 5); + + REQUIRE(layer->anyState() != nullptr); + REQUIRE(layer->entryState() != nullptr); + REQUIRE(layer->exitState() != nullptr); + + rive::AnimationState* animationState1 = nullptr; + rive::AnimationState* animationState2 = nullptr; + + int foundAnimationStates = 0; + for (int i = 0; i < layer->stateCount(); i++) + { + auto state = layer->state(i); + if (state->is()) + { + foundAnimationStates++; + REQUIRE(state->as()->animation() != nullptr); + if (foundAnimationStates == 1) + { + animationState1 = state->as(); + } + else + { + animationState2 = state->as(); + } + } + } + REQUIRE(foundAnimationStates == 2); + REQUIRE(animationState1 != nullptr); + REQUIRE(animationState2 != nullptr); + + auto triggerInstance = stateMachineInstance->getTrigger("Trigger 1"); + REQUIRE(triggerInstance != nullptr); + + triggerInstance->fire(); + stateMachineInstance->advanceAndApply(0.1f); + auto currentLayerState = stateMachineInstance->layerState(0); + + REQUIRE(currentLayerState == animationState2); + + triggerInstance->fire(); + stateMachineInstance->advanceAndApply(0.1f); + currentLayerState = stateMachineInstance->layerState(0); + + REQUIRE(currentLayerState == animationState1); + + delete stateMachineInstance; +}