Merge pull request #28589 from bdach/scale-handling-crash

Fix crashes when opening scale/rotation popovers during selection box operations
ppy · Jun 25, 2024 · aadb104 · aadb104
2 parents 3d22f70 + d722be1
commit aadb104
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 10 deletions.
diff --git a/osu.Game.Rulesets.Osu/Edit/OsuSelectionRotationHandler.cs b/osu.Game.Rulesets.Osu/Edit/OsuSelectionRotationHandler.cs
@@ -53,9 +53,11 @@ private void updateState()
 
         public override void Begin()
         {
-            if (objectsInRotation != null)
+            if (OperationInProgress.Value)
                 throw new InvalidOperationException($"Cannot {nameof(Begin)} a rotate operation while another is in progress!");
 
+            base.Begin();
+
             changeHandler?.BeginChange();
 
             objectsInRotation = selectedMovableObjects.ToArray();
@@ -68,10 +70,10 @@ public override void Begin()
 
         public override void Update(float rotation, Vector2? origin = null)
         {
-            if (objectsInRotation == null)
+            if (!OperationInProgress.Value)
                 throw new InvalidOperationException($"Cannot {nameof(Update)} a rotate operation without calling {nameof(Begin)} first!");
 
-            Debug.Assert(originalPositions != null && originalPathControlPointPositions != null && defaultOrigin != null);
+            Debug.Assert(objectsInRotation != null && originalPositions != null && originalPathControlPointPositions != null && defaultOrigin != null);
 
             Vector2 actualOrigin = origin ?? defaultOrigin.Value;
 
@@ -91,11 +93,13 @@ public override void Update(float rotation, Vector2? origin = null)
 
         public override void Commit()
         {
-            if (objectsInRotation == null)
+            if (!OperationInProgress.Value)
                 throw new InvalidOperationException($"Cannot {nameof(Commit)} a rotate operation without calling {nameof(Begin)} first!");
 
             changeHandler?.EndChange();
 
+            base.Commit();
+
             objectsInRotation = null;
             originalPositions = null;
             originalPathControlPointPositions = null;

diff --git a/osu.Game.Rulesets.Osu/Edit/OsuSelectionScaleHandler.cs b/osu.Game.Rulesets.Osu/Edit/OsuSelectionScaleHandler.cs
@@ -72,9 +72,11 @@ private void updateState()
 
         public override void Begin()
         {
-            if (objectsInScale != null)
+            if (OperationInProgress.Value)
                 throw new InvalidOperationException($"Cannot {nameof(Begin)} a scale operation while another is in progress!");
 
+            base.Begin();
+
             changeHandler?.BeginChange();
 
             objectsInScale = selectedMovableObjects.ToDictionary(ho => ho, ho => new OriginalHitObjectState(ho));
@@ -86,10 +88,10 @@ public override void Begin()
 
         public override void Update(Vector2 scale, Vector2? origin = null, Axes adjustAxis = Axes.Both)
         {
-            if (objectsInScale == null)
+            if (!OperationInProgress.Value)
                 throw new InvalidOperationException($"Cannot {nameof(Update)} a scale operation without calling {nameof(Begin)} first!");
 
-            Debug.Assert(defaultOrigin != null && OriginalSurroundingQuad != null);
+            Debug.Assert(objectsInScale != null && defaultOrigin != null && OriginalSurroundingQuad != null);
 
             Vector2 actualOrigin = origin ?? defaultOrigin.Value;
 
@@ -117,11 +119,13 @@ public override void Update(Vector2 scale, Vector2? origin = null, Axes adjustAx
 
         public override void Commit()
         {
-            if (objectsInScale == null)
+            if (!OperationInProgress.Value)
                 throw new InvalidOperationException($"Cannot {nameof(Commit)} a rotate operation without calling {nameof(Begin)} first!");
 
             changeHandler?.EndChange();
 
+            base.Commit();
+
             objectsInScale = null;
             OriginalSurroundingQuad = null;
             defaultOrigin = null;

diff --git a/osu.Game.Rulesets.Osu/Edit/TransformToolboxGroup.cs b/osu.Game.Rulesets.Osu/Edit/TransformToolboxGroup.cs
@@ -77,13 +77,15 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
             {
                 case GlobalAction.EditorToggleRotateControl:
                 {
-                    rotateButton.TriggerClick();
+                    if (!RotationHandler.OperationInProgress.Value || rotateButton.Selected.Value)
+                        rotateButton.TriggerClick();
                     return true;
                 }
 
                 case GlobalAction.EditorToggleScaleControl:
                 {
-                    scaleButton.TriggerClick();
+                    if (!ScaleHandler.OperationInProgress.Value || scaleButton.Selected.Value)
+                        scaleButton.TriggerClick();
                     return true;
                 }
             }

diff --git a/osu.Game.Tests/Visual/Editing/TestSceneComposeSelectBox.cs b/osu.Game.Tests/Visual/Editing/TestSceneComposeSelectBox.cs
@@ -84,6 +84,8 @@ public override void Begin()
 
                 targetContainer = getTargetContainer();
                 initialRotation = targetContainer!.Rotation;
+
+                base.Begin();
             }
 
             public override void Update(float rotation, Vector2? origin = null)
@@ -102,6 +104,8 @@ public override void Commit()
 
                 targetContainer = null;
                 initialRotation = null;
+
+                base.Commit();
             }
         }
 

diff --git a/osu.Game/Overlays/SkinEditor/SkinSelectionRotationHandler.cs b/osu.Game/Overlays/SkinEditor/SkinSelectionRotationHandler.cs
@@ -61,6 +61,8 @@ public override void Begin()
             originalRotations = objectsInRotation.ToDictionary(d => d, d => d.Rotation);
             originalPositions = objectsInRotation.ToDictionary(d => d, d => d.ToScreenSpace(d.OriginPosition));
             defaultOrigin = GeometryUtils.GetSurroundingQuad(objectsInRotation.SelectMany(d => d.ScreenSpaceDrawQuad.GetVertices().ToArray())).Centre;
+
+            base.Begin();
         }
 
         public override void Update(float rotation, Vector2? origin = null)
@@ -99,6 +101,8 @@ public override void Commit()
             originalPositions = null;
             originalRotations = null;
             defaultOrigin = null;
+
+            base.Commit();
         }
     }
 }
diff --git a/osu.Game/Screens/Edit/Compose/Components/SelectionBoxRotationHandle.cs b/osu.Game/Screens/Edit/Compose/Components/SelectionBoxRotationHandle.cs
@@ -67,6 +67,9 @@ protected override bool OnDragStart(DragStartEvent e)
 
             if (rotationHandler == null) return false;
 
+            if (rotationHandler.OperationInProgress.Value)
+                return false;
+
             rotationHandler.Begin();
             return true;
         }

diff --git a/osu.Game/Screens/Edit/Compose/Components/SelectionBoxScaleHandle.cs b/osu.Game/Screens/Edit/Compose/Components/SelectionBoxScaleHandle.cs
@@ -32,6 +32,9 @@ protected override bool OnDragStart(DragStartEvent e)
 
             if (scaleHandler == null) return false;
 
+            if (scaleHandler.OperationInProgress.Value)
+                return false;
+
             originalAnchor = Anchor;
 
             scaleHandler.Begin();

diff --git a/osu.Game/Screens/Edit/Compose/Components/SelectionRotationHandler.cs b/osu.Game/Screens/Edit/Compose/Components/SelectionRotationHandler.cs
@@ -12,6 +12,11 @@ namespace osu.Game.Screens.Edit.Compose.Components
     /// </summary>
     public partial class SelectionRotationHandler : Component
     {
+        /// <summary>
+        /// Whether there is any ongoing scale operation right now.
+        /// </summary>
+        public Bindable<bool> OperationInProgress { get; private set; } = new BindableBool();
+
         /// <summary>
         /// Whether rotation anchored by the selection origin can currently be performed.
         /// </summary>
@@ -50,6 +55,7 @@ public void Rotate(float rotation, Vector2? origin = null)
         /// </remarks>
         public virtual void Begin()
         {
+            OperationInProgress.Value = true;
         }
 
         /// <summary>
@@ -85,6 +91,7 @@ public virtual void Update(float rotation, Vector2? origin = null)
         /// </remarks>
         public virtual void Commit()
         {
+            OperationInProgress.Value = false;
         }
     }
 }
diff --git a/osu.Game/Screens/Edit/Compose/Components/SelectionScaleHandler.cs b/osu.Game/Screens/Edit/Compose/Components/SelectionScaleHandler.cs
@@ -13,6 +13,11 @@ namespace osu.Game.Screens.Edit.Compose.Components
     /// </summary>
     public partial class SelectionScaleHandler : Component
     {
+        /// <summary>
+        /// Whether there is any ongoing scale operation right now.
+        /// </summary>
+        public Bindable<bool> OperationInProgress { get; private set; } = new BindableBool();
+
         /// <summary>
         /// Whether horizontal scaling (from the left or right edge) support should be enabled.
         /// </summary>
@@ -63,6 +68,7 @@ public void ScaleSelection(Vector2 scale, Vector2? origin = null, Axes adjustAxi
         /// </remarks>
         public virtual void Begin()
         {
+            OperationInProgress.Value = true;
         }
 
         /// <summary>
@@ -99,6 +105,7 @@ public virtual void Update(Vector2 scale, Vector2? origin = null, Axes adjustAxi
         /// </remarks>
         public virtual void Commit()
         {
+            OperationInProgress.Value = false;
         }
     }
 }