Modified how C# GUI element destruction works so it now properly handles elements that go out of scope

MBansheeEngine/GUI/GUIElement.cs

@@ -6,9 +6,6 @@ namespace BansheeEngine
 {
     public abstract class GUIElement : ScriptObject
     {
-        protected GUILayout parent;
-        private bool isDestroyed;
-
         public Action<bool> OnFocusChanged;
 
         public Rect2I Bounds
@@ -28,25 +25,9 @@ namespace BansheeEngine
                 OnFocusChanged(focus);
         }
 
-        internal virtual void SetParent(GUILayout layout)
-        {
-            if (parent != null)
-                parent.RemoveInternal(this);
-
-            parent = layout;
-        }
-
         public virtual void Destroy()
         {
-            SetParent(null);
-
             Internal_Destroy(mCachedPtr);
-            isDestroyed = true;
-        }
-
-        public bool IsDestroyed()
-        {
-            return isDestroyed;
         }
 
         public void SetVisible(bool visible)

MBansheeEngine/GUI/GUILayout.cs

@@ -6,111 +6,26 @@ namespace BansheeEngine
 {
     public abstract class GUILayout : GUIElement
     {
-        internal List<GUIElement> children = new List<GUIElement>();
-
-        internal bool AddElementInternal(GUIElement element)
-        {
-            if (IsDestroyed())
-            {
-                Debug.LogWarning("Attempting to add an element to a destroyed layout. Ignoring operation.");
-                return false;
-            }
-
-            if (!children.Contains(element))
-            {
-                element.SetParent(this);
-                children.Add(element);
-
-                return true;
-            }
-
-            return false;
-        }
-
-        internal bool InsertElementInternal(int index, GUIElement element)
-        {
-            if (IsDestroyed())
-            {
-                Debug.LogWarning("Attempting to add an element to a destroyed layout. Ignoring operation.");
-                return false;
-            }
-
-            if (!children.Contains(element))
-            {
-                element.SetParent(this);
-                children.Insert(index, element);
-
-                return true;
-            }
-
-            return false;
-        }
-
-        internal void RemoveInternal(GUIElement element)
-        {
-            children.Remove(element);
-        }
-
-        internal override void SetParent(GUILayout layout)
-        {
-            if (parent != null)
-                parent.RemoveInternal(this);
-
-            parent = layout;
-
-            if (parent != null)
-                parent.children.Add(this);
-        }
-
         public void AddElement(GUIElement element)
         {
-            if(AddElementInternal(element))
+            if(element != null)
                 Internal_AddElement(mCachedPtr, element.mCachedPtr);
         }
 
         public void InsertElement(int index, GUIElement element)
         {
-            if (InsertElementInternal(index, element))
+            if (element != null)
                 Internal_InsertElement(mCachedPtr, index, element.mCachedPtr);
         }
 
-        public void Remove(GUIElement element)
-        {
-            if (children.Contains(element))
-                element.SetParent(null);
-        }
-
-        public void Remove(int childIdx)
-        {
-            if (childIdx >= 0 && childIdx < children.Count)
-            {
-                GUIElement element = children[childIdx];
-                element.SetParent(null);
-            }
-        }
-
         public int GetNumChildren()
         {
-            return children.Count;
+            return Internal_GetChildCount(mCachedPtr);
         }
 
         public GUIElement GetChild(int index)
         {
-            if (index < 0 || index >= children.Count)
-                return null;
-
-            return children[index];
-        }
-
-        public override void Destroy()
-        {
-            GUIElement[] childArray = children.ToArray(); // Iterating over it will modify it so make a copy
-            for (int i = 0; i < childArray.Length; i++)
-                childArray[i].Destroy();
-
-            children.Clear();
-
-            base.Destroy();
+            return Internal_GetChild(mCachedPtr, index);
         }
 
         public GUILayoutX AddLayoutX(params GUIOption[] options)
@@ -215,5 +130,11 @@ namespace BansheeEngine
 
         [MethodImpl(MethodImplOptions.InternalCall)]
         protected static extern void Internal_InsertElement(IntPtr instance, int index, IntPtr element);
+
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        protected static extern int Internal_GetChildCount(IntPtr instance);
+
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        protected static extern GUIElement Internal_GetChild(IntPtr instance, int index);
     }
 }

MBansheeEngine/GUI/GUISpace.cs

@@ -10,18 +10,6 @@ namespace BansheeEngine
             Internal_CreateInstance(this, size);
         }
 
-        internal override void SetParent(GUILayout layout)
-        {
-            // Space only gets one parent set on initialization and then it cannot be moved
-            if (parent == null)
-            {
-                parent = layout;
-
-                if (parent != null)
-                    parent.children.Add(this);
-            }
-        }
-
         public void SetSize(int size)
         {
             Internal_SetSize(mCachedPtr, size);
@@ -41,18 +29,6 @@ namespace BansheeEngine
             Internal_CreateInstance(this);
         }
 
-        internal override void SetParent(GUILayout layout)
-        {
-            // Space only gets one parent set on initialization and then it cannot be moved
-            if (parent == null)
-            {
-                parent = layout;
-
-                if (parent != null)
-                    parent.children.Add(this);
-            }
-        }
-
         [MethodImpl(MethodImplOptions.InternalCall)]
         private static extern void Internal_CreateInstance(GUIFlexibleSpace instance);
     }

SBansheeEditor/Include/BsScriptDropDownWindow.h

@@ -70,5 +70,6 @@ namespace BansheeEngine
 		uint32_t mGCHandle;
 
 		ScriptDropDownWindow* mScriptParent;
+		ScriptGUILayout* mContentsPanel;
 	};
 }

SBansheeEditor/Include/BsScriptEditorWindow.h

@@ -97,5 +97,6 @@ namespace BansheeEngine
 		MonoObject* mManagedInstance;
 
 		ScriptEditorWindow* mScriptParent;
+		ScriptGUILayout* mContentsPanel;
 	};
 }

SBansheeEditor/Include/BsScriptModalWindow.h

@@ -90,5 +90,6 @@ namespace BansheeEngine
 		uint32_t mGCHandle;
 
 		ScriptModalWindow* mScriptParent;
+		ScriptGUILayout* mContentsPanel;
 	};
 }

SBansheeEditor/Source/BsScriptDropDownWindow.cpp

@@ -126,11 +126,12 @@ namespace BansheeEngine
 	ManagedDropDownWindow::ManagedDropDownWindow(const RenderWindowPtr& parent, Viewport* target,
 		const Vector2I& position, MonoObject* managedInstance, UINT32 width, UINT32 height)
 		:DropDownWindow(parent, target, position, width, height), mUpdateThunk(nullptr), mManagedInstance(managedInstance),
-		mOnInitializeThunk(nullptr), mOnDestroyThunk(nullptr), mGCHandle(0), mScriptParent(nullptr)
+		mOnInitializeThunk(nullptr), mOnDestroyThunk(nullptr), mGCHandle(0), mScriptParent(nullptr), mContentsPanel(nullptr)
 	{
 		mGCHandle = mono_gchandle_new(mManagedInstance, false);
 
 		MonoObject* guiPanel = ScriptGUIPanel::createFromExisting(mContents);
+		mContentsPanel = ScriptGUILayout::toNative(guiPanel);
 		ScriptDropDownWindow::guiPanelField->setValue(mManagedInstance, guiPanel);
 
 		::MonoClass* rawMonoClass = mono_object_get_class(mManagedInstance);
@@ -144,6 +145,7 @@ namespace BansheeEngine
 
 	ManagedDropDownWindow::~ManagedDropDownWindow()
 	{
+		mContentsPanel->destroy();
 		triggerOnDestroy();
 		mScriptParent->notifyWindowClosed();
 

SBansheeEditor/Source/BsScriptEditorWindow.cpp

@@ -255,13 +255,16 @@ namespace BansheeEngine
 
 	ScriptEditorWidget::ScriptEditorWidget(const String& ns, const String& type, EditorWidgetContainer& parentContainer)
 		:EditorWidgetBase(HString(toWString(type)), ns + "." + type, parentContainer), mNamespace(ns), mTypename(type),
-		mUpdateThunk(nullptr), mManagedInstance(nullptr), mOnInitializeThunk(nullptr), mOnDestroyThunk(nullptr)
+		mUpdateThunk(nullptr), mManagedInstance(nullptr), mOnInitializeThunk(nullptr), mOnDestroyThunk(nullptr), mContentsPanel(nullptr)
 	{
 		createManagedInstance();
 	}
 
 	ScriptEditorWidget::~ScriptEditorWidget()
 	{
+		mContentsPanel->destroy();
+		mContentsPanel = nullptr;
+
 		triggerOnDestroy();
 		ScriptEditorWindow::unregisterScriptEditorWindow(getName());
 	}
@@ -279,6 +282,7 @@ namespace BansheeEngine
 				mManagedInstance = editorWindowClass->createInstance();
 
 				MonoObject* guiPanel = ScriptGUIPanel::createFromExisting(mContent);
+				mContentsPanel = ScriptGUILayout::toNative(guiPanel);
 				ScriptEditorWindow::guiPanelField->setValue(mManagedInstance, guiPanel);
 
 				reloadMonoTypes(editorWindowClass);

SBansheeEditor/Source/BsScriptModalWindow.cpp

@@ -73,9 +73,9 @@ namespace BansheeEngine
 		if (mModalWindow == nullptr)
 			return;
 
+		mModalWindow->mContentsPanel->destroy();
 		mModalWindow->triggerOnDestroy();
 		mModalWindow->releaseManagedInstance();
-		mModalWindow->close();
 		mModalWindow = nullptr;
 	}
 
@@ -188,7 +188,7 @@ namespace BansheeEngine
 	ManagedModalWindow::ManagedModalWindow(bool allowCloseButton, MonoObject* managedInstance)
 		:ModalWindow(HString::dummy(), allowCloseButton), mUpdateThunk(nullptr), mManagedInstance(managedInstance),
 		mOnInitializeThunk(nullptr), mOnDestroyThunk(nullptr), mOnWindowResizedMethod(nullptr), mGCHandle(0),
-		mScriptParent(nullptr)
+		mScriptParent(nullptr), mContentsPanel(nullptr)
 	{
 		mGCHandle = mono_gchandle_new(mManagedInstance, false);
 
@@ -223,6 +223,7 @@ namespace BansheeEngine
 				mGCHandle = mono_gchandle_new(mManagedInstance, false);
 
 				MonoObject* guiPanel = ScriptGUIPanel::createFromExisting(mContents);
+				mContentsPanel = ScriptGUILayout::toNative(guiPanel);
 				ScriptModalWindow::guiPanelField->setValue(mManagedInstance, guiPanel);
 
 				reloadMonoTypes(editorWindowClass);

SBansheeEngine/Include/BsScriptGUIElement.h

@@ -16,14 +16,20 @@ namespace BansheeEngine
 		GUIElementBase* getGUIElement() const { return (GUIElementBase*)mElement; }
 
 		virtual void destroy() = 0;
+		bool isDestroyed() const { return mIsDestroyed; }
+		ScriptGUILayout* getParent() const { return mParent; }
+		void setParent(ScriptGUILayout* parent) { mParent = parent; }
 
 	protected:
 		void initialize(GUIElementBase* element);
 
+		virtual void _onManagedInstanceDeleted() override;
+
 		static void onFocusChanged(MonoObject* instance, bool focus);
 
 		bool mIsDestroyed;
 		GUIElementBase* mElement;
+		ScriptGUILayout* mParent;
 	};
 
 	template <class Type>
@@ -56,7 +62,7 @@ namespace BansheeEngine
 		ScriptGUIElementTBase(MonoObject* instance);
 		virtual ~ScriptGUIElementTBase() {}
 
-		virtual void destroy();
+		virtual void destroy() override;
 	};
 
 	template <class Type>

SBansheeEngine/Include/BsScriptGUILayout.h

@@ -7,28 +7,42 @@ namespace BansheeEngine
 {
 	class BS_SCR_BE_EXPORT ScriptGUILayout : public TScriptGUIElementBase<ScriptGUILayout>
 	{
+		struct ChildInfo
+		{
+			ScriptGUIElementBaseTBase* element;
+			uint32_t gcHandle;
+		};
+
 	public:
 		SCRIPT_OBJ(ENGINE_ASSEMBLY, "BansheeEngine", "GUILayout")
 
 		GUILayout* getInternalValue() const { return mLayout; }
 		void* getNativeRaw() const { return mLayout; }
 
+		void addChild(ScriptGUIElementBaseTBase* element);
+		void insertChild(UINT32 index, ScriptGUIElementBaseTBase* element);
+		void removeChild(ScriptGUIElementBaseTBase* element);
+
+		void destroy() override;
+
 	private:
 		friend class ScriptGUIPanel;
 
 		static void internal_createInstanceX(MonoObject* instance, MonoArray* guiOptions);
 		static void internal_createInstanceY(MonoObject* instance, MonoArray* guiOptions);
 		static void internal_createInstancePanel(MonoObject* instance, INT16 depth, UINT16 depthRangeMin, UINT32 depthRangeMax, MonoArray* guiOptions);
-		static void internal_addElement(ScriptGUILayout* instance, ScriptGUIElementTBase* element);
-		static void internal_insertElement(ScriptGUILayout* instance, UINT32 index, ScriptGUIElementTBase* element);
+		static void internal_addElement(ScriptGUILayout* instance, ScriptGUIElementBaseTBase* element);
+		static void internal_insertElement(ScriptGUILayout* instance, UINT32 index, ScriptGUIElementBaseTBase* element);
+		static UINT32 internal_getChildCount(ScriptGUILayout* instance);
+		static MonoObject* internal_getChild(ScriptGUILayout* instance, UINT32 index);
 
 		static void internal_createInstanceYFromScrollArea(MonoObject* instance, MonoObject* parentScrollArea);
 
 		ScriptGUILayout(MonoObject* instance, GUILayout* layout);
 
-		void destroy();
-
 		GUILayout* mLayout;
+		Vector<ChildInfo> mChildren;
+
 		bool mIsDestroyed;
 	};
 

SBansheeEngine/Source/BsScriptGUIElement.cpp

@@ -15,11 +15,9 @@ using namespace std::placeholders;
 namespace BansheeEngine
 {
 	ScriptGUIElementBaseTBase::ScriptGUIElementBaseTBase(MonoObject* instance)
-		:ScriptObjectBase(instance), mIsDestroyed(false), mElement(nullptr)
+		:ScriptObjectBase(instance), mIsDestroyed(false), mElement(nullptr), mParent(nullptr)
 	{ }
 
-
-
 	void ScriptGUIElementBaseTBase::initialize(GUIElementBase* element)
 	{
 		mElement = element;
@@ -39,6 +37,11 @@ namespace BansheeEngine
 		MonoUtil::throwIfException(exception);
 	}
 
+	void ScriptGUIElementBaseTBase::_onManagedInstanceDeleted()
+	{
+		destroy();
+	}
+
 	ScriptGUIElementTBase::ScriptGUIElementTBase(MonoObject* instance)
 		:ScriptGUIElementBaseTBase(instance)
 	{
@@ -49,6 +52,9 @@ namespace BansheeEngine
 	{
 		if(!mIsDestroyed)
 		{
+			if (mParent != nullptr)
+				mParent->removeChild(this);
+
 			if (mElement->_getType() == GUIElementBase::Type::Element)
 			{
 				GUIElement::destroy((GUIElement*)mElement);

SBansheeEngine/Source/BsScriptGUIFixedSpace.cpp

@@ -27,6 +27,9 @@ namespace BansheeEngine
 	{
 		if (!mIsDestroyed)
 		{
+			if (mParent != nullptr)
+				mParent->removeChild(this);
+
 			GUIFixedSpace::destroy(mFixedSpace);
 
 			mIsDestroyed = true;

SBansheeEngine/Source/BsScriptGUIFlexibleSpace.cpp

@@ -26,6 +26,9 @@ namespace BansheeEngine
 	{
 		if(!mIsDestroyed)
 		{
+			if (mParent != nullptr)
+				mParent->removeChild(this);
+
 			GUIFlexibleSpace::destroy(mFlexibleSpace);
 
 			mIsDestroyed = true;

SBansheeEngine/Source/BsScriptGUILayout.cpp

@@ -26,20 +26,65 @@ namespace BansheeEngine
 		metaData.scriptClass->addInternalCall("Internal_CreateInstanceYFromScrollArea", &ScriptGUILayout::internal_createInstanceYFromScrollArea);
 		metaData.scriptClass->addInternalCall("Internal_AddElement", &ScriptGUILayout::internal_addElement);
 		metaData.scriptClass->addInternalCall("Internal_InsertElement", &ScriptGUILayout::internal_insertElement);
+		metaData.scriptClass->addInternalCall("Internal_GetChildCount", &ScriptGUILayout::internal_getChildCount);
+		metaData.scriptClass->addInternalCall("Internal_GetChild", &ScriptGUILayout::internal_getChild);
 	}
 
 	void ScriptGUILayout::destroy()
 	{
 		if(!mIsDestroyed)
 		{
+			if (mParent != nullptr)
+				mParent->removeChild(this);
+
+			while (mChildren.size() > 0)
+			{
+				ChildInfo childInfo = mChildren[0];
+				childInfo.element->destroy();
+			}
+
 			GUILayout::destroy(mLayout);
 
 			mLayout = nullptr;
-
 			mIsDestroyed = true;
 		}
 	}
 
+	void ScriptGUILayout::addChild(ScriptGUIElementBaseTBase* element)
+	{
+		ChildInfo childInfo;
+
+		childInfo.element = element;
+		childInfo.gcHandle = mono_gchandle_new(element->getManagedInstance(), false);
+
+		mChildren.push_back(childInfo);
+	}
+
+	void ScriptGUILayout::insertChild(UINT32 idx, ScriptGUIElementBaseTBase* element)
+	{
+		ChildInfo childInfo;
+
+		childInfo.element = element;
+		childInfo.gcHandle = mono_gchandle_new(element->getManagedInstance(), false);
+
+		mChildren.insert(mChildren.begin() + idx, childInfo);
+	}
+
+	void ScriptGUILayout::removeChild(ScriptGUIElementBaseTBase* element)
+	{
+		auto iterFind = std::find_if(mChildren.begin(), mChildren.end(), 
+			[&](const ChildInfo& x)
+		{
+			return x.element == element;
+		});
+
+		if (iterFind != mChildren.end())
+		{
+			mono_gchandle_free(iterFind->gcHandle);
+			mChildren.erase(iterFind);
+		}
+	}
+
 	void ScriptGUILayout::internal_createInstanceX(MonoObject* instance, MonoArray* guiOptions)
 	{
 		GUIOptions options;
@@ -90,14 +135,48 @@ namespace BansheeEngine
 			ScriptGUILayout(instance, nativeLayout);
 	}
 
-	void ScriptGUILayout::internal_addElement(ScriptGUILayout* instance, ScriptGUIElementTBase* element)
+	void ScriptGUILayout::internal_addElement(ScriptGUILayout* instance, ScriptGUIElementBaseTBase* element)
 	{
+		if (instance->isDestroyed() || element->isDestroyed())
+			return;
+
 		instance->getInternalValue()->addElement(element->getGUIElement());
+
+		if (element->getParent() != nullptr)
+			element->getParent()->removeChild(element);
+
+		element->setParent(instance);
+		instance->addChild(element);
 	}
 
-	void ScriptGUILayout::internal_insertElement(ScriptGUILayout* instance, UINT32 index, ScriptGUIElementTBase* element)
+	void ScriptGUILayout::internal_insertElement(ScriptGUILayout* instance, UINT32 index, ScriptGUIElementBaseTBase* element)
 	{
+		if (instance->isDestroyed() || element->isDestroyed())
+			return;
+
 		instance->getInternalValue()->insertElement(index, element->getGUIElement());
+
+		if (element->getParent() != nullptr)
+			element->getParent()->removeChild(element);
+
+		element->setParent(instance);
+		instance->insertChild(index, element);
+	}
+
+	UINT32 ScriptGUILayout::internal_getChildCount(ScriptGUILayout* instance)
+	{
+		if (instance->isDestroyed())
+			return 0;
+
+		return instance->mLayout->getNumChildren();
+	}
+
+	MonoObject* ScriptGUILayout::internal_getChild(ScriptGUILayout* instance, UINT32 index)
+	{
+		if (instance->isDestroyed() || instance->mChildren.size() >= index)
+			return nullptr;
+
+		return instance->mChildren[index].element->getManagedInstance();
 	}
 
 	ScriptGUIPanel::ScriptGUIPanel(MonoObject* instance)

TODO.txt

@@ -35,12 +35,6 @@ Crash when opening and closing the context menu many times, trying to delete a G
  - In general, do I want GUI elements to go out of scope when not being referenced? Probably doesn't matter much, and even if I add C++ handles
    that doesn't solve my issue as elements will go out of scope during assembly refresh
 
-Other:
- - In larger icon views scrollbar doesn't show, and view elements can overlap the input box
-
-Test context menu
-Test drop down window
-
 When clicking on drop down window it will interact with GUI elements beneath it
 
 Simple tasks: