Aggressively check for valid columns before iterating endlessly

osu.Game.Rulesets.Mania/Beatmaps/Patterns/Legacy/DistanceObjectPatternGenerator.cs

@@ -173,26 +173,18 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
             var pattern = new Pattern();
 
             int usableColumns = TotalColumns - RandomStart - PreviousPattern.ColumnWithObjects;
-            int nextColumn = Random.Next(RandomStart, TotalColumns);
+            int nextColumn = GetRandomColumn();
             for (int i = 0; i < Math.Min(usableColumns, noteCount); i++)
             {
                 // Find available column
-                RunWhile(() => pattern.ColumnHasObject(nextColumn) || PreviousPattern.ColumnHasObject(nextColumn), () =>
+                nextColumn = FindAvailableColumn(nextColumn, pattern, PreviousPattern);
-                {
-                    nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
-
                 addToPattern(pattern, nextColumn, startTime, EndTime);
             }
 
             // This is can't be combined with the above loop due to RNG
             for (int i = 0; i < noteCount - usableColumns; i++)
             {
-                RunWhile(() => pattern.ColumnHasObject(nextColumn), () =>
+                nextColumn = FindAvailableColumn(nextColumn, pattern);
-                {
-                    nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
-
                 addToPattern(pattern, nextColumn, startTime, EndTime);
             }
 
@@ -217,23 +209,13 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
 
             int nextColumn = GetColumn((HitObject as IHasXPosition)?.X ?? 0, true);
             if (convertType.HasFlag(PatternType.ForceNotStack) && PreviousPattern.ColumnWithObjects < TotalColumns)
-            {
+                nextColumn = FindAvailableColumn(nextColumn, PreviousPattern);
-                RunWhile(() => PreviousPattern.ColumnHasObject(nextColumn), () =>
-                {
-                    nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
-            }
 
             int lastColumn = nextColumn;
             for (int i = 0; i < noteCount; i++)
             {
                 addToPattern(pattern, nextColumn, startTime, startTime);
-
+                nextColumn = FindAvailableColumn(nextColumn, validation: c => c != lastColumn);
-                RunWhile(() => nextColumn == lastColumn, () =>
-                {
-                    nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
-
                 lastColumn = nextColumn;
                 startTime += SegmentDuration;
             }
@@ -325,7 +307,7 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
                 if (TotalColumns > 2)
                     addToPattern(pattern, nextColumn, startTime, startTime);
 
-                nextColumn = Random.Next(RandomStart, TotalColumns);
+                nextColumn = GetRandomColumn();
                 startTime += SegmentDuration;
             }
 
@@ -404,20 +386,11 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
 
             int nextColumn = GetColumn((HitObject as IHasXPosition)?.X ?? 0, true);
             if (convertType.HasFlag(PatternType.ForceNotStack) && PreviousPattern.ColumnWithObjects < TotalColumns)
-            {
+                nextColumn = FindAvailableColumn(nextColumn, PreviousPattern);
-                RunWhile(() => PreviousPattern.ColumnHasObject(nextColumn), () =>
-                {
-                    nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
-            }
 
             for (int i = 0; i < columnRepeat; i++)
             {
-                RunWhile(() => pattern.ColumnHasObject(nextColumn), () =>
+                nextColumn = FindAvailableColumn(nextColumn, pattern);
-                {
-                    nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
-
                 addToPattern(pattern, nextColumn, startTime, EndTime);
                 startTime += SegmentDuration;
             }
@@ -442,17 +415,12 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
 
             int holdColumn = GetColumn((HitObject as IHasXPosition)?.X ?? 0, true);
             if (convertType.HasFlag(PatternType.ForceNotStack) && PreviousPattern.ColumnWithObjects < TotalColumns)
-            {
+                holdColumn = FindAvailableColumn(holdColumn, PreviousPattern);
-                RunWhile(() => PreviousPattern.ColumnHasObject(holdColumn), () =>
-                {
-                    holdColumn = Random.Next(RandomStart, TotalColumns);
-                });
-            }
 
             // Create the hold note
             addToPattern(pattern, holdColumn, startTime, EndTime);
 
-            int nextColumn = Random.Next(RandomStart, TotalColumns);
+            int nextColumn = GetRandomColumn();
             int noteCount;
             if (ConversionDifficulty > 6.5)
                 noteCount = GetRandomNoteCount(0.63, 0);
@@ -473,11 +441,7 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
                 {
                     for (int j = 0; j < noteCount; j++)
                     {
-                        RunWhile(() => rowPattern.ColumnHasObject(nextColumn) || nextColumn == holdColumn, () =>
+                        nextColumn = FindAvailableColumn(nextColumn, validation: c => c != holdColumn, patterns: rowPattern);
-                        {
-                            nextColumn = Random.Next(RandomStart, TotalColumns);
-                        });
-
                         addToPattern(rowPattern, nextColumn, startTime, startTime);
                     }
                 }

osu.Game.Rulesets.Mania/Beatmaps/Patterns/Legacy/EndTimeObjectPatternGenerator.cs

@@ -39,34 +39,17 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
                     addToPattern(pattern, 0, generateHold);
                     break;
                 case 8:
-                    addToPattern(pattern, getNextRandomColumn(RandomStart), generateHold);
+                    addToPattern(pattern, FindAvailableColumn(GetRandomColumn(), PreviousPattern), generateHold);
                     break;
                 default:
                     if (TotalColumns > 0)
-                        addToPattern(pattern, getNextRandomColumn(0), generateHold);
+                        addToPattern(pattern, GetRandomColumn(), generateHold);
                     break;
             }
 
             return pattern;
         }
 
-        /// <summary>
-        /// Picks a random column after a column.
-        /// </summary>
-        /// <param name="start">The starting column.</param>
-        /// <returns>A random column after <paramref name="start"/>.</returns>
-        private int getNextRandomColumn(int start)
-        {
-            int nextColumn = Random.Next(start, TotalColumns);
-
-            RunWhile(() => PreviousPattern.ColumnHasObject(nextColumn), () =>
-            {
-                nextColumn = Random.Next(start, TotalColumns);
-            });
-
-            return nextColumn;
-        }
-
         /// <summary>
         /// Constructs and adds a note to a pattern.
         /// </summary>

osu.Game.Rulesets.Mania/Beatmaps/Patterns/Legacy/HitObjectPatternGenerator.cs

@@ -231,22 +231,27 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
             int nextColumn = GetColumn((HitObject as IHasXPosition)?.X ?? 0, true);
             for (int i = 0; i < noteCount; i++)
             {
-                RunWhile(() => pattern.ColumnHasObject(nextColumn) || PreviousPattern.ColumnHasObject(nextColumn) && !allowStacking, () =>
+                nextColumn = allowStacking
-                {
+                    ? FindAvailableColumn(nextColumn, nextColumn: getNextColumn, patterns: pattern)
-                    if (convertType.HasFlag(PatternType.Gathered))
+                    : FindAvailableColumn(nextColumn, nextColumn: getNextColumn, patterns: new[] { pattern, PreviousPattern });
-                    {
-                        nextColumn++;
-                        if (nextColumn == TotalColumns)
-                            nextColumn = RandomStart;
-                    }
-                    else
-                        nextColumn = Random.Next(RandomStart, TotalColumns);
-                });
 
                 addToPattern(pattern, nextColumn);
             }
 
             return pattern;
+
+            int getNextColumn(int last)
+            {
+                if (convertType.HasFlag(PatternType.Gathered))
+                {
+                    last++;
+                    if (last == TotalColumns)
+                        last = RandomStart;
+                }
+                else
+                    last = GetRandomColumn();
+                return last;
+            }
         }
 
         /// <summary>
@@ -292,13 +297,10 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
             int noteCount = getRandomNoteCountMirrored(centreProbability, p2, p3, out addToCentre);
 
             int columnLimit = (TotalColumns % 2 == 0 ? TotalColumns : TotalColumns - 1) / 2;
-            int nextColumn = Random.Next(RandomStart, columnLimit);
+            int nextColumn = GetRandomColumn(upperBound: columnLimit);
             for (int i = 0; i < noteCount; i++)
             {
-                RunWhile(() => pattern.ColumnHasObject(nextColumn), () =>
+                nextColumn = FindAvailableColumn(nextColumn, upperBound: columnLimit, patterns: pattern);
-                {
-                    nextColumn = Random.Next(RandomStart, columnLimit);
-                });
 
                 // Add normal note
                 addToPattern(pattern, nextColumn);

osu.Game.Rulesets.Mania/Beatmaps/Patterns/Legacy/PatternGenerator.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Linq;
+using JetBrains.Annotations;
 using osu.Game.Beatmaps;
 using osu.Game.Rulesets.Mania.MathUtils;
 using osu.Game.Rulesets.Objects;
@@ -90,6 +91,7 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
         }
 
         private double? conversionDifficulty;
+
         /// <summary>
         /// A difficulty factor used for various conversion methods from osu!stable.
         /// </summary>
@@ -116,5 +118,82 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns.Legacy
                 return conversionDifficulty.Value;
             }
         }
+
+        /// <summary>
+        /// Finds a new column in which a <see cref="HitObject"/> can be placed.
+        /// This uses <see cref="GetRandomColumn"/> to pick the next candidate column.
+        /// </summary>
+        /// <param name="initialColumn">The initial column to test. This may be returned if it is already a valid column.</param>
+        /// <param name="patterns">A list of patterns for which the validity of a column should be checked against.
+        /// A column is not a valid candidate if a <see cref="HitObject"/> occupies the same column in any of the patterns.</param>
+        /// <returns>A column for which there are no <see cref="HitObject"/>s in any of <paramref name="patterns"/> occupying the same column.</returns>
+        /// <exception cref="NotEnoughColumnsException">If there are no valid candidate columns.</exception>
+        protected int FindAvailableColumn(int initialColumn, params Pattern[] patterns)
+            => FindAvailableColumn(initialColumn, null, patterns: patterns);
+
+        /// <summary>
+        /// Finds a new column in which a <see cref="HitObject"/> can be placed.
+        /// </summary>
+        /// <param name="initialColumn">The initial column to test. This may be returned if it is already a valid column.</param>
+        /// <param name="nextColumn">A function to retrieve the next column. If null, a randomisation scheme will be used.</param>
+        /// <param name="validation">A function to perform additional validation checks to determine if a column is a valid candidate for a <see cref="HitObject"/>.</param>
+        /// <param name="lowerBound">The minimum column index. If null, <see cref="RandomStart"/> is used.</param>
+        /// <param name="upperBound">The maximum column index. If null, <see cref="PatternGenerator.TotalColumns"/> is used.</param>
+        /// <param name="patterns">A list of patterns for which the validity of a column should be checked against.
+        /// A column is not a valid candidate if a <see cref="HitObject"/> occupies the same column in any of the patterns.</param>
+        /// <returns>A column which has passed the <paramref name="validation"/> check and for which there are no
+        /// <see cref="HitObject"/>s in any of <paramref name="patterns"/> occupying the same column.</returns>
+        /// <exception cref="NotEnoughColumnsException">If there are no valid candidate columns.</exception>
+        protected int FindAvailableColumn(int initialColumn, int? lowerBound = null, int? upperBound = null, Func<int, int> nextColumn = null, [InstantHandle] Func<int, bool> validation = null,
+                                          params Pattern[] patterns)
+        {
+            lowerBound = lowerBound ?? RandomStart;
+            upperBound = upperBound ?? TotalColumns;
+            nextColumn = nextColumn ?? (_ => GetRandomColumn(lowerBound, upperBound));
+
+            // Check for the initial column
+            if (isValid(initialColumn))
+                return initialColumn;
+
+            // Ensure that we have at least one free column, so that an endless loop is avoided
+            bool hasValidColumns = false;
+            for (int i = lowerBound.Value; i < upperBound.Value; i++)
+            {
+                hasValidColumns = isValid(i);
+                if (hasValidColumns)
+                    break;
+            }
+
+            if (!hasValidColumns)
+                throw new NotEnoughColumnsException();
+
+            // Iterate until a valid column is found. This is a random iteration in the default case.
+            do
+            {
+                initialColumn = nextColumn(initialColumn);
+            } while (!isValid(initialColumn));
+
+            return initialColumn;
+
+            bool isValid(int column) => validation?.Invoke(column) != false && !patterns.Any(p => p.ColumnHasObject(column));
+        }
+
+        /// <summary>
+        /// Returns a random column index in the range [RandomStart, TotalColumns).
+        /// </summary>
+        /// <param name="lowerBound">The minimum column index. If null, <see cref="RandomStart"/> is used.</param>
+        /// <param name="upperBound">The maximum column index. If null, <see cref="PatternGenerator.TotalColumns"/> is used.</param>
+        protected int GetRandomColumn(int? lowerBound = null, int? upperBound = null) => Random.Next(lowerBound ?? RandomStart, upperBound ?? TotalColumns);
+
+        /// <summary>
+        /// Occurs when mania conversion is stuck in an infinite loop unable to find columns to place new hitobjects in.
+        /// </summary>
+        public class NotEnoughColumnsException : Exception
+        {
+            public NotEnoughColumnsException()
+                : base("There were not enough columns to complete conversion.")
+            {
+            }
+        }
     }
 }

osu.Game.Rulesets.Mania/Beatmaps/Patterns/PatternGenerator.cs

@@ -3,9 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
-using JetBrains.Annotations;
-using osu.Framework.Logging;
 using osu.Game.Rulesets.Objects;
 
 namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns
@@ -15,14 +12,6 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns
     /// </summary>
     internal abstract class PatternGenerator
     {
-        /// <summary>
-        /// An arbitrary maximum amount of iterations to perform in <see cref="RunWhile"/>.
-        /// The specific value is not super important - enough such that no false-positives occur.
-        ///
-        /// /b/933228 requires at least 23 iterations.
-        /// </summary>
-        private const int max_rng_iterations = 30;
-
         /// <summary>
         /// The last pattern.
         /// </summary>
@@ -53,44 +42,10 @@ namespace osu.Game.Rulesets.Mania.Beatmaps.Patterns
             TotalColumns = Beatmap.TotalColumns;
         }
 
-        protected void RunWhile([InstantHandle] Func<bool> condition, Action action)
-        {
-            int iterations = 0;
-
-            while (condition())
-            {
-                if (iterations++ >= max_rng_iterations)
-                {
-                    // log an error but don't throw. we want to continue execution.
-                    Logger.Error(new ExceededAllowedIterationsException(new StackTrace(0)),
-                        "Conversion encountered errors. The beatmap may not be correctly converted.");
-                    return;
-                }
-
-                action();
-            }
-        }
-
         /// <summary>
         /// Generates the patterns for <see cref="HitObject"/>, each filled with hit objects.
         /// </summary>
         /// <returns>The <see cref="Pattern"/>s containing the hit objects.</returns>
         public abstract IEnumerable<Pattern> Generate();
-
-        /// <summary>
-        /// Denotes when a single conversion operation is in an infinitely looping state.
-        /// </summary>
-        public class ExceededAllowedIterationsException : Exception
-        {
-            private readonly string stackTrace;
-
-            public ExceededAllowedIterationsException(StackTrace stackTrace)
-            {
-                this.stackTrace = stackTrace.ToString();
-            }
-
-            public override string StackTrace => stackTrace;
-            public override string ToString() => $"{GetType().Name}: {Message}\r\n{StackTrace}";
-        }
     }
 }