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 12a1caee221..d22b9f0ad65 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),
@@ -506,15 +515,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 379dd52df5b..d4f50c1b345 100644
--- a/source/gameengine/Ketsji/BL_Action.h
+++ b/source/gameengine/Ketsji/BL_Action.h
@@ -105,10 +105,6 @@ public:
 	 * Update the action's frame, etc.
 	 */
 	void Update(float curtime);
-	/**
-	 * Update object IPOs (note: not thread-safe!)
-	 */
-	void UpdateIPOs();
 
 	// Accessors
 	float GetFrame();
@@ -144,6 +140,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 9e4690548d3..3aefb3650c7 100644
--- a/source/gameengine/Ketsji/BL_ActionManager.cpp
+++ b/source/gameengine/Ketsji/BL_ActionManager.cpp
@@ -170,13 +170,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 97d6d88cf22..599ee28a298 100644
--- a/source/gameengine/Ketsji/BL_ActionManager.h
+++ b/source/gameengine/Ketsji/BL_ActionManager.h
@@ -129,11 +129,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 c3da80bc14f..e7ba5ea1124 100644
--- a/source/gameengine/Ketsji/KX_GameObject.cpp
+++ b/source/gameengine/Ketsji/KX_GameObject.cpp
@@ -484,11 +484,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);
@@ -952,6 +947,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 c10802a83b2..a5b6d12f252 100644
--- a/source/gameengine/Ketsji/KX_GameObject.h
+++ b/source/gameengine/Ketsji/KX_GameObject.h
@@ -321,12 +321,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 723736906a9..56707bfb218 100644
--- a/source/gameengine/Ketsji/KX_Scene.cpp
+++ b/source/gameengine/Ketsji/KX_Scene.cpp
@@ -1678,10 +1678,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)