Fix crash in animations system when assets gets loading/unloaded while async jobs are active

#2974

Source/Engine/Animations/Animations.cpp

@@ -27,6 +27,7 @@ class AnimationsSystem : public TaskGraphSystem
 {
 public:
     float DeltaTime, UnscaledDeltaTime, Time, UnscaledTime;
+    bool Active;
 
     void Job(int32 index);
     void Execute(TaskGraph* graph) override;
@@ -51,6 +52,7 @@ namespace
 
 AnimationsService AnimationManagerInstance;
 TaskGraphSystem* Animations::System = nullptr;
+ConcurrentSystemLocker Animations::SystemLocker;
 #if USE_EDITOR
 Delegate<Animations::DebugFlowInfo> Animations::DebugFlow;
 #endif
@@ -116,6 +118,10 @@ void AnimationsSystem::Execute(TaskGraph* graph)
 {
     if (AnimationManagerInstance.UpdateList.Count() == 0)
         return;
+    Active = true;
+
+    // Ensure no animation assets it being reloading/modified before running async update
+    Animations::SystemLocker.Begin(false);
 
     // Setup data for async update
     const auto& tickData = Time::Update;
@@ -138,6 +144,8 @@ void AnimationsSystem::Execute(TaskGraph* graph)
 
 void AnimationsSystem::PostExecute(TaskGraph* graph)
 {
+    if (!Active)
+        return;
     PROFILE_CPU_NAMED("Animations.PostExecute");
 
     // Update gameplay
@@ -153,6 +161,8 @@ void AnimationsSystem::PostExecute(TaskGraph* graph)
 
     // Cleanup
     AnimationManagerInstance.UpdateList.Clear();
+    Animations::SystemLocker.End(false);
+    Active = false;
 }
 
 void Animations::AddToUpdate(AnimatedModel* obj)

Source/Engine/Animations/Animations.h

@@ -4,6 +4,7 @@
 
 #include "Engine/Scripting/ScriptingType.h"
 #include "Engine/Core/Delegate.h"
+#include "Engine/Threading/ConcurrentSystemLocker.h"
 
 class TaskGraphSystem;
 class AnimatedModel;
@@ -21,6 +22,9 @@ API_CLASS(Static) class FLAXENGINE_API Animations
     /// </summary>
     API_FIELD(ReadOnly) static TaskGraphSystem* System;
 
+    // Data access locker for animations data.
+    static ConcurrentSystemLocker SystemLocker;
+
 #if USE_EDITOR
     // Data wrapper for the debug flow information.
     API_STRUCT(NoDefault) struct DebugFlowInfo

Source/Engine/Content/Assets/Animation.cpp

@@ -6,6 +6,7 @@
 #include "Engine/Content/Factories/BinaryAssetFactory.h"
 #include "Engine/Animations/CurveSerialization.h"
 #include "Engine/Animations/AnimEvent.h"
+#include "Engine/Animations/Animations.h"
 #include "Engine/Animations/SceneAnimations/SceneAnimation.h"
 #include "Engine/Scripting/Scripting.h"
 #include "Engine/Threading/Threading.h"
@@ -407,7 +408,6 @@ bool Animation::Save(const StringView& path)
         LOG(Error, "Asset loading failed. Cannot save it.");
         return true;
     }
-
     ScopeLock lock(Locker);
 
     // Serialize animation data to the stream
@@ -552,6 +552,8 @@ void Animation::OnScriptingDispose()
 
 Asset::LoadResult Animation::load()
 {
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
+
     // Get stream with animations data
     const auto dataChunk = GetChunk(0);
     if (dataChunk == nullptr)
@@ -682,6 +684,7 @@ Asset::LoadResult Animation::load()
 
 void Animation::unload(bool isReloading)
 {
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
 #if USE_EDITOR
     if (_registeredForScriptingReload)
     {

Source/Engine/Content/Assets/AnimationGraph.cpp

@@ -10,6 +10,7 @@
 #include "Engine/Serialization/MemoryReadStream.h"
 #include "Engine/Serialization/MemoryWriteStream.h"
 #include "Engine/Content/Factories/BinaryAssetFactory.h"
+#include "Engine/Animations/Animations.h"
 #include "Engine/Threading/Threading.h"
 #include "Engine/Debug/Exceptions/ArgumentNullException.h"
 
@@ -24,6 +25,8 @@ AnimationGraph::AnimationGraph(const SpawnParams& params, const AssetInfo* info)
 
 Asset::LoadResult AnimationGraph::load()
 {
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
+
     // Get stream with graph data
     const auto surfaceChunk = GetChunk(0);
     if (surfaceChunk == nullptr)
@@ -48,6 +51,7 @@ Asset::LoadResult AnimationGraph::load()
 
 void AnimationGraph::unload(bool isReloading)
 {
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
     Graph.Clear();
 }
 
@@ -79,6 +83,7 @@ bool AnimationGraph::InitAsAnimation(SkinnedModel* baseModel, Animation* anim, b
         Log::ArgumentNullException();
         return true;
     }
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
 
     // Create Graph data
     MemoryWriteStream writeStream(512);
@@ -172,7 +177,7 @@ bool AnimationGraph::SaveSurface(BytesContainer& data)
         LOG(Error, "Asset loading failed. Cannot save it.");
         return true;
     }
-
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
     ScopeLock lock(Locker);
 
     if (IsVirtual())

Source/Engine/Content/Assets/AnimationGraphFunction.cpp

@@ -4,6 +4,7 @@
 #include "Engine/Core/Log.h"
 #include "Engine/Core/Types/DataContainer.h"
 #include "Engine/Serialization/MemoryReadStream.h"
+#include "Engine/Animations/Animations.h"
 #include "Engine/Content/Factories/BinaryAssetFactory.h"
 #include "Engine/Threading/Threading.h"
 
@@ -16,6 +17,8 @@ AnimationGraphFunction::AnimationGraphFunction(const SpawnParams& params, const
 
 Asset::LoadResult AnimationGraphFunction::load()
 {
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
+
     // Get graph data from chunk
     const auto surfaceChunk = GetChunk(0);
     if (!surfaceChunk || !surfaceChunk->IsLoaded())
@@ -41,6 +44,7 @@ Asset::LoadResult AnimationGraphFunction::load()
 
 void AnimationGraphFunction::unload(bool isReloading)
 {
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
     GraphData.Release();
     Inputs.Clear();
     Outputs.Clear();
@@ -65,6 +69,7 @@ BytesContainer AnimationGraphFunction::LoadSurface() const
 
 void AnimationGraphFunction::GetSignature(Array<StringView, FixedAllocation<32>>& types, Array<StringView, FixedAllocation<32>>& names)
 {
+    ScopeLock lock(Locker);
     types.Resize(32);
     names.Resize(32);
     for (int32 i = 0, j = 0; i < Inputs.Count(); i++)
@@ -96,7 +101,7 @@ bool AnimationGraphFunction::SaveSurface(const BytesContainer& data)
         LOG(Error, "Asset loading failed. Cannot save it.");
         return true;
     }
-
+    ConcurrentSystemLocker::WriteScope systemScope(Animations::SystemLocker);
     ScopeLock lock(Locker);
 
     // Set Visject Surface data
@@ -120,6 +125,7 @@ bool AnimationGraphFunction::SaveSurface(const BytesContainer& data)
 
 void AnimationGraphFunction::ProcessGraphForSignature(AnimGraphBase* graph, bool canUseOutputs)
 {
+    ScopeLock lock(Locker);
     for (int32 i = 0; i < graph->Nodes.Count(); i++)
     {
         auto& node = graph->Nodes[i];

Source/Engine/Threading/ConcurrentSystemLocker.cpp

@@ -0,0 +1,41 @@
+// Copyright (c) 2012-2024 Wojciech Figat. All rights reserved.
+
+#include "ConcurrentSystemLocker.h"
+#include "Engine/Platform/Platform.h"
+
+ConcurrentSystemLocker::ConcurrentSystemLocker()
+{
+    _counters[0] = _counters[1] = 0;
+}
+
+void ConcurrentSystemLocker::Begin(bool write)
+{
+    volatile int64* thisCounter = &_counters[write];
+    volatile int64* otherCounter = &_counters[!write];
+RETRY:
+    // Check if we can enter (cannot read while someone else is writing and vice versa)
+    if (Platform::AtomicRead(otherCounter) != 0)
+    {
+        // Someone else is doing opposite operation so wait for it's end
+        // TODO: use ConditionVariable+CriticalSection to prevent active-waiting
+        Platform::Sleep(1);
+        goto RETRY;
+    }
+
+    // Mark that we entered this section
+    Platform::InterlockedIncrement(thisCounter);
+
+    // Double-check if we're safe to go
+    if (Platform::InterlockedCompareExchange(otherCounter, 0, 0))
+    {
+        // Someone else is doing opposite operation while this thread was doing counter increment so retry
+        Platform::InterlockedDecrement(thisCounter);
+        goto RETRY;
+    }
+}
+
+void ConcurrentSystemLocker::End(bool write)
+{
+    // Mark that we left this section
+    Platform::InterlockedDecrement(&_counters[write]);
+}

Source/Engine/Threading/ConcurrentSystemLocker.h

@@ -0,0 +1,46 @@
+// Copyright (c) 2012-2024 Wojciech Figat. All rights reserved.
+
+#pragma once
+
+#include "Engine/Core/Core.h"
+#include "Engine/Core/Types/BaseTypes.h"
+
+/// <summary>
+/// Utility for guarding system data access from different threads depending on the resources usage (eg. block read on write).
+/// </summary>
+struct ConcurrentSystemLocker
+{
+private:
+    volatile int64 _counters[2];
+
+public:
+    NON_COPYABLE(ConcurrentSystemLocker);
+    ConcurrentSystemLocker();
+
+    void Begin(bool write);
+    void End(bool write);
+
+public:
+    template<bool Write>
+    struct Scope
+    {
+        NON_COPYABLE(Scope);
+
+        Scope(ConcurrentSystemLocker& locker)
+            : _locker(locker)
+        {
+            _locker.Begin(Write);
+        }
+
+        ~Scope()
+        {
+            _locker.End(Write);
+        }
+
+    private:
+        ConcurrentSystemLocker& _locker;
+    };
+
+    typedef Scope<false> ReadScope;
+    typedef Scope<true> WriteScope;
+};