Merge pull request #25254 from ZehMatt/fix-25201

Fix #25201: Maintain ride list sort order, apply natural sorting

distribution/changelog.txt

@@ -22,6 +22,7 @@
 - Fix: [#25163] Some of the Junior Roller Coaster flat to steep track wooden support clearance heights are different to RCT1.
 - Fix: [#25173] Desync when placing a park entrance in multiplayer.
 - Fix: [#25179] The LIM Launched Roller Coaster inline twists have incorrect wooden support clearance heights (original bug).
+- Fix: [#25201] Ride list sort order can be unstable when sorted in descending order, change the order for unknown popularity and satisfaction to be last not first.
 - Fix: [#25207] Building a block brake on an LIM coaster does not automatically switch it to powered launch block sectioned mode.
 - Fix: [#25238] The chance of thunder and lightning effects happening is lower than vanilla.
 

src/openrct2-ui/windows/RideList.cpp

@@ -983,6 +983,11 @@ namespace OpenRCT2::Ui::Windows
 
         void SortList()
         {
+            // Maintain stability by first sorting by ride id.
+            SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
+                return thisRide.id.ToUnderlying() < otherRide.id.ToUnderlying();
+            });
+
             switch (listInformationType)
             {
                 case INFORMATION_TYPE_STATUS:
@@ -990,77 +995,76 @@ namespace OpenRCT2::Ui::Windows
                     break;
                 case INFORMATION_TYPE_POPULARITY:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.popularity * 4 <= otherRide.popularity * 4;
+                        return static_cast<int8_t>(thisRide.popularity) < static_cast<int8_t>(otherRide.popularity);
                     });
                     break;
                 case INFORMATION_TYPE_SATISFACTION:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.satisfaction * 5 <= otherRide.satisfaction * 5;
+                        return static_cast<int8_t>(thisRide.satisfaction) < static_cast<int8_t>(otherRide.satisfaction);
                     });
                     break;
                 case INFORMATION_TYPE_PROFIT:
-                    SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.profit <= otherRide.profit;
-                    });
+                    SortListByPredicate(
+                        [](const Ride& thisRide, const Ride& otherRide) -> bool { return thisRide.profit < otherRide.profit; });
                     break;
                 case INFORMATION_TYPE_TOTAL_CUSTOMERS:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.totalCustomers <= otherRide.totalCustomers;
+                        return thisRide.totalCustomers < otherRide.totalCustomers;
                     });
                     break;
                 case INFORMATION_TYPE_TOTAL_PROFIT:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.totalProfit <= otherRide.totalProfit;
+                        return thisRide.totalProfit < otherRide.totalProfit;
                     });
                     break;
                 case INFORMATION_TYPE_CUSTOMERS:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return RideCustomersPerHour(thisRide) <= RideCustomersPerHour(otherRide);
+                        return RideCustomersPerHour(thisRide) < RideCustomersPerHour(otherRide);
                     });
                     break;
                 case INFORMATION_TYPE_AGE:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.buildDate <= otherRide.buildDate;
+                        return thisRide.buildDate < otherRide.buildDate;
                     });
                     break;
                 case INFORMATION_TYPE_INCOME:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.incomePerHour <= otherRide.incomePerHour;
+                        return thisRide.incomePerHour < otherRide.incomePerHour;
                     });
                     break;
                 case INFORMATION_TYPE_RUNNING_COST:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.upkeepCost <= otherRide.upkeepCost;
+                        return thisRide.upkeepCost < otherRide.upkeepCost;
                     });
                     break;
                 case INFORMATION_TYPE_QUEUE_LENGTH:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.getTotalQueueLength() <= otherRide.getTotalQueueLength();
+                        return thisRide.getTotalQueueLength() < otherRide.getTotalQueueLength();
                     });
                     break;
                 case INFORMATION_TYPE_QUEUE_TIME:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.getMaxQueueTime() <= otherRide.getMaxQueueTime();
+                        return thisRide.getMaxQueueTime() < otherRide.getMaxQueueTime();
                     });
                     break;
                 case INFORMATION_TYPE_RELIABILITY:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.reliabilityPercentage <= otherRide.reliabilityPercentage;
+                        return thisRide.reliabilityPercentage < otherRide.reliabilityPercentage;
                     });
                     break;
                 case INFORMATION_TYPE_DOWN_TIME:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.downtime <= otherRide.downtime;
+                        return thisRide.downtime < otherRide.downtime;
                     });
                     break;
                 case INFORMATION_TYPE_LAST_INSPECTION:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.lastInspection <= otherRide.lastInspection;
+                        return thisRide.lastInspection < otherRide.lastInspection;
                     });
                     break;
                 case INFORMATION_TYPE_GUESTS_FAVOURITE:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
-                        return thisRide.guestsFavourite <= otherRide.guestsFavourite;
+                        return thisRide.guestsFavourite < otherRide.guestsFavourite;
                     });
                     break;
                 case INFORMATION_TYPE_EXCITEMENT:
@@ -1068,7 +1072,7 @@ namespace OpenRCT2::Ui::Windows
                         const auto leftValue = thisRide.ratings.isNull() ? RideRating::kUndefined : thisRide.ratings.excitement;
                         const auto rightValue = otherRide.ratings.isNull() ? RideRating::kUndefined
                                                                            : otherRide.ratings.excitement;
-                        return leftValue <= rightValue;
+                        return leftValue < rightValue;
                     });
                     break;
                 case INFORMATION_TYPE_INTENSITY:
@@ -1076,14 +1080,14 @@ namespace OpenRCT2::Ui::Windows
                         const auto leftValue = thisRide.ratings.isNull() ? RideRating::kUndefined : thisRide.ratings.intensity;
                         const auto rightValue = otherRide.ratings.isNull() ? RideRating::kUndefined
                                                                            : otherRide.ratings.intensity;
-                        return leftValue <= rightValue;
+                        return leftValue < rightValue;
                     });
                     break;
                 case INFORMATION_TYPE_NAUSEA:
                     SortListByPredicate([](const Ride& thisRide, const Ride& otherRide) -> bool {
                         const auto leftValue = thisRide.ratings.isNull() ? RideRating::kUndefined : thisRide.ratings.nausea;
                         const auto rightValue = otherRide.ratings.isNull() ? RideRating::kUndefined : otherRide.ratings.nausea;
-                        return leftValue <= rightValue;
+                        return leftValue < rightValue;
                     });
                     break;
             }

src/openrct2/core/String.cpp

@@ -722,7 +722,7 @@ namespace OpenRCT2::String
         return escaped.str();
     }
 
-    /* Case insensitive logical compare */
+    /* Case insensitive logical compare, produces the same output as Notepad++ lexicographical sort */
     // Example:
     // - Guest 10
     // - Guest 99
@@ -731,34 +731,88 @@ namespace OpenRCT2::String
     // - John v2.1
     int32_t logicalCmp(const char* s1, const char* s2)
     {
-        for (;;)
+        const auto isDigit = [](char c) { return std::isdigit(static_cast<unsigned char>(c)); };
+        const auto toUpper = [](char c) { return std::toupper(static_cast<unsigned char>(c)); };
+
+        // Prioritise strings starting with digits
+        bool s1StartsDigit = isDigit(*s1);
+        bool s2StartsDigit = isDigit(*s2);
+        if (s1StartsDigit && !s2StartsDigit)
         {
-            if (*s2 == '\0')
-                return *s1 != '\0';
-            if (*s1 == '\0')
-                return -1;
-            if (!(isdigit(static_cast<unsigned char>(*s1)) && isdigit(static_cast<unsigned char>(*s2))))
-            {
-                if (toupper(*s1) != toupper(*s2))
-                    return toupper(*s1) - toupper(*s2);
-
-                ++s1;
-                ++s2;
-            }
-            else
-            {
-                char *lim1, *lim2;
-                unsigned long n1 = strtoul(s1, &lim1, 10);
-                unsigned long n2 = strtoul(s2, &lim2, 10);
-                if (n1 > n2)
-                    return 1;
-                if (n1 < n2)
-                    return -1;
-
-                s1 = lim1;
-                s2 = lim2;
-            }
+            return -1; // s1 (starts with digit) comes before s2
         }
+        if (!s1StartsDigit && s2StartsDigit)
+        {
+            return 1; // s2 (starts with digit) comes before s1
+        }
+
+        // If both start with digits, compare lexicographically
+        if (s1StartsDigit && s2StartsDigit)
+        {
+            while (*s1 != '\0' && *s2 != '\0')
+            {
+                char c1 = toUpper(*s1);
+                char c2 = toUpper(*s2);
+                if (c1 != c2)
+                {
+                    return c1 - c2;
+                }
+                s1++;
+                s2++;
+            }
+            return *s1 == '\0' ? (*s2 == '\0' ? 0 : -1) : 1;
+        }
+
+        while (*s1 != '\0' && *s2 != '\0')
+        {
+            // Check if both characters are digits
+            if (isDigit(*s1) && isDigit(*s2))
+            {
+                // Skip leading zeros
+                while (*s1 == '0' && isDigit(*(s1 + 1)))
+                    s1++;
+                while (*s2 == '0' && isDigit(*(s2 + 1)))
+                    s2++;
+
+                unsigned long long num1 = 0, num2 = 0;
+                const char* p1 = s1;
+                const char* p2 = s2;
+
+                while (isDigit(*p1))
+                {
+                    num1 = num1 * 10 + (*p1 - '0');
+                    p1++;
+                }
+                while (isDigit(*p2))
+                {
+                    num2 = num2 * 10 + (*p2 - '0');
+                    p2++;
+                }
+
+                if (num1 != num2)
+                {
+                    return num1 < num2 ? -1 : 1;
+                }
+
+                s1 = p1;
+                s2 = p2;
+
+                continue;
+            }
+
+            // Compare non-digit characters case-insensitively
+            char c1 = toUpper(*s1);
+            char c2 = toUpper(*s2);
+            if (c1 != c2)
+            {
+                return c1 - c2;
+            }
+
+            s1++;
+            s2++;
+        }
+
+        return *s1 == '\0' ? (*s2 == '\0' ? 0 : -1) : 1;
     }
 
     char* safeUtf8Copy(char* destination, const char* source, size_t size)

test/tests/StringTest.cpp

@@ -10,6 +10,7 @@
 #include "AssertHelpers.hpp"
 #include "helpers/StringHelpers.hpp"
 
+#include <algorithm>
 #include <gtest/gtest.h>
 #include <openrct2/core/CodepointView.hpp>
 #include <openrct2/core/EnumUtils.hpp>
@@ -17,6 +18,7 @@
 #include <string>
 #include <tuple>
 #include <utility>
+#include <vector>
 
 using namespace OpenRCT2;
 
@@ -250,3 +252,38 @@ TEST_F(CodepointViewTest, CodepointView_iterate)
     AssertCodepoints("ゲスト", { U'ゲ', U'ス', U'ト' });
     AssertCodepoints("<🎢>", { U'<', U'🎢', U'>' });
 }
+
+TEST_F(StringTest, LogicalCompare)
+{
+    std::vector<std::string> expected = {
+        "1001 Troubles", "3D Cinema 1", "Aerial Cycles", "Batflyer",  "bpb",
+        "bpb.sv6",       "Drive-by",    "foo",           "foobar",    "Guest 10",
+        "Guest 99",      "Guest 100",   "John v2.0",     "John v2.1", "River of the Damned",
+        "Terror-dactyl",
+    };
+
+    std::vector<std::string> inputs = {
+        "Guest 99",
+        "Batflyer",
+        "John v2.1",
+        "bpb",
+        "3D Cinema 1",
+        "Drive-by",
+        "John v2.0",
+        "Guest 10",
+        "Terror-dactyl",
+        "Aerial Cycles",
+        "foobar",
+        "1001 Troubles",
+        "River of the Damned",
+        "bpb.sv6",
+        "Guest 100",
+        "foo",
+    };
+
+    std::sort(inputs.begin(), inputs.end(), [](const auto& a, const auto& b) {
+        return String::logicalCmp(a.c_str(), b.c_str()) < 0;
+    });
+
+    AssertVector<std::string>(inputs, expected);
+}