fix(scripting): Advance detached view model instances at end of frame (#13002) a77c1355bd Co-authored-by: hernan <hernan@rive.app>
diff --git a/.rive_head b/.rive_head index a0c0b4f..638af8d 100644 --- a/.rive_head +++ b/.rive_head
@@ -1 +1 @@ -ef0e10041348832ffb0d398b9bb20df97a1d00b8 +a77c1355bdd6e26f733edfd531ec7871f942efcf
diff --git a/include/rive/animation/state_machine_instance.hpp b/include/rive/animation/state_machine_instance.hpp index d8e90e5..a45b556 100644 --- a/include/rive/animation/state_machine_instance.hpp +++ b/include/rive/animation/state_machine_instance.hpp
@@ -142,6 +142,11 @@ const LayerState* stateChangedByIndex(size_t index) const; bool advanceAndApply(float secs) override; + // When advanceViewModels is false, skips consuming/advancing the bound and + // detached view model instances (used when a script drives a nested + // scripted artboard, whose view models are advanced by the host frame + // instead). Animation and artboard component reset still run. + bool advanceAndApply(float secs, bool advanceViewModels); void advancedDataContext(); void reset(); std::string name() const override;
diff --git a/include/rive/artboard.hpp b/include/rive/artboard.hpp index 6633181..0bf1a7f 100644 --- a/include/rive/artboard.hpp +++ b/include/rive/artboard.hpp
@@ -380,6 +380,10 @@ #ifdef WITH_RIVE_SCRIPTING void scriptingVM(ScriptingVM* value) { m_scriptingVM = value; } #endif + // Advances detached scripted view model instances (those with no parents), + // which are not reachable from the bound view model tree. No-op when + // scripting is disabled. Called at the end of each frame. + void advanceScriptedViewModels(); NestedArtboard* nestedArtboard(const std::string& name) const; NestedArtboard* nestedArtboardAtPath(const std::string& path) const;
diff --git a/include/rive/lua/rive_lua_libs.hpp b/include/rive/lua/rive_lua_libs.hpp index eb838ab..8b03697 100644 --- a/include/rive/lua/rive_lua_libs.hpp +++ b/include/rive/lua/rive_lua_libs.hpp
@@ -1076,7 +1076,8 @@ ScriptReffedArtboard(File* file, std::unique_ptr<ArtboardInstance>&& artboardInstance, rcp<ViewModelInstance> viewModelInstance, - rcp<DataContext> parentDataContext); + rcp<DataContext> parentDataContext, + ScriptingContext* scriptingContext); ~ScriptReffedArtboard(); rive::File* file(); @@ -1089,6 +1090,7 @@ std::unique_ptr<ArtboardInstance> m_artboard; std::unique_ptr<StateMachineInstance> m_stateMachine; rcp<ViewModelInstance> m_viewModelInstance; + ScriptingContext* m_scriptingContext = nullptr; }; class ScriptedArtboard @@ -1223,6 +1225,7 @@ rcp<ViewModel> m_viewModel; rcp<ViewModelInstance> m_viewModelInstance; std::unordered_map<std::string, int> m_propertyRefs; + ScriptingContext* m_scriptingContext = nullptr; }; class ScriptedPropertyViewModel : public ScriptedProperty, @@ -1488,6 +1491,23 @@ void recordMissingDependency(const std::string& requiringModule, const std::string& missingModule); + // Track detached view model instances (no parents, e.g. created via + // vm:instance()) so they can be advanced at the end of each frame — they + // are not reachable from the artboard's bound view model tree, so the + // normal DataContext advance never reaches them. Instances are keyed by + // owner lifetime, not by Lua-wrapper GC: every live owner (a + // ScriptedViewModel wrapper, a ScriptReffedArtboard) registers on + // construction and unregisters on destruction. The context holds a strong + // reference for as long as any owner is alive, so the instance survives + // even if the script drops its wrapper while it is still bound to a + // scripted artboard. + void trackViewModelInstance(rcp<ViewModelInstance> instance); + void untrackViewModelInstance(ViewModelInstance* instance); + // Advances every tracked instance that has no parents. Instances with + // parents are already advanced through the bound tree (and via their + // detached-root ancestor's recursion), so they are skipped. + void advanceDetachedViewModels(); + // Ore GPU context for this VM, derived from the render factory. Null when // there is no render context, or it is not GPU-backed. Returned as void* so // callers that include ore headers cast to ore::Context*. @@ -1571,6 +1591,18 @@ std::unordered_map<std::string, ModuleDetails*> m_moduleLookup; std::unordered_set<ModuleDetails*> m_pendingModules; + // Detached view model instances tracked for end-of-frame advance, keyed by + // instance pointer. Each entry keeps a strong reference alive and counts + // how many live owners registered it; the entry is erased when the count + // returns to zero. + struct TrackedViewModelInstance + { + rcp<ViewModelInstance> instance; + int registrations = 0; + }; + std::unordered_map<ViewModelInstance*, TrackedViewModelInstance> + m_trackedViewModelInstances; + #ifdef WITH_RIVE_TOOLS // Editor-only: Map from asset ID to generator function ref. // Allows direct ScriptedObject reinitialization without regenerating
diff --git a/include/rive/viewmodel/viewmodel_instance.hpp b/include/rive/viewmodel/viewmodel_instance.hpp index cd8412c..323af89 100644 --- a/include/rive/viewmodel/viewmodel_instance.hpp +++ b/include/rive/viewmodel/viewmodel_instance.hpp
@@ -61,6 +61,7 @@ void advanced(); void addParent(ViewModelInstance*); void removeParent(ViewModelInstance*); + bool hasParents() const { return !m_parents.empty(); } void addDependent(DataBindContainer*); void removeDependent(DataBindContainer*); #ifdef TESTING
diff --git a/src/animation/state_machine_instance.cpp b/src/animation/state_machine_instance.cpp index e1ed97d..786d710 100644 --- a/src/animation/state_machine_instance.cpp +++ b/src/animation/state_machine_instance.cpp
@@ -2573,6 +2573,12 @@ bool StateMachineInstance::advanceAndApply(float seconds) { + return advanceAndApply(seconds, true); +} + +bool StateMachineInstance::advanceAndApply(float seconds, + bool advanceViewModels) +{ RIVE_PROF_SCOPE_L(1) // Advancing by 0 could return false, when it shouldn't. Force keepGoing // to true. @@ -2607,13 +2613,26 @@ { keepGoing = true; } - reset(); + if (advanceViewModels) + { + reset(); // advancedDataContext() (VM consume) + artboard reset + } + else + { + m_artboardInstance->reset(); // artboard component reset only + } if (!m_artboardInstance->hasDirt(ComponentDirt::Components)) { break; } } + if (advanceViewModels) + { + // Advance detached scripted view models (created via scripts, not part + // of the bound view model tree) at the end of the frame. + m_artboardInstance->advanceScriptedViewModels(); + } return keepGoing || !m_reportedEvents.empty() || !m_reportedListenerViewModels.empty(); }
diff --git a/src/artboard.cpp b/src/artboard.cpp index 5ed982f..0c14798 100644 --- a/src/artboard.cpp +++ b/src/artboard.cpp
@@ -908,6 +908,19 @@ internalDrawCanvases(); } +void Artboard::advanceScriptedViewModels() +{ +#ifdef WITH_RIVE_SCRIPTING + if (m_scriptingVM != nullptr) + { + if (auto* context = m_scriptingVM->context()) + { + context->advanceDetachedViewModels(); + } + } +#endif +} + void Artboard::internalDrawCanvases() { for (auto obj : m_ScriptedObjects)
diff --git a/src/lua/lua_artboards.cpp b/src/lua/lua_artboards.cpp index 63b83d7..9d20211 100644 --- a/src/lua/lua_artboards.cpp +++ b/src/lua/lua_artboards.cpp
@@ -21,10 +21,12 @@ File* file, std::unique_ptr<ArtboardInstance>&& artboardInstance, rcp<ViewModelInstance> viewModelInstance, - rcp<DataContext> parentDataContext) : + rcp<DataContext> parentDataContext, + ScriptingContext* scriptingContext) : m_file(file), m_artboard(std::move(artboardInstance)), - m_stateMachine(m_artboard->defaultStateMachine()) + m_stateMachine(m_artboard->defaultStateMachine()), + m_scriptingContext(scriptingContext) { if (viewModelInstance) { @@ -47,10 +49,21 @@ m_stateMachine->bindViewModelInstance(m_viewModelInstance); } } + // Keep the bound instance tracked for end-of-frame advance for as long as + // this artboard uses it — even if the script drops its ScriptedViewModel + // wrapper. Covers both the passed-in and the auto-created instance. + if (m_scriptingContext != nullptr) + { + m_scriptingContext->trackViewModelInstance(m_viewModelInstance); + } } ScriptReffedArtboard::~ScriptReffedArtboard() { + if (m_scriptingContext != nullptr) + { + m_scriptingContext->untrackViewModelInstance(m_viewModelInstance.get()); + } // Make sure state machine is deleted before artboard since // StateMachineInstance destructor accesses the artboard. m_stateMachine = nullptr; @@ -92,7 +105,9 @@ auto machine = stateMachine(); if (machine) { - return machine->advanceAndApply(seconds); + // A scripted artboard's view models are advanced/reset by the host + // frame, not by this script-driven advance, so skip the VM consume. + return machine->advanceAndApply(seconds, false); } else { @@ -471,11 +486,12 @@ rcp<ViewModelInstance> viewModelInstance, rcp<DataContext> dataContext) : m_state(L), - m_scriptReffedArtboard( - make_rcp<ScriptReffedArtboard>(file, - std::move(artboardInstance), - viewModelInstance, - dataContext)), + m_scriptReffedArtboard(make_rcp<ScriptReffedArtboard>( + file, + std::move(artboardInstance), + viewModelInstance, + dataContext, + static_cast<ScriptingContext*>(lua_getthreaddata(L)))), m_dataContext(dataContext) {}
diff --git a/src/lua/lua_properties.cpp b/src/lua/lua_properties.cpp index 1cf30b0..3d1a637 100644 --- a/src/lua/lua_properties.cpp +++ b/src/lua/lua_properties.cpp
@@ -357,10 +357,25 @@ rcp<ViewModel> viewModel, rcp<ViewModelInstance> viewModelInstance) : m_state(L), m_viewModel(viewModel), m_viewModelInstance(viewModelInstance) -{} +{ + // Register the instance so detached ones (no parents) get advanced at the + // end of each frame. Tracking is keyed to owner lifetime, so the instance + // stays tracked while any owner (this wrapper, a scripted artboard) is + // alive. Store the context so unregistration in the destructor does not + // depend on lua_getthreaddata during Lua finalization. + m_scriptingContext = scriptingContext(L); + if (m_scriptingContext != nullptr) + { + m_scriptingContext->trackViewModelInstance(m_viewModelInstance); + } +} ScriptedViewModel::~ScriptedViewModel() { + if (m_scriptingContext != nullptr) + { + m_scriptingContext->untrackViewModelInstance(m_viewModelInstance.get()); + } for (auto itr : m_propertyRefs) { lua_unref(m_state, itr.second);
diff --git a/src/lua/rive_lua_libs.cpp b/src/lua/rive_lua_libs.cpp index b7356b0..78b12eb 100644 --- a/src/lua/rive_lua_libs.cpp +++ b/src/lua/rive_lua_libs.cpp
@@ -1,6 +1,7 @@ #ifdef WITH_RIVE_SCRIPTING #include "rive/lua/rive_lua_libs.hpp" #include "rive/assets/script_asset.hpp" +#include "rive/viewmodel/viewmodel_instance.hpp" #include "rive/async/work_pool.hpp" #ifdef RIVE_CANVAS #include "rive/renderer/render_context.hpp" @@ -981,6 +982,45 @@ } #endif +void ScriptingContext::trackViewModelInstance(rcp<ViewModelInstance> instance) +{ + if (instance == nullptr) + { + return; + } + auto& tracked = m_trackedViewModelInstances[instance.get()]; + tracked.instance = instance; + tracked.registrations++; +} + +void ScriptingContext::untrackViewModelInstance(ViewModelInstance* instance) +{ + if (instance == nullptr) + { + return; + } + auto it = m_trackedViewModelInstances.find(instance); + if (it != m_trackedViewModelInstances.end() && + --it->second.registrations <= 0) + { + m_trackedViewModelInstances.erase(it); + } +} + +void ScriptingContext::advanceDetachedViewModels() +{ + for (auto& entry : m_trackedViewModelInstances) + { + ViewModelInstance* instance = entry.second.instance.get(); + // Only advance detached roots. Instances with parents are already + // reached through the bound tree or their detached-root ancestor. + if (!instance->hasParents()) + { + instance->advanced(); + } + } +} + // ── WorkPool integration ─────────────────────────────────────────────────── // getGlobalWorkPool() is defined in work_pool.cpp (shared singleton).
diff --git a/tests/unit_tests/assets/reset_shared_viewmodel_instance_test.riv b/tests/unit_tests/assets/reset_shared_viewmodel_instance_test.riv new file mode 100644 index 0000000..fcd45ec --- /dev/null +++ b/tests/unit_tests/assets/reset_shared_viewmodel_instance_test.riv Binary files differ
diff --git a/tests/unit_tests/runtime/data_binding_test.cpp b/tests/unit_tests/runtime/data_binding_test.cpp index 8fe33b3..b4deb63 100644 --- a/tests/unit_tests/runtime/data_binding_test.cpp +++ b/tests/unit_tests/runtime/data_binding_test.cpp
@@ -180,6 +180,44 @@ REQUIRE(shapeMapped->y() == 350); } +// advanceAndApply(secs, advanceViewModels=false) runs the state machine but +// must NOT consume the bound view model instances. This is what +// ScriptedArtboard::advance uses so a script-driven nested artboard doesn't +// reset view models owned by the host frame. +TEST_CASE("advanceAndApply can skip view model reset", "[data binding]") +{ + auto file = ReadRiveFile("assets/data_binding_test.riv"); + + auto artboard = file->artboard("artboard-2")->instance(); + REQUIRE(artboard != nullptr); + auto viewModelInstance = + file->createDefaultViewModelInstance(artboard.get()); + REQUIRE(viewModelInstance != nullptr); + auto machine = artboard->defaultStateMachine(); + REQUIRE(machine != nullptr); + machine->bindViewModelInstance(viewModelInstance); + + auto triggerProperty = viewModelInstance->propertyValue("trigger-prop"); + REQUIRE(triggerProperty != nullptr); + REQUIRE(triggerProperty->is<rive::ViewModelInstanceTrigger>()); + auto trigger = triggerProperty->as<rive::ViewModelInstanceTrigger>(); + + // Settle initial state. + machine->advanceAndApply(0.0f); + + // advanceViewModels=false: the bound view model is not consumed, so a + // trigger set before the advance is retained (the host frame will consume + // it, not this advance). + trigger->propertyValue(1); + machine->advanceAndApply(0.0f, false); + CHECK(trigger->propertyValue() == 1); + + // The default (advanceViewModels=true) path consumes the trigger, resetting + // it to 0 via ViewModelInstance::advanced(). + machine->advanceAndApply(0.0f, true); + CHECK(trigger->propertyValue() == 0); +} + TEST_CASE("calculate and to string converters with numbers", "[data binding]") { auto file = ReadRiveFile("assets/data_binding_test.riv");
diff --git a/tests/unit_tests/runtime/scripting/scripting_detached_viewmodel_advance_test.cpp b/tests/unit_tests/runtime/scripting/scripting_detached_viewmodel_advance_test.cpp new file mode 100644 index 0000000..07b6590 --- /dev/null +++ b/tests/unit_tests/runtime/scripting/scripting_detached_viewmodel_advance_test.cpp
@@ -0,0 +1,131 @@ +#include <catch.hpp> +#include "scripting_test_utilities.hpp" +#include "rive/viewmodel/viewmodel_instance.hpp" +#include "rive/viewmodel/viewmodel_instance_trigger.hpp" +#include "rive/viewmodel/viewmodel_instance_viewmodel.hpp" + +using namespace rive; + +namespace +{ +// Builds a ViewModelInstance carrying a single trigger property. The trigger is +// returned via `outTrigger` so the test can set/read its value; the instance +// owns it. A triggered value is reset to 0 by ViewModelInstance::advanced(). +rcp<ViewModelInstance> makeInstanceWithTrigger( + ViewModelInstanceTrigger** outTrigger) +{ + auto instance = make_rcp<ViewModelInstance>(); + auto* trigger = new ViewModelInstanceTrigger(); + instance->addValue(trigger); + *outTrigger = trigger; + return instance; +} +} // namespace + +// A ScriptedViewModel whose instance is detached from the bound tree (no +// parents) must have its instance advanced by advanceDetachedViewModels(), +// while one whose instance has a parent is skipped (it is already advanced +// through the tree). Advancing consumes the trigger (resets it to 0), so we +// assert on that. +TEST_CASE("advanceDetachedViewModels advances only parentless instances", + "[scripting]") +{ + ScriptingTest test("return 1"); + auto* context = test.vm()->context(); + auto* L = test.state(); + + // Detached: no parents. + ViewModelInstanceTrigger* detachedTrigger = nullptr; + auto detached = makeInstanceWithTrigger(&detachedTrigger); + + // Attached: referenced by a parent, which calls addParent under the hood. + ViewModelInstanceTrigger* attachedTrigger = nullptr; + auto attached = makeInstanceWithTrigger(&attachedTrigger); + auto parent = make_rcp<ViewModelInstance>(); + auto* vmProperty = new ViewModelInstanceViewModel(); + vmProperty->parentViewModelInstance(parent.get()); + vmProperty->referenceViewModelInstance(attached); + parent->addValue(vmProperty); + + REQUIRE_FALSE(detached->hasParents()); + REQUIRE(attached->hasParents()); + + { + // Constructing a ScriptedViewModel registers it with the context via + // the lua thread data; destruction unregisters it. + ScriptedViewModel scriptedDetached(L, nullptr, detached); + ScriptedViewModel scriptedAttached(L, nullptr, attached); + + detachedTrigger->trigger(); + attachedTrigger->trigger(); + REQUIRE(detachedTrigger->propertyValue() == 1); + REQUIRE(attachedTrigger->propertyValue() == 1); + + context->advanceDetachedViewModels(); + + // Detached root advanced -> trigger consumed. + CHECK(detachedTrigger->propertyValue() == 0); + // Parented instance skipped -> trigger retained (it is advanced through + // the bound tree instead). + CHECK(attachedTrigger->propertyValue() == 1); + } + + // Both scripted view models are now destroyed and untracked; advancing must + // not touch dangling pointers. + context->advanceDetachedViewModels(); +} + +// A ScriptedViewModel that wraps no instance (property-less) registers nothing +// and must be handled safely during advance. +TEST_CASE("advanceDetachedViewModels tolerates null instances", "[scripting]") +{ + ScriptingTest test("return 1"); + auto* context = test.vm()->context(); + auto* L = test.state(); + + ScriptedViewModel scriptedNull(L, nullptr, nullptr); + context->advanceDetachedViewModels(); +} + +// The core lifetime fix: tracking is keyed to owner lifetime, not to the Lua +// wrapper's GC. An instance owned by more than one registrant (e.g. a scripted +// artboard AND a ScriptedViewModel wrapper) must stay tracked — and keep being +// advanced — until the LAST owner goes away, even if the script drops its +// wrapper first. +TEST_CASE("tracked instance outlives its wrapper while another owner holds it", + "[scripting]") +{ + ScriptingTest test("return 1"); + auto* context = test.vm()->context(); + auto* L = test.state(); + + ViewModelInstanceTrigger* trigger = nullptr; + auto instance = makeInstanceWithTrigger(&trigger); + REQUIRE_FALSE(instance->hasParents()); + + // A second owner (simulating a ScriptReffedArtboard) registers the instance + // directly, independent of any Lua wrapper. + context->trackViewModelInstance(instance); + + { + // A ScriptedViewModel wrapper is a second registrant (registrations 2). + ScriptedViewModel wrapper(L, nullptr, instance); + trigger->trigger(); + context->advanceDetachedViewModels(); + CHECK(trigger->propertyValue() == 0); // advanced + } + + // Wrapper destroyed (registrations 2 -> 1). The instance is still owned by + // the simulated artboard, so it must STILL be advanced. This is the bug the + // fix addresses: previously it would have been untracked here. + trigger->trigger(); + context->advanceDetachedViewModels(); + CHECK(trigger->propertyValue() == 0); + + // Last owner releases it (registrations 1 -> 0): no longer tracked, so it + // is not advanced (and nothing dangles). + context->untrackViewModelInstance(instance.get()); + trigger->trigger(); + context->advanceDetachedViewModels(); + CHECK(trigger->propertyValue() == 1); +}
diff --git a/tests/unit_tests/runtime/scripting/scripting_properties_test.cpp b/tests/unit_tests/runtime/scripting/scripting_properties_test.cpp index 4c09919..22bb278 100644 --- a/tests/unit_tests/runtime/scripting/scripting_properties_test.cpp +++ b/tests/unit_tests/runtime/scripting/scripting_properties_test.cpp
@@ -842,4 +842,53 @@ artboard->draw(renderer.get()); CHECK(silver.matches("image_scripting_property_value")); +} + +TEST_CASE("Reset detached view model instances at end of frame", "[silver]") +{ + SerializingFactory silver; + auto file = ReadRiveFile("assets/reset_shared_viewmodel_instance_test.riv", + &silver); + + auto artboard = file->artboardDefault(); + REQUIRE(artboard != nullptr); + + silver.frameSize(artboard->width(), artboard->height()); + + auto stateMachine = artboard->stateMachineAt(0); + + auto vmi = file->createDefaultViewModelInstance(artboard.get()); + auto valueProp = vmi->propertyValue("tri1")->as<ViewModelInstanceTrigger>(); + + auto renderer = silver.makeRenderer(); + stateMachine->bindViewModelInstance(vmi); + stateMachine->advanceAndApply(0.0f); + artboard->draw(renderer.get()); + silver.addFrame(); + stateMachine->advanceAndApply(0.016f); + artboard->draw(renderer.get()); + + silver.addFrame(); + valueProp->trigger(); + stateMachine->advanceAndApply(0.016f); + artboard->draw(renderer.get()); + + silver.addFrame(); + valueProp->trigger(); + stateMachine->pointerDown(rive::Vec2D(45.0f, 165.0f)); + stateMachine->pointerUp(rive::Vec2D(45.0f, 165.0f)); + stateMachine->advanceAndApply(0.016f); + artboard->draw(renderer.get()); + + silver.addFrame(); + stateMachine->advanceAndApply(0.016f); + artboard->draw(renderer.get()); + + silver.addFrame(); + stateMachine->pointerDown(rive::Vec2D(45.0f, 165.0f)); + stateMachine->pointerUp(rive::Vec2D(45.0f, 165.0f)); + stateMachine->advanceAndApply(0.016f); + artboard->draw(renderer.get()); + + CHECK(silver.matches("reset_shared_viewmodel_instance_test")); } \ No newline at end of file
diff --git a/tests/unit_tests/silvers/reset_shared_viewmodel_instance_test.sriv b/tests/unit_tests/silvers/reset_shared_viewmodel_instance_test.sriv new file mode 100644 index 0000000..e844cd9 --- /dev/null +++ b/tests/unit_tests/silvers/reset_shared_viewmodel_instance_test.sriv Binary files differ