Fixing an issue with GameObject finalization - Objects that were finalized were still being retrievable from game object manager for a single frame

BansheeEngine/Include/BsGUISpace.h

@@ -26,7 +26,7 @@ namespace BansheeEngine
 		/**
 		 * @brief	Changes the size of the space to the specified value, in pixels.
 		 */
-		void setSize(UINT32 size) { mSize = size; _markLayoutAsDirty(); }
+		void setSize(UINT32 size) { if (mSize != size) { mSize = size; _markLayoutAsDirty(); } }
 
 		/**
 		 * @copydoc	GUIElementBase::_getType

BansheeEngine/Source/BsGUIElementBase.cpp

@@ -226,8 +226,11 @@ namespace BansheeEngine
 			return; // Cannot enable if parent is disabled
 
 		// Make sure to mark everything as dirty, as we didn't track any dirty flags while the element was disabled
-		mIsDisabled = false;
-		_markLayoutAsDirty();
+		if (mIsDisabled)
+		{
+			mIsDisabled = false;
+			_markLayoutAsDirty();
+		}
 
 		for(auto& elem : mChildren)
 		{
@@ -237,8 +240,11 @@ namespace BansheeEngine
 
 	void GUIElementBase::disableRecursively()
 	{
-		_markMeshAsDirty(); // Just need to hide the mesh
-		mIsDisabled = true;
+		if (!mIsDisabled)
+		{
+			_markLayoutAsDirty();
+			mIsDisabled = true;
+		}
 
 		for(auto& elem : mChildren)
 		{

SBansheeEngine/Include/BsScriptComponent.h

@@ -29,8 +29,7 @@ namespace BansheeEngine
 		friend class ScriptGameObjectManager;
 
 		ScriptComponent(MonoObject* instance);
-		~ScriptComponent() {}
-
+		
 		/**
 		 * @copydoc	ScriptObjectBase::beginRefresh
 		 */
@@ -46,6 +45,11 @@ namespace BansheeEngine
 		 */
 		virtual MonoObject* _createManagedInstance(bool construct) override;
 
+		/**
+		 * @copydoc	ScriptObjectBase::_onManagedInstanceFinalized
+		 */
+		void _onManagedInstanceFinalized() override;
+
 		/**
 		 * @copydoc	ScriptObjectBase::_onManagedInstanceDeleted
 		 */

SBansheeEngine/Include/BsScriptGameObject.h

@@ -35,7 +35,7 @@ namespace BansheeEngine
 		virtual void endRefresh(const ScriptObjectBackup& backupData) override;
 
 	protected:
-		bool mRefreshInProgress;
+		std::atomic<bool> mRefreshInProgress;
 	};
 
 	/**

SBansheeEngine/Include/BsScriptGameObjectManager.h

@@ -10,6 +10,8 @@ namespace BansheeEngine
 	 * @brief	Manages all active GameObject interop objects. GameObjects can be created from native
 	 *			code and used in managed code therefore we need to keep a dictionary or all the native
 	 *			objects we have mapped to managed objects.
+	 *
+	 * @note	Thread safe.
 	 */
 	class BS_SCR_BE_EXPORT ScriptGameObjectManager : public Module<ScriptGameObjectManager>
 	{
@@ -92,5 +94,9 @@ namespace BansheeEngine
 
 		UnorderedMap<UINT64, ScriptGameObjectEntry> mScriptGameObjects;
 		HEvent mOnAssemblyReloadDoneConn;
+
+		// Mutex needed as we need to be able unregister game objects as soon as they're finalized, and that happens on
+		// a separate finalizer thread. 
+		mutable Mutex mMutex;
 	};
 }

SBansheeEngine/Include/BsScriptObject.h

@@ -61,8 +61,14 @@ namespace BansheeEngine
 		virtual void _restoreManagedInstance() { }
 
 		/**
-		 * @brief	Called when the managed instance gets destroyed. Usually
-		 *			triggered by the runtime when the finalizer is called.
+		 * @brief	Called when the managed instance gets finalized. Triggered by the runtime
+		 *			on a finalizer thread.
+		 */
+		virtual void _onManagedInstanceFinalized() { }
+
+		/**
+		 * @brief	Called when the managed instance gets finalized. Triggered on the main thread
+		 *			some time after _onManagedInstanceFinalized is called.
 		 */
 		virtual void _onManagedInstanceDeleted();
 

SBansheeEngine/Include/BsScriptSceneObject.h

@@ -40,6 +40,11 @@ namespace BansheeEngine
 
 		ScriptSceneObject(MonoObject* instance, const HSceneObject& sceneObject);
 
+		/**
+		 * @copydoc	ScriptObjectBase::_onManagedInstanceFinalized
+		 */
+		void _onManagedInstanceFinalized() override;
+
 		/**
 		 * @copydoc	ScriptObjectBase::_onManagedInstanceDeleted
 		 */

SBansheeEngine/Source/BsScriptComponent.cpp

@@ -141,6 +141,7 @@ namespace BansheeEngine
 		if (scriptSO == nullptr)
 			scriptSO = ScriptGameObjectManager::instance().createScriptSceneObject(sceneObject);
 
+		assert(scriptSO->getManagedInstance() != nullptr);
 		return scriptSO->getManagedInstance();
 	}
 
@@ -186,21 +187,37 @@ namespace BansheeEngine
 
 	void ScriptComponent::endRefresh(const ScriptObjectBackup& backupData)
 	{
-		ComponentBackupData componentBackup = any_cast<ComponentBackupData>(backupData.data);
-		mManagedComponent->restore(mManagedInstance, componentBackup, mTypeMissing);
+		// It's possible that managed component is destroyed but a reference to it
+		// is still kept during assembly refresh. Such components shouldn't be restored
+		// so we delete them.
+		if (mManagedComponent.isDestroyed(true))
+		{
+			ScriptGameObjectManager::instance().destroyScriptGameObject(this);
+
+			mManagedInstance = nullptr;
+			bs_delete(this);
+		}
+		else
+		{
+			ComponentBackupData componentBackup = any_cast<ComponentBackupData>(backupData.data);
+			mManagedComponent->restore(mManagedInstance, componentBackup, mTypeMissing);
+		}
 		
 		ScriptGameObjectBase::endRefresh(backupData);
 	}
 
+	void ScriptComponent::_onManagedInstanceFinalized()
+	{
+		if (!mRefreshInProgress.load(std::memory_order_acquire))
+			ScriptGameObjectManager::instance().destroyScriptGameObject(this);
+	}
+
 	void ScriptComponent::_onManagedInstanceDeleted()
 	{
 		mManagedInstance = nullptr;
 
-		// It's possible that managed component is destroyed but a reference to it
-		// is still kept during assembly refresh. Such components shouldn't be restored
-		// so we delete them.
-		if (!mRefreshInProgress || mManagedComponent.isDestroyed(true))
-			ScriptGameObjectManager::instance().destroyScriptGameObject(this);
+		if (!mRefreshInProgress.load(std::memory_order_acquire))
+			bs_delete(this);
 	}
 
 	void ScriptComponent::setNativeHandle(const HGameObject& gameObject)

SBansheeEngine/Source/BsScriptGameObject.cpp

@@ -1,22 +1,24 @@
 #include "BsScriptGameObject.h"
-#include "BsDebug.h"
+#include "BsScriptGameObjectManager.h"
 
 namespace BansheeEngine
 {
 	ScriptGameObjectBase::ScriptGameObjectBase(MonoObject* instance)
 		:PersistentScriptObjectBase(instance), mRefreshInProgress(false)
-	{ }
+	{
+		
+	}
 
 	ScriptObjectBackup ScriptGameObjectBase::beginRefresh()
 	{
-		mRefreshInProgress = true;
+		mRefreshInProgress.store(true, std::memory_order_release);
 
 		return PersistentScriptObjectBase::beginRefresh();
 	}
 
 	void ScriptGameObjectBase::endRefresh(const ScriptObjectBackup& backupData)
 	{
-		mRefreshInProgress = false;
+		mRefreshInProgress.store(false, std::memory_order_release);
 
 		PersistentScriptObjectBase::endRefresh(backupData);
 	}

SBansheeEngine/Source/BsScriptGameObjectManager.cpp

@@ -36,11 +36,19 @@ namespace BansheeEngine
 
 	ScriptSceneObject* ScriptGameObjectManager::getOrCreateScriptSceneObject(const HSceneObject& sceneObject)
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(sceneObject.getInstanceId());
 		if (findIter != mScriptGameObjects.end())
 			return static_cast<ScriptSceneObject*>(findIter->second.instance);
 
-		return createScriptSceneObject(sceneObject);
+		MonoClass* sceneObjectClass = ScriptAssemblyManager::instance().getSceneObjectClass();
+		MonoObject* instance = sceneObjectClass->createInstance();
+
+		ScriptSceneObject* nativeInstance = new (bs_alloc<ScriptSceneObject>()) ScriptSceneObject(instance, sceneObject);
+		mScriptGameObjects[sceneObject.getInstanceId()] = ScriptGameObjectEntry(nativeInstance, false);
+
+		return nativeInstance;
 	}
 
 	ScriptSceneObject* ScriptGameObjectManager::createScriptSceneObject(const HSceneObject& sceneObject)
@@ -53,6 +61,8 @@ namespace BansheeEngine
 
 	ScriptSceneObject* ScriptGameObjectManager::createScriptSceneObject(MonoObject* existingInstance, const HSceneObject& sceneObject)
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(sceneObject.getInstanceId());
 		if(findIter != mScriptGameObjects.end())
 			BS_EXCEPT(InvalidStateException, "Script SceneObject for this SceneObject already exists.");
@@ -65,6 +75,8 @@ namespace BansheeEngine
 
 	ScriptComponent* ScriptGameObjectManager::createScriptComponent(MonoObject* existingInstance, const GameObjectHandle<ManagedComponent>& component)
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(component->getInstanceId());
 		if(findIter != mScriptGameObjects.end())
 			BS_EXCEPT(InvalidStateException, "Script component for this Component already exists.");
@@ -78,6 +90,8 @@ namespace BansheeEngine
 
 	ScriptComponent* ScriptGameObjectManager::getScriptComponent(const GameObjectHandle<ManagedComponent>& component) const
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(component.getInstanceId());
 		if(findIter != mScriptGameObjects.end())
 			return static_cast<ScriptComponent*>(findIter->second.instance);
@@ -87,6 +101,8 @@ namespace BansheeEngine
 
 	ScriptComponent* ScriptGameObjectManager::getScriptComponent(UINT64 instanceId) const
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(instanceId);
 		if (findIter != mScriptGameObjects.end())
 			return static_cast<ScriptComponent*>(findIter->second.instance);
@@ -96,6 +112,8 @@ namespace BansheeEngine
 
 	ScriptSceneObject* ScriptGameObjectManager::getScriptSceneObject(const HSceneObject& sceneObject) const
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(sceneObject.getInstanceId());
 		if(findIter != mScriptGameObjects.end())
 			return static_cast<ScriptSceneObject*>(findIter->second.instance);
@@ -105,6 +123,8 @@ namespace BansheeEngine
 
 	ScriptGameObjectBase* ScriptGameObjectManager::getScriptGameObject(UINT64 instanceId) const
 	{
+		Lock<> lock(mMutex);
+
 		auto findIter = mScriptGameObjects.find(instanceId);
 		if (findIter != mScriptGameObjects.end())
 			return static_cast<ScriptSceneObject*>(findIter->second.instance);
@@ -114,15 +134,22 @@ namespace BansheeEngine
 
 	void ScriptGameObjectManager::destroyScriptGameObject(ScriptGameObjectBase* gameObject)
 	{
+		Lock<> lock(mMutex);
+
 		UINT64 idx = gameObject->getNativeHandle().getInstanceId();
 		mScriptGameObjects.erase(idx);
-
-		bs_delete(gameObject);
 	}
 
 	void ScriptGameObjectManager::sendComponentResetEvents()
 	{
-		for (auto& scriptObjectEntry : mScriptGameObjects)
+		UnorderedMap<UINT64, ScriptGameObjectEntry> scriptGameObjects;
+
+		{
+			Lock<> lock(mMutex);
+			scriptGameObjects = mScriptGameObjects;
+		}
+
+		for (auto& scriptObjectEntry : scriptGameObjects)
 		{
 			const ScriptGameObjectEntry& entry = scriptObjectEntry.second;
 

SBansheeEngine/Source/BsScriptObjectManager.cpp

@@ -75,6 +75,8 @@ namespace BansheeEngine
 	{
 		assert(instance != nullptr);
 
+		instance->_onManagedInstanceFinalized();
+
 		BS_LOCK_MUTEX(mMutex);
 		mFinalizedObjects[mFinalizedQueueIdx].push_back(instance);
 	}

SBansheeEngine/Source/BsScriptSceneObject.cpp

@@ -338,12 +338,18 @@ namespace BansheeEngine
 		return false;
 	}
 
+	void ScriptSceneObject::_onManagedInstanceFinalized()
+	{
+		if (!mRefreshInProgress.load(std::memory_order_acquire))
+			ScriptGameObjectManager::instance().destroyScriptGameObject(this);
+	}
+
 	void ScriptSceneObject::_onManagedInstanceDeleted()
 	{
 		mManagedInstance = nullptr;
 
-		if (!mRefreshInProgress)
-			ScriptGameObjectManager::instance().destroyScriptGameObject(this);
+		if (!mRefreshInProgress.load(std::memory_order_acquire))
+			bs_delete(this);
 	}
 
 	void ScriptSceneObject::setNativeHandle(const HGameObject& gameObject)

TODO.txt

@@ -55,7 +55,6 @@ Polish
 Ribek use:
  - Hook up color picker to guicolor field
  - When hiding a component in inspector, it doesn't immediately reposition the components below it
- - Having en empty component in inspector shows a small empty background, it shouldn't show anything
  - Resource inspectors for: Shader, GUISkin, StringTable
  - Test release mode