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

@@ -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)
+                this.scrollBarMaxHistory[0] = this.scrollBarMaxHistory[1];
-                if (!this.scrollBarMaxHistory[0].Equals(this.scrollBarMaxHistory[2], Element.Epsilon) || !this.scrollBarMaxHistory[1].Equals(scrollBarMax, Element.Epsilon)) {
+                this.scrollBarMaxHistory[1] = this.scrollBarMaxHistory[2];
-                    this.scrollBarMaxHistory[0] = this.scrollBarMaxHistory[1];
+                this.scrollBarMaxHistory[2] = scrollBarMax;
-                    this.scrollBarMaxHistory[1] = this.scrollBarMaxHistory[2];
-                    this.scrollBarMaxHistory[2] = scrollBarMax;
 
-                    this.ScrollBar.MaxValue = scrollBarMax;
+                this.ScrollBar.MaxValue = scrollBarMax;
-                    this.relevantChildrenDirty = true;
+                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;
+            }
+        }
+
     }
 }

Sandbox/GameImpl.cs

@@ -70,7 +70,7 @@ public class GameImpl : MlemGame {
         //var font = new GenericBitmapFont(LoadContent<BitmapFont>("Fonts/Regular"));
         var font = new GenericStashFont(system.GetFont(32));
         var spriteFont = new GenericSpriteFont(MlemGame.LoadContent<SpriteFont>("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)

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<SpriteFont>(
 #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() {

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;