From e21729de6778be2a9962eb09295117ee898fbc05 Mon Sep 17 00:00:00 2001 From: Ellpeck Date: Thu, 24 Nov 2022 18:38:51 +0100 Subject: [PATCH] fixed some memory management issues in MLEM.Ui --- CHANGELOG.md | 4 ++++ MLEM.Ui/Elements/Element.cs | 5 ++++- MLEM.Ui/Elements/Panel.cs | 24 ++++++++++++------------ MLEM.Ui/Parsers/UiParser.cs | 12 +++++++++--- MLEM/Graphics/StaticSpriteBatch.cs | 1 - 5 files changed, 29 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad707a1..07d0d9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,10 @@ Fixes - Fixed an exception when trying to force-update the area of an element without a ui system - Fixed the scroll bar of an empty panel being positioned incorrectly - Fixed UiControls maintaining old input states when input types are toggled off +- Fixed an occasional deadlock when a game is disposed with a scrolling Panel present + +Removals +- Marked Element.OnDisposed as obsolete in favor of the more predictable OnRemovedFromUi ### MLEM.Data Additions diff --git a/MLEM.Ui/Elements/Element.cs b/MLEM.Ui/Elements/Element.cs index 049ed6e..aabcb30 100644 --- a/MLEM.Ui/Elements/Element.cs +++ b/MLEM.Ui/Elements/Element.cs @@ -425,9 +425,10 @@ namespace MLEM.Ui.Elements { /// public GenericCallback OnRemovedFromUi; /// - /// Event that is called when this element's method is called, which also happens in . + /// Event that is called when this element's method is called. /// This event is useful for unregistering global event handlers when this object should be destroyed. /// + [Obsolete("OnDisposed will be removed in a future update. To unregister custom event handlers, use OnRemovedFromUi instead.")] public GenericCallback OnDisposed; /// @@ -488,6 +489,7 @@ namespace MLEM.Ui.Elements { } /// + [Obsolete("Dispose will be removed in a future update. To unregister custom event handlers, use OnRemovedFromUi instead.")] ~Element() { this.Dispose(); } @@ -1098,6 +1100,7 @@ namespace MLEM.Ui.Elements { } /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources. + [Obsolete("Dispose will be removed in a future update. To unregister custom event handlers, use OnRemovedFromUi instead.")] public virtual void Dispose() { this.OnDisposed?.Invoke(this); GC.SuppressFinalize(this); diff --git a/MLEM.Ui/Elements/Panel.cs b/MLEM.Ui/Elements/Panel.cs index dafda63..5f050e7 100644 --- a/MLEM.Ui/Elements/Panel.cs +++ b/MLEM.Ui/Elements/Panel.cs @@ -70,6 +70,13 @@ namespace MLEM.Ui.Elements { this.scrollOverflow = scrollOverflow; this.CanBeSelected = false; + // we dispose our render target when removing so that it doesn't cause a memory leak + // if we're added back afterwards, it'll be recreated in ScrollSetup anyway + this.OnRemovedFromUi += _ => { + this.renderTarget?.Dispose(); + this.renderTarget = null; + }; + if (scrollOverflow) { this.ScrollBar = new ScrollBar(Anchor.TopRight, Vector2.Zero, 0, 0) { OnValueChanged = (element, value) => this.ScrollChildren(), @@ -177,15 +184,6 @@ namespace MLEM.Ui.Elements { return base.GetElementUnderPos(position); } - /// - public override void Dispose() { - if (this.renderTarget != null) { - this.renderTarget.Dispose(); - this.renderTarget = null; - } - base.Dispose(); - } - /// /// Scrolls this panel's to the given in such a way that its center is positioned in the center of this panel. /// @@ -274,11 +272,13 @@ namespace MLEM.Ui.Elements { // update the render target var targetArea = (Rectangle) this.GetRenderTargetArea(); - if (targetArea.Width <= 0 || targetArea.Height <= 0) + if (targetArea.Width <= 0 || targetArea.Height <= 0) { + this.renderTarget?.Dispose(); + this.renderTarget = null; return; + } if (this.renderTarget == null || targetArea.Width != this.renderTarget.Width || targetArea.Height != this.renderTarget.Height) { - if (this.renderTarget != null) - this.renderTarget.Dispose(); + this.renderTarget?.Dispose(); this.renderTarget = targetArea.IsEmpty ? null : new RenderTarget2D(this.System.Game.GraphicsDevice, targetArea.Width, targetArea.Height); this.relevantChildrenDirty = true; } diff --git a/MLEM.Ui/Parsers/UiParser.cs b/MLEM.Ui/Parsers/UiParser.cs index b50525e..a76e454 100644 --- a/MLEM.Ui/Parsers/UiParser.cs +++ b/MLEM.Ui/Parsers/UiParser.cs @@ -27,7 +27,7 @@ namespace MLEM.Ui.Parsers { public static readonly ElementType[] ElementTypes = #if NET6_0_OR_GREATER Enum.GetValues(); - #else + #else (ElementType[]) Enum.GetValues(typeof(ElementType)); #endif @@ -145,9 +145,15 @@ namespace MLEM.Ui.Parsers { throw new NullReferenceException("A UI parser requires a GraphicsDevice for parsing images"); TextureRegion image = null; - LoadImageAsync(); return new Image(Anchor.AutoLeft, new Vector2(1, -1), _ => image) { - OnDisposed = e => image?.Texture.Dispose() + OnAddedToUi = e => { + if (image == null) + LoadImageAsync(); + }, + OnRemovedFromUi = e => { + image?.Texture.Dispose(); + image = null; + } }; async void LoadImageAsync() { diff --git a/MLEM/Graphics/StaticSpriteBatch.cs b/MLEM/Graphics/StaticSpriteBatch.cs index a707db7..4879427 100644 --- a/MLEM/Graphics/StaticSpriteBatch.cs +++ b/MLEM/Graphics/StaticSpriteBatch.cs @@ -411,7 +411,6 @@ namespace MLEM.Graphics { this.indices?.Dispose(); foreach (var buffer in this.vertexBuffers) buffer.Dispose(); - GC.SuppressFinalize(this); } private Item Add(Texture2D texture, Vector2 pos, Vector2 offset, Vector2 size, float sin, float cos, Color color, Vector2 texTl, Vector2 texBr, float depth) {