Properly destroy script instances

Now without memory leaks!

GlitchyEngine/src/Scripting/ScriptEngine.bf

@@ -340,6 +340,8 @@ static class ScriptEngine
 
 		Debug.Assert(s_Context == null, "StartRuntime was called twice without StopRuntime in between!");
 		Context = context;
+
+		ReloadAssemblies();
 	}
 
 	/// Stopts the script runtime and disposes of all script instances.
@@ -400,10 +402,18 @@ static class ScriptEngine
 
 		script.ScriptClassName = null;
 
+		DestroyInstance(entityId);
+	}
+
+	public static void DestroyInstance(UUID entityId)
+	{
 		if (_entityScriptInstances.TryGetValue(entityId, let currentInstance))
 			currentInstance.ReleaseRef();
+	}
 
-		_entityScriptInstances[entityId] = null;
+	internal static void UnregisterScriptInstance(UUID entityId)
+	{
+		_entityScriptInstances.Remove(entityId);
 	}
 
 	/// Returns an instance that can be used as a reference to the entity with the given ID in Scripts
@@ -806,7 +816,6 @@ static class ScriptEngine
 		return scriptClass;
 	}
 
-	
 	internal static void HandleMonoException(MonoException* exception, UUID entityId)
 	{
 		MonoExceptionHelper wrappedException = new MonoExceptionHelper(exception);

GlitchyEngine/src/Scripting/ScriptGlue.bf

@@ -107,7 +107,22 @@ static class ScriptGlue
 			Log.EngineLogger.AssertDebug(managedType != null, scope $"No C# component with name \"{className}\" found for Beef type \"{typeof(T)}\"");
 		}
 	}
-	
+
+	/// Gets the entity with the given id. Throws a mono exception, if the entity doesn't exist.
+	static Entity GetEntitySafe(UUID entityId)
+	{
+		Result<Entity> foundEntity = ScriptEngine.Context.GetEntityByID(entityId);
+
+		if (foundEntity case .Ok(let entity))
+		{
+			return foundEntity;
+		}
+		else
+		{
+			ThrowArgumentException(null, "The entity doesn't exist or was deleted.");
+		}
+	}
+
 	/// Gets the component of the specified type that is attached to the given entity. Or null, if the entity doesn't exist or doesn't have the specified component.
 	static T* GetComponentSafe<T>(UUID entityId) where T: struct, new
 	{
@@ -346,44 +361,41 @@ static class ScriptGlue
 	[RegisterCall("ScriptGlue::Entity_SetScript")]
 	static MonoObject* Entity_SetScript(UUID entityId, MonoReflectionType* scriptType)
 	{
-		Entity entity = ScriptEngine.Context.GetEntityByID(entityId);
+		Entity entity = GetEntitySafe(entityId);
+
+		ScriptComponent* scriptComponent = null;
 
 		if (!entity.HasComponent<ScriptComponent>())
 		{
-			entity.AddComponent<ScriptComponent>();
+			scriptComponent = entity.AddComponent<ScriptComponent>();
 		}
-
+		else
-		if (entity.TryGetComponent<ScriptComponent>(let scriptComponent))
 		{
-			scriptComponent.Instance = null;
+			scriptComponent = entity.GetComponent<ScriptComponent>();
-
-			MonoType* type = Mono.mono_reflection_type_get_type(scriptType);
-
-			scriptComponent.ScriptClassName = StringView(Mono.mono_type_full_name(type));
-			
-			// Initializes the created instance
-			// TODO: this returns false, if no script with ScriptClassName exists, we have to handle this case correctly I think.
-			ScriptEngine.InitializeInstance(entity, scriptComponent);
-
-			return scriptComponent.Instance.MonoInstance;
 		}
 
-		Log.EngineLogger.AssertDebug(false, "Failed to set script.");
+		if (scriptComponent.Instance != null)
+			ScriptEngine.Context.DestroyScriptDeferred(scriptComponent.Instance, false);
 
-		return null;
+		scriptComponent.Instance = null;
+
+		MonoType* type = Mono.mono_reflection_type_get_type(scriptType);
+
+		scriptComponent.ScriptClassName = StringView(Mono.mono_type_full_name(type));
+		
+		// Initializes the created instance
+		// TODO: this returns false, if no script with ScriptClassName exists, we have to handle this case correctly I think.
+		ScriptEngine.InitializeInstance(entity, scriptComponent);
+
+		return scriptComponent.Instance.MonoInstance;
 	}
 	
 	[RegisterCall("ScriptGlue::Entity_RemoveScript")]
 	static void Entity_RemoveScript(UUID entityId)
 	{
-		Entity entity = ScriptEngine.Context.GetEntityByID(entityId);
+		ScriptComponent* scriptComponent = GetComponentSafe<ScriptComponent>(entityId);
 
-		if (entity.TryGetComponent<ScriptComponent>(let scriptComponent))
+		ScriptEngine.Context.DestroyScriptDeferred(scriptComponent.Instance, true);
-		{
-			ScriptEngine.DestroyInstance(entity, scriptComponent);
-			
-			entity.RemoveComponent<ScriptComponent>();
-		}
 	}
 	
 	[RegisterCall("ScriptGlue::Entity_GetName")]
@@ -403,7 +415,7 @@ static class ScriptGlue
     
 	[RegisterCall("ScriptGlue::Entity_SetName")]
     static void Entity_SetName(UUID entityId, MonoString* name)
-	{		
+	{
 		Entity entity = ScriptEngine.Context.GetEntityByID(entityId);
 
 		char8* rawName = Mono.mono_string_to_utf8(name);
@@ -739,10 +751,12 @@ static class ScriptGlue
 		RegisterCall<function bool(half)>("Math.Half::IsInfinity_Impl", (value) => value.IsInfinity);
 		RegisterCall<function bool(half)>("Math.Half::IsNan_Impl", (value) => value.IsNaN);
 		RegisterCall<function bool(half)>("Math.Half::IsSubnormal_Impl", (value) => value.IsSubnormal);
+
+		RegisterCall<function float(float, float)>("Math.Math::Atan2", (y, x) => Math.Atan2(y, x));
 	}
 
 #endregion
-	
+
 	[RegisterCall("ScriptGlue::UUID_CreateNew")]
 	static void UUID_Create(out UUID id)
 	{

GlitchyEngine/src/Scripting/ScriptInstance.bf

@@ -39,13 +39,25 @@ class ScriptInstance : RefCounter
 	}
 
 	private ~this()
+	{
+		Destroy();
+		_scriptClass?.ReleaseRef();
+	}
+
+	public void Destroy()
 	{
 		if (_instance != null)
 		{
-			InvokeOnDestroy();
+			if (ScriptEngine.ApplicationInfo.IsInPlayMode || ScriptClass.RunInEditMode)
+			{
+				InvokeOnDestroy();
+			}
+
 			Mono.mono_gchandle_free(_gcHandle);
+			_instance = null;
+
+			ScriptEngine.UnregisterScriptInstance(_entityId);
 		}
-		_scriptClass?.ReleaseRef();
 	}
 
 	public void Instantiate(UUID uuid)

GlitchyEngine/src/World/Scene.bf

@@ -621,6 +621,8 @@ namespace GlitchyEngine.World
 
 		private append List<Entity> _destroyQueue = .();
 
+		private append List<ScriptInstance> _destroyScriptQueue = .();
+
 		public void Update(GameTime gameTime, UpdateMode mode)
 		{
 			Debug.Profiler.ProfileRendererFunction!();
@@ -634,31 +636,73 @@ namespace GlitchyEngine.World
 				// Run scripts
 				for (let (entity, script) in _ecsWorld.Enumerate<ScriptComponent>())
 				{
+					ScriptInstance scriptInstance = null;
+
 					if (!script.IsCreated)
 					{
 						if (!script.IsInitialized)
 							ScriptEngine.InitializeInstance(Entity(entity, this), script);
+						
+						scriptInstance = script.Instance;
 
 						// Skip OnCreate and OnUpdate invocation if we didn't create an instance
 						// (happens, if script component has no script class associated)
 						// Also skip if we are in edit mode and the class doesn't have the RunInEditMode-Attribute
-						if (script.Instance == null || (mode.HasFlag(.EditMode) && !script.Instance.ScriptClass.RunInEditMode))
+						if (scriptInstance == null || (mode.HasFlag(.EditMode) && !scriptInstance.ScriptClass.RunInEditMode))
 							continue;
 
-						script.Instance.InvokeOnCreate();
+						// OnCreate can remove the script, so we have to make sure that we have a reference and it survives.
+						// Todo: Technically we can rely on scriptInstance surviving a delete because we always defer deletion (unless we might not?)
+						scriptInstance.AddRef();
+
+						scriptInstance.InvokeOnCreate();
+					}
+					else
+					{
+						scriptInstance = script.Instance..AddRef();
 					}
 
+					// TODO: When the entity is destroyed in OnCreate it's OnUpdate will still be called. Is this fine?
+					// It would require that we somehow track whether the entity is to be deleted. We technically have this info but would probably need
+					// some faster way. If we for some reason ever happen to implement such a fast way we can check for planned deletion here (or after on Create and just continue;).
+
 					// Update the script, if we aren't in editor or it has RunInEditMode-Attribute
-					if (!mode.HasFlag(.EditMode) || script.Instance.ScriptClass.RunInEditMode)
+					if (!mode.HasFlag(.EditMode) || scriptInstance.ScriptClass.RunInEditMode)
 					{
 						if (_updateBlockList.Contains(entity))
 							_updateBlockList.Remove(entity);
 						else
-							script.Instance.InvokeOnUpdate(gameTime.DeltaTime);
+							scriptInstance.InvokeOnUpdate(gameTime.DeltaTime);
 					}
+
+					scriptInstance?.ReleaseRef();
 				}
 			}
 
+			if (!_destroyScriptQueue.IsEmpty)
+			{
+				for (let scriptInstance in _destroyScriptQueue)
+				{
+					scriptInstance.ReleaseRef();
+
+					Log.EngineLogger.AssertDebug(scriptInstance.RefCount == 1, "Too many references to script instance. Did we leak it?");
+
+					ScriptEngine.DestroyInstance(scriptInstance.EntityId);
+				}
+
+				_destroyScriptQueue.Clear();
+			}
+
+			if (!_destroyQueue.IsEmpty)
+			{
+				for (let entity in _destroyQueue)
+				{
+					DestroyEntity(entity);
+				}
+
+				_destroyQueue.Clear();
+			}
+
 			if (mode.HasFlag(.Physics))
 			{
 				// Update 2D physics
@@ -716,17 +760,6 @@ namespace GlitchyEngine.World
 					}
 				}
 			}
-
-			if (!_destroyQueue.IsEmpty)
-			{
-				for (let entity in _destroyQueue)
-				{
-					DestroyEntity(entity);
-				}
-
-				_destroyQueue.Clear();
-
-			}
 		}
 
 		/// Creates a new Entity with the given name.
@@ -755,6 +788,8 @@ namespace GlitchyEngine.World
 		public void DestroyEntity(Entity entity, bool destroyChildren = false)
 		{
 			_idToEntity.Remove(entity.UUID);
+			
+			ScriptEngine.DestroyInstance(entity.UUID);
 
 			if (destroyChildren)
 			{
@@ -780,6 +815,28 @@ namespace GlitchyEngine.World
 			}
 		}
 
+		/**
+		 * Marks the given scriptInstance so that it will be deleted at the end of the update-loop. 
+		 * @param scriptInstance The script instance to destroy.
+		 * @param removeComponent If set to true the ScriptComponent will be removed from the entity.
+		 */
+		public void DestroyScriptDeferred(ScriptInstance scriptInstance, bool removeComponent)
+		{
+			_destroyScriptQueue.Add(scriptInstance..AddRef());
+
+			if (removeComponent)
+			{
+				Result<Entity> foundEntity = GetEntityByID(scriptInstance.EntityId);
+
+				Log.EngineLogger.AssertDebug(foundEntity case .Ok, "DestroyScriptDeferred: Could not find entity.");
+
+				if (foundEntity case .Ok(let entity))
+				{
+					entity.RemoveComponent<ScriptComponent>();
+				}
+			}
+		}
+
 		/** Creates a copy of the given entity, including all components and children.
 		 * @param entity the entity to copy.
 		 * @returns the newly create entity.