From dbd52d5b9d1c3b113860a6bb42e7f1728f5ce8d8 Mon Sep 17 00:00:00 2001 From: Ell Date: Tue, 29 Oct 2024 23:31:49 +0100 Subject: [PATCH] Fixed newly introduced issues since #28 (#30) * fixed newly introduced stack overflow when adding/removing from scrolling panel * added second part of issue as test * fixed TestIssue29Inconsistencies --- MLEM.Ui/Elements/Panel.cs | 34 +++++++++------ Sandbox/GameImpl.cs | 33 ++++++++++++-- Tests/TestGame.cs | 8 +++- Tests/UiTests.cs | 91 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 144 insertions(+), 22 deletions(-) diff --git a/MLEM.Ui/Elements/Panel.cs b/MLEM.Ui/Elements/Panel.cs index a8fc091..9082d2d 100644 --- a/MLEM.Ui/Elements/Panel.cs +++ b/MLEM.Ui/Elements/Panel.cs @@ -82,6 +82,9 @@ namespace MLEM.Ui.Elements { this.CanBeSelected = false; if (scrollOverflow) { + this.scrollBarMaxHistory = new float[3]; + this.ResetScrollBarMaxHistory(); + this.ScrollBar = new ScrollBar(Anchor.TopRight, Vector2.Zero, 0, 0) { OnValueChanged = (element, value) => this.ScrollChildren(), CanAutoAnchorsAttach = false, @@ -98,10 +101,6 @@ namespace MLEM.Ui.Elements { this.ScrollToElement(e); }; this.AddChild(this.ScrollBar); - - this.scrollBarMaxHistory = new float[3]; - for (var i = 0; i < this.scrollBarMaxHistory.Length; i++) - this.scrollBarMaxHistory[i] = -1; } } @@ -150,6 +149,8 @@ namespace MLEM.Ui.Elements { throw new NotSupportedException("A panel that scrolls overflow cannot have its scroll bar removed from its list of children"); base.RemoveChild(element); + this.ResetScrollBarMaxHistory(); + // when removing children, our scroll bar might have to be hidden // if we don't do this before adding children again, they might incorrectly assume that the scroll bar will still be visible and adjust their size accordingly this.childrenDirtyForScroll = true; @@ -161,6 +162,8 @@ namespace MLEM.Ui.Elements { if (this.childrenDirtyForScroll && this.System != null) this.ScrollSetup(); + this.ResetScrollBarMaxHistory(); + return base.AddChild(element, index); } @@ -324,16 +327,16 @@ namespace MLEM.Ui.Elements { // the max value of the scroll bar is the amount of non-scaled pixels taken up by overflowing components var scrollBarMax = Math.Max(0, (childrenHeight - this.ChildPaddedArea.Height) / this.Scale); + // avoid an infinite show/hide oscillation that occurs while updating our area by simply using the maximum recent height in that case + if (this.scrollBarMaxHistory[0].Equals(this.scrollBarMaxHistory[2], Element.Epsilon) && this.scrollBarMaxHistory[1].Equals(scrollBarMax, Element.Epsilon)) + scrollBarMax = Math.Max(scrollBarMax, this.scrollBarMaxHistory.Max()); if (!this.ScrollBar.MaxValue.Equals(scrollBarMax, Element.Epsilon)) { - // avoid a show/hide oscillation that occurs while updating our area with children that can lose height when the scroll bar is shown (like long paragraphs) - if (!this.scrollBarMaxHistory[0].Equals(this.scrollBarMaxHistory[2], Element.Epsilon) || !this.scrollBarMaxHistory[1].Equals(scrollBarMax, Element.Epsilon)) { - this.scrollBarMaxHistory[0] = this.scrollBarMaxHistory[1]; - this.scrollBarMaxHistory[1] = this.scrollBarMaxHistory[2]; - this.scrollBarMaxHistory[2] = scrollBarMax; + this.scrollBarMaxHistory[0] = this.scrollBarMaxHistory[1]; + this.scrollBarMaxHistory[1] = this.scrollBarMaxHistory[2]; + this.scrollBarMaxHistory[2] = scrollBarMax; - this.ScrollBar.MaxValue = scrollBarMax; - this.relevantChildrenDirty = true; - } + this.ScrollBar.MaxValue = scrollBarMax; + this.relevantChildrenDirty = true; } // update child padding based on whether the scroll bar is visible @@ -415,5 +418,12 @@ namespace MLEM.Ui.Elements { this.relevantChildrenDirty = true; } + private void ResetScrollBarMaxHistory() { + if (this.scrollOverflow) { + for (var i = 0; i < this.scrollBarMaxHistory.Length; i++) + this.scrollBarMaxHistory[i] = -1; + } + } + } } diff --git a/Sandbox/GameImpl.cs b/Sandbox/GameImpl.cs index 0431ac3..711d05f 100644 --- a/Sandbox/GameImpl.cs +++ b/Sandbox/GameImpl.cs @@ -70,7 +70,7 @@ public class GameImpl : MlemGame { //var font = new GenericBitmapFont(LoadContent("Fonts/Regular")); var font = new GenericStashFont(system.GetFont(32)); var spriteFont = new GenericSpriteFont(MlemGame.LoadContent("Fonts/TestFont")); - this.UiSystem.Style = new UntexturedStyle(this.SpriteBatch) { + /*this.UiSystem.Style = new UntexturedStyle(this.SpriteBatch) { Font = font, TextScale = 0.5F, PanelTexture = new NinePatch(new TextureRegion(tex, 0, 8, 24, 24), 8), @@ -78,7 +78,7 @@ public class GameImpl : MlemGame { }; this.UiSystem.AutoScaleReferenceSize = new Point(1280, 720); this.UiSystem.AutoScaleWithScreen = true; - this.UiSystem.GlobalScale = 5; + this.UiSystem.GlobalScale = 5;*/ /*this.OnDraw += (g, time) => { const string strg = "This is a test string\nto test things\n\nMany things are being tested, like the ability\nfor this font to agree\n\nwith newlines"; @@ -398,7 +398,7 @@ public class GameImpl : MlemGame { Console.WriteLine("Buttons: " + string.Join(", ", GenericInput.AllButtons)); Console.WriteLine("Inputs: " + string.Join(", ", GenericInput.AllInputs));*/ - var hsv = new Panel(Anchor.Center, new Vector2(100), true); + /*var hsv = new Panel(Anchor.Center, new Vector2(100), true); var color = Color.Pink.ToHsv(); hsv.AddChild(new Paragraph(Anchor.AutoLeft, 1, "H")); hsv.AddChild(new Slider(Anchor.AutoLeft, new Vector2(1, 10), 5, 1) { @@ -418,9 +418,27 @@ public class GameImpl : MlemGame { hsv.AddChild(new Group(Anchor.AutoLeft, new Vector2(1, 40), false) { OnDrawn = (e, _, batch, _, _) => batch.Draw(batch.GetBlankTexture(), e.DisplayArea, ColorHelper.FromHsv(color)) }); - this.UiSystem.Add("HSV", hsv); + this.UiSystem.Add("HSV", hsv);*/ + + var group = new SquishingGroup(Anchor.TopLeft, Vector2.One); + var root = this.UiSystem.Add("UI", group); + + var centerGroup = new ScissorGroup(Anchor.TopCenter, Vector2.One); + var centerPanel = new Panel(Anchor.TopRight, Vector2.One); + centerPanel.DrawColor = Color.Red; + centerPanel.Padding = new MLEM.Maths.Padding(5); + centerGroup.AddChild(centerPanel); + group.AddChild(centerGroup); + + this.listView = new Panel(Anchor.TopLeft, new Vector2(1, 1), false, true); + group.AddChild(this.listView); + + var bottomPane = new Panel(Anchor.BottomCenter, new Vector2(1, 500)); + group.AddChild(bottomPane); } + private Panel listView; + protected override void DoUpdate(GameTime gameTime) { base.DoUpdate(gameTime); if (this.InputHandler.IsPressed(Keys.F11)) @@ -431,6 +449,13 @@ public class GameImpl : MlemGame { this.camera.Zoom(0.1F * Math.Sign(delta), this.InputHandler.ViewportMousePosition.ToVector2()); } + if (this.InputHandler.TryConsumePressed(Keys.Space)) { + var c = new Panel(Anchor.AutoLeft, new Vector2(1, 50), false); + c.DrawColor = Color.Green; + c.Padding = new MLEM.Maths.Padding(5); + this.listView.AddChild(c); + } + /*if (Input.InputsDown.Length > 0) Console.WriteLine("Down: " + string.Join(", ", Input.InputsDown));*/ /*if (MlemGame.Input.InputsPressed.Length > 0) diff --git a/Tests/TestGame.cs b/Tests/TestGame.cs index eb306a5..ff16fca 100644 --- a/Tests/TestGame.cs +++ b/Tests/TestGame.cs @@ -1,4 +1,7 @@ -using Microsoft.Xna.Framework; +using System; +using System.Diagnostics; +using System.Runtime.CompilerServices; +using Microsoft.Xna.Framework; using Microsoft.Xna.Framework.Graphics; using MLEM.Data.Content; using MLEM.Font; @@ -24,7 +27,6 @@ public class TestGame : MlemGame { // make sure that the viewport is always the same size, since RunOneFrame doesn't ensure window size is correct this.UiSystem.Viewport = new Rectangle(0, 0, 1280, 720); - // we use precompiled fonts and kni uses a different asset compilation system, so we just have both stored this.UiSystem.Style.Font = new GenericSpriteFont(MlemGame.LoadContent( #if KNI @@ -33,6 +35,8 @@ public class TestGame : MlemGame { "TestFont" #endif )); + // make sure we catch a potential ui stack overflow as part of the tests by ensuring a sufficient execution stack + this.UiSystem.OnElementAreaUpdated += _ => RuntimeHelpers.EnsureSufficientExecutionStack(); } public static TestGame Create() { diff --git a/Tests/UiTests.cs b/Tests/UiTests.cs index d6f92a5..c5fd621 100644 --- a/Tests/UiTests.cs +++ b/Tests/UiTests.cs @@ -1,6 +1,7 @@ using System; using System.Diagnostics; using System.Linq; +using System.Runtime.CompilerServices; using Microsoft.Xna.Framework; using MLEM.Maths; using MLEM.Ui; @@ -147,10 +148,9 @@ public class UiTests : GameTestFixture { } } + // Stack overflow related to panel scrolling and scrollbar auto-hiding [Test] public void TestIssue27([Values(5, 50, 15)] int numChildren) { - // Stack overflow related to panel scrolling and scrollbar auto-hiding - var group = new SquishingGroup(Anchor.TopLeft, Vector2.One); var centerGroup = new ScissorGroup(Anchor.TopCenter, Vector2.One); @@ -172,7 +172,86 @@ public class UiTests : GameTestFixture { var bottomPane = new Panel(Anchor.BottomCenter, new Vector2(1, 500)); group.AddChild(bottomPane); - this.AddAndUpdate(group, out _, out _); + Assert.DoesNotThrow(() => this.AddAndUpdate(group, out _, out _)); + } + + // Removing and re-adding to a scrolling panel causes a stack overflow + [Test] + public void TestIssue29StackOverflow([Values(5, 50, 15)] int numChildren) { + var group = new SquishingGroup(Anchor.TopLeft, Vector2.One); + + var centerGroup = new ScissorGroup(Anchor.TopCenter, Vector2.One); + var centerPanel = new Panel(Anchor.TopRight, Vector2.One); + centerPanel.DrawColor = Color.Red; + centerPanel.Padding = new MLEM.Maths.Padding(5); + centerGroup.AddChild(centerPanel); + group.AddChild(centerGroup); + + var leftColumn = new SquishingGroup(Anchor.TopLeft, new Vector2(500, 1)); + group.AddChild(leftColumn); + var namePanel = new Panel(Anchor.TopLeft, new Vector2(1, 50), true); + var test = new Panel(Anchor.TopCenter, new Vector2(1, 30)); + test.DrawColor = Color.Red; + namePanel.AddChild(test); + var listView = new Panel(Anchor.TopLeft, new Vector2(1, 1), false, true); + leftColumn.AddChild(listView); + leftColumn.AddChild(namePanel); + + var bottomPane = new Panel(Anchor.BottomCenter, new Vector2(1, 500)); + group.AddChild(bottomPane); + + Repopulate(); + Assert.DoesNotThrow(() => this.AddAndUpdate(group, out _, out _)); + Repopulate(); + Assert.DoesNotThrow(() => UiTests.ForceUpdate(group, out _)); + + void Repopulate() { + listView.RemoveChildren(); + for (var i = 0; i < numChildren; i++) { + var c = new Panel(Anchor.AutoLeft, new Vector2(1, 30)); + c.DrawColor = Color.Green; + c.Padding = new MLEM.Maths.Padding(5); + listView.AddChild(c); + } + } + } + + // Adding children causes the scroll bar to disappear when it shouldn't + [Test] + public void TestIssue29Inconsistencies() { + var group = new SquishingGroup(Anchor.TopLeft, Vector2.One); + + var centerGroup = new ScissorGroup(Anchor.TopCenter, Vector2.One); + var centerPanel = new Panel(Anchor.TopRight, Vector2.One); + centerPanel.DrawColor = Color.Red; + centerPanel.Padding = new MLEM.Maths.Padding(5); + centerGroup.AddChild(centerPanel); + group.AddChild(centerGroup); + + var listView = new Panel(Anchor.TopLeft, new Vector2(1, 1), false, true); + group.AddChild(listView); + + var bottomPane = new Panel(Anchor.BottomCenter, new Vector2(1, 500)); + group.AddChild(bottomPane); + + Assert.DoesNotThrow(() => this.AddAndUpdate(group, out _, out _)); + + var appeared = false; + for (var i = 0; i < 100; i++) { + var c = new Panel(Anchor.AutoLeft, new Vector2(1, 50)); + c.DrawColor = Color.Green; + c.Padding = new MLEM.Maths.Padding(5); + listView.AddChild(c); + Console.WriteLine($"Adding child, up to {i}"); + + Assert.DoesNotThrow(() => UiTests.ForceUpdate(group, out _)); + if (appeared) { + Assert.False(listView.ScrollBar.IsHidden, $"Fail bar was hidden after {i} children"); + } else if (!listView.ScrollBar.IsHidden) { + appeared = true; + } + } + Assert.True(appeared, "Scroll bar never appeared"); } private void AddAndUpdate(Element element, out TimeSpan addTime, out TimeSpan updateTime) { @@ -185,7 +264,11 @@ public class UiTests : GameTestFixture { stopwatch.Stop(); addTime = stopwatch.Elapsed; - stopwatch.Restart(); + UiTests.ForceUpdate(element, out updateTime); + } + + private static void ForceUpdate(Element element, out TimeSpan updateTime) { + var stopwatch = Stopwatch.StartNew(); element.ForceUpdateArea(); stopwatch.Stop(); updateTime = stopwatch.Elapsed;