BGE: Fix T46381 : last action frame not updated.

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

8 files changed, 36 insertions, 39 deletions

diff --git a/source/gameengine/Ketsji/BL_Action.cpp b/source/gameengine/Ketsji/BL_Action.cpp
index 507476dbd0e..4d5b6a0e139 100644
--- a/source/gameengine/Ketsji/BL_Action.cpp
+++ b/source/gameengine/Ketsji/BL_Action.cpp
@@ -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);
 }
diff --git a/source/gameengine/Ketsji/BL_Action.h b/source/gameengine/Ketsji/BL_Action.h
index 7a404416758..f6124b032aa 100644
--- a/source/gameengine/Ketsji/BL_Action.h
+++ b/source/gameengine/Ketsji/BL_Action.h
@@ -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
diff --git a/source/gameengine/Ketsji/BL_ActionManager.cpp b/source/gameengine/Ketsji/BL_ActionManager.cpp
index 4249db55b45..491be035d66 100644
--- a/source/gameengine/Ketsji/BL_ActionManager.cpp
+++ b/source/gameengine/Ketsji/BL_ActionManager.cpp
@@ -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();
-	}
-}
diff --git a/source/gameengine/Ketsji/BL_ActionManager.h b/source/gameengine/Ketsji/BL_ActionManager.h
index 1292938d8f4..69c6d611df0 100644
--- a/source/gameengine/Ketsji/BL_ActionManager.h
+++ b/source/gameengine/Ketsji/BL_ActionManager.h
@@ -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
diff --git a/source/gameengine/Ketsji/KX_GameObject.cpp b/source/gameengine/Ketsji/KX_GameObject.cpp
index d7a94f0601c..1dbcf14af89 100644
--- a/source/gameengine/Ketsji/KX_GameObject.cpp
+++ b/source/gameengine/Ketsji/KX_GameObject.cpp
@@ -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);
diff --git a/source/gameengine/Ketsji/KX_GameObject.h b/source/gameengine/Ketsji/KX_GameObject.h
index abd109a5e52..c2c455dab6a 100644
--- a/source/gameengine/Ketsji/KX_GameObject.h
+++ b/source/gameengine/Ketsji/KX_GameObject.h
@@ -327,12 +327,6 @@ public:
 	 */
 	void UpdateActionManager(float curtime);
 
-	/**
-	 * Have the action manager update IPOs
-	 * note: not thread-safe!
-	 */
-	void UpdateActionIPOs();
-
 	/*********************************
 	 * End Animation API
 	 *********************************/
diff --git a/source/gameengine/Ketsji/KX_KetsjiEngine.cpp b/source/gameengine/Ketsji/KX_KetsjiEngine.cpp
index 9d6fae6f555..c6e5f734c16 100644
--- a/source/gameengine/Ketsji/KX_KetsjiEngine.cpp
+++ b/source/gameengine/Ketsji/KX_KetsjiEngine.cpp
@@ -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();
 }
 
 
diff --git a/source/gameengine/Ketsji/KX_Scene.cpp b/source/gameengine/Ketsji/KX_Scene.cpp
index c9c63371713..16d1fdd6ea2 100644
--- a/source/gameengine/Ketsji/KX_Scene.cpp
+++ b/source/gameengine/Ketsji/KX_Scene.cpp
@@ -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)