From c906fc9667c8dd873b1bacf3908421e8d0ea77a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20L=C3=BCbe=C3=9F?= Date: Thu, 18 Jan 2024 21:29:11 +0100 Subject: [PATCH] Properly destroy script instances Now without memory leaks! --- GlitchyEngine/src/Scripting/ScriptEngine.bf | 13 ++- GlitchyEngine/src/Scripting/ScriptGlue.bf | 68 +++++++++------ GlitchyEngine/src/Scripting/ScriptInstance.bf | 16 +++- GlitchyEngine/src/World/Scene.bf | 87 +++++++++++++++---- 4 files changed, 138 insertions(+), 46 deletions(-) diff --git a/GlitchyEngine/src/Scripting/ScriptEngine.bf b/GlitchyEngine/src/Scripting/ScriptEngine.bf index 2d09e65..dd84f5e 100644 --- a/GlitchyEngine/src/Scripting/ScriptEngine.bf +++ b/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); diff --git a/GlitchyEngine/src/Scripting/ScriptGlue.bf b/GlitchyEngine/src/Scripting/ScriptGlue.bf index 1227e86..6e7ad35 100644 --- a/GlitchyEngine/src/Scripting/ScriptGlue.bf +++ b/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 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(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()) { - entity.AddComponent(); + scriptComponent = entity.AddComponent(); } - - if (entity.TryGetComponent(let scriptComponent)) + else { - 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; + scriptComponent = entity.GetComponent(); } - 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(entityId); - if (entity.TryGetComponent(let scriptComponent)) - { - ScriptEngine.DestroyInstance(entity, scriptComponent); - - entity.RemoveComponent(); - } + ScriptEngine.Context.DestroyScriptDeferred(scriptComponent.Instance, true); } [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("Math.Half::IsInfinity_Impl", (value) => value.IsInfinity); RegisterCall("Math.Half::IsNan_Impl", (value) => value.IsNaN); RegisterCall("Math.Half::IsSubnormal_Impl", (value) => value.IsSubnormal); + + RegisterCall("Math.Math::Atan2", (y, x) => Math.Atan2(y, x)); } #endregion - + [RegisterCall("ScriptGlue::UUID_CreateNew")] static void UUID_Create(out UUID id) { diff --git a/GlitchyEngine/src/Scripting/ScriptInstance.bf b/GlitchyEngine/src/Scripting/ScriptInstance.bf index 9667887..b6b2254 100644 --- a/GlitchyEngine/src/Scripting/ScriptInstance.bf +++ b/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) diff --git a/GlitchyEngine/src/World/Scene.bf b/GlitchyEngine/src/World/Scene.bf index 9d95b34..19fd53b 100644 --- a/GlitchyEngine/src/World/Scene.bf +++ b/GlitchyEngine/src/World/Scene.bf @@ -621,6 +621,8 @@ namespace GlitchyEngine.World private append List _destroyQueue = .(); + private append List _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()) { + 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 foundEntity = GetEntityByID(scriptInstance.EntityId); + + Log.EngineLogger.AssertDebug(foundEntity case .Ok, "DestroyScriptDeferred: Could not find entity."); + + if (foundEntity case .Ok(let entity)) + { + entity.RemoveComponent(); + } + } + } + /** Creates a copy of the given entity, including all components and children. * @param entity the entity to copy. * @returns the newly create entity.