BGE: Fix T46381 : last action frame not updated.
authorPorteries Tristan <republicthunderbolt9@gmail.com>
Mon, 19 Oct 2015 14:03:40 +0000 (16:03 +0200)
committerPorteries Tristan <republicthunderbolt9@gmail.com>
Mon, 19 Oct 2015 14:03:40 +0000 (16:03 +0200)
It fix T46381. Normally BL_Action::Update (manage action time, end, loop…) should be called the same number of times as BL_Action::UpdateIPO (update action position, scale ect… in the game object).
But the bug report shows that UpdateIPO is called one less time than Update. To fix it i revert the commit 362b25b38287cb75e4d22b30bdbc7f47e8eb3fdf and implement a mutex in BL_Action::Update.
Example file : {F245823}

Reviewers: lordloki, kupoman, campbellbarton, youle, moguri, sybren

Reviewed By: youle, moguri, sybren

Maniphest Tasks: T39928, T46381

Differential Revision: https://developer.blender.org/D1562

source/gameengine/Ketsji/BL_Action.cpp
source/gameengine/Ketsji/BL_Action.h
source/gameengine/Ketsji/BL_ActionManager.cpp
source/gameengine/Ketsji/BL_ActionManager.h
source/gameengine/Ketsji/KX_GameObject.cpp
source/gameengine/Ketsji/KX_GameObject.h
source/gameengine/Ketsji/KX_KetsjiEngine.cpp
source/gameengine/Ketsji/KX_Scene.cpp

index 507476dbd0eb9a2afde958da196f95d0f5cbd568..4d5b6a0e139fa7c1b9bdb90c0fff8ec41eb26c3d 100644 (file)
@@ -56,6 +56,15 @@ extern "C" {
 #include "BKE_library.h"
 #include "BKE_global.h"
 
+#include "BLI_threads.h" // for lock
+
+/* Lock to solve animation thread issues.
+ * A spin lock is better than a mutex in case of short wait 
+ * because spin lock stop the thread by a loop contrary to mutex
+ * which switch all memory, process.
+ */ 
+static SpinLock BL_ActionLock;
+
 BL_Action::BL_Action(class KX_GameObject* gameobj)
 :
        m_action(NULL),
@@ -501,15 +510,23 @@ void BL_Action::Update(float curtime)
                }
        }
 
-       // This isn't thread-safe, so we move it into it's own function for now
-       //m_obj->UpdateIPO(m_localtime, m_ipo_flags & ACT_IPOFLAG_CHILD);
+       BLI_spin_lock(&BL_ActionLock);
+       /* This function is not thread safe because of recursive scene graph transform
+        * updates on children. e.g: If an object and one of its children is animated,
+        * the both can write transform at the same time. A thread lock avoid problems. */
+       m_obj->UpdateIPO(m_localtime, m_ipo_flags & ACT_IPOFLAG_CHILD);
+       BLI_spin_unlock(&BL_ActionLock);
 
        if (m_done)
                ClearControllerList();
 }
 
-void BL_Action::UpdateIPOs()
+void BL_Action::InitLock()
+{
+       BLI_spin_init(&BL_ActionLock);
+}
+
+void BL_Action::EndLock()
 {
-       if (!m_done)
-               m_obj->UpdateIPO(m_localtime, m_ipo_flags & ACT_IPOFLAG_CHILD);
+       BLI_spin_end(&BL_ActionLock);
 }
index 7a404416758767587cec7f779f820dfc1a555b93..f6124b032aa321ebe8a05183cabde1046b5194d1 100644 (file)
@@ -101,10 +101,6 @@ public:
         * Update the action's frame, etc.
         */
        void Update(float curtime);
-       /**
-        * Update object IPOs (note: not thread-safe!)
-        */
-       void UpdateIPOs();
 
        // Accessors
        float GetFrame();
@@ -140,6 +136,11 @@ public:
                ACT_IPOFLAG_CHILD = 8,
        };
 
+       /// Initialize a lock for animation thread issues.
+       static void InitLock();
+       /// Finalize a lock for animation thread issues.
+       static void EndLock();
+
 #ifdef WITH_CXX_GUARDEDALLOC
        MEM_CXX_CLASS_ALLOC_FUNCS("GE:BL_Action")
 #endif
index 4249db55b4515dff695b0a5ca28eeb8717982ef2..491be035d66e8361b6b4aa32f65bebb9372953ae 100644 (file)
@@ -162,13 +162,3 @@ void BL_ActionManager::Update(float curtime)
                }
        }
 }
-
-void BL_ActionManager::UpdateIPOs()
-{
-       BL_ActionMap::iterator it;
-       for (it = m_layers.begin(); it != m_layers.end(); ++it)
-       {
-               if (!it->second->IsDone())
-                       it->second->UpdateIPOs();
-       }
-}
index 1292938d8f4182f64612f41701612d7eaeb95f1f..69c6d611df0e84662a081c6f9f237f448321e5a7 100644 (file)
@@ -124,11 +124,6 @@ public:
         */
        void Update(float);
 
-       /**
-        * Update object IPOs (note: not thread-safe!)
-        */
-       void UpdateIPOs();
-
 #ifdef WITH_CXX_GUARDEDALLOC
        MEM_CXX_CLASS_ALLOC_FUNCS("GE:BL_ActionManager")
 #endif
index d7a94f0601c55859a306a3e5a8a5a67e0f619855..1dbcf14af89d66e7e527b656322ba0d2bcb0a7f3 100644 (file)
@@ -481,11 +481,6 @@ void KX_GameObject::UpdateActionManager(float curtime)
        GetActionManager()->Update(curtime);
 }
 
-void KX_GameObject::UpdateActionIPOs()
-{
-       GetActionManager()->UpdateIPOs();
-}
-
 float KX_GameObject::GetActionFrame(short layer)
 {
        return GetActionManager()->GetActionFrame(layer);
@@ -949,6 +944,9 @@ void KX_GameObject::InitIPO(bool ipo_as_force,
 void KX_GameObject::UpdateIPO(float curframetime,
                                                          bool recurse) 
 {
+       /* This function shouldn't call BL_Action::Update, not even indirectly, 
+        * as it will cause deadlock due to the lock in BL_Action::Update. */
+
        // just the 'normal' update procedure.
        GetSGNode()->SetSimulatedTime(curframetime,recurse);
        GetSGNode()->UpdateWorldData(curframetime);
index abd109a5e52aa68bc8070d3d2050c72fd2769e55..c2c455dab6a0d321761bb24e0fbd3e73b4b1714a 100644 (file)
@@ -327,12 +327,6 @@ public:
         */
        void UpdateActionManager(float curtime);
 
-       /**
-        * Have the action manager update IPOs
-        * note: not thread-safe!
-        */
-       void UpdateActionIPOs();
-
        /*********************************
         * End Animation API
         *********************************/
index 9d6fae6f555857afbb93d33ca4ec5b6c1092bf0d..c6e5f734c163d6651a3ccd83d6fe1a40b1cf970b 100644 (file)
@@ -75,6 +75,8 @@
 
 #include "KX_NavMeshObject.h"
 
+#include "BL_Action.h" // For managing action lock.
+
 #define DEFAULT_LOGIC_TIC_RATE 60.0
 //#define DEFAULT_PHYSICS_TIC_RATE 60.0
 
@@ -181,6 +183,8 @@ KX_KetsjiEngine::KX_KetsjiEngine(KX_ISystem* system)
 #endif
 
        m_taskscheduler = BLI_task_scheduler_create(TASK_SCHEDULER_AUTO_THREADS);
+
+       BL_Action::InitLock();
 }
 
 
@@ -200,6 +204,8 @@ KX_KetsjiEngine::~KX_KetsjiEngine()
 
        if (m_taskscheduler)
                BLI_task_scheduler_free(m_taskscheduler);
+
+       BL_Action::EndLock();
 }
 
 
index c9c63371713d138154d3a62f9badd92d4bf1bebd..16d1fdd6ea2a04233602829107dc2982e6fd9048 100644 (file)
@@ -1689,10 +1689,6 @@ void KX_Scene::UpdateAnimations(double curtime)
 
        BLI_task_pool_work_and_wait(pool);
        BLI_task_pool_free(pool);
-
-       for (int i=0; i<m_animatedlist->GetCount(); ++i) {
-               ((KX_GameObject*)m_animatedlist->GetValue(i))->UpdateActionIPOs();
-       }
 }
 
 void KX_Scene::LogicUpdateFrame(double curtime, bool frame)