From 7fc49fca39202d04f13aa4c310ef6f1572f9836d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 20 Oct 2021 00:50:34 +0300 Subject: [PATCH 1/4] Replace StringVariant with std::variant --- src/openrct2-ui/windows/RideConstruction.cpp | 2 +- src/openrct2-ui/windows/TopToolbar.cpp | 4 +- src/openrct2/actions/GameActionCompat.cpp | 5 +- src/openrct2/actions/GameActionResult.cpp | 36 ++++++------ src/openrct2/actions/GameActionResult.h | 55 +------------------ src/openrct2/actions/LandSetHeightAction.cpp | 5 +- src/openrct2/actions/LandSmoothAction.cpp | 2 +- src/openrct2/actions/MazeSetTrackAction.cpp | 5 +- .../actions/PlaceParkEntranceAction.cpp | 5 +- src/openrct2/actions/WaterSetHeightAction.cpp | 3 +- src/openrct2/peep/Peep.cpp | 11 ++-- 11 files changed, 38 insertions(+), 95 deletions(-) diff --git a/src/openrct2-ui/windows/RideConstruction.cpp b/src/openrct2-ui/windows/RideConstruction.cpp index d582e02a56..87a07a6d5d 100644 --- a/src/openrct2-ui/windows/RideConstruction.cpp +++ b/src/openrct2-ui/windows/RideConstruction.cpp @@ -1810,7 +1810,7 @@ static void window_ride_construction_construct(rct_window* w) // Used by some functions if (res->Error != GameActions::Status::Ok) { - if (auto error = res->ErrorMessage.AsStringId()) + if (auto* error = std::get_if(&res->ErrorMessage)) gGameCommandErrorText = *error; else gGameCommandErrorText = STR_NONE; diff --git a/src/openrct2-ui/windows/TopToolbar.cpp b/src/openrct2-ui/windows/TopToolbar.cpp index 0fa790d3bb..e9c3ac91d2 100644 --- a/src/openrct2-ui/windows/TopToolbar.cpp +++ b/src/openrct2-ui/windows/TopToolbar.cpp @@ -1905,7 +1905,7 @@ static void window_top_toolbar_scenery_tool_down(const ScreenCoordsXY& windowPos break; } - if (auto message = res->ErrorMessage.AsStringId()) + if (const auto* message = std::get_if(&res->ErrorMessage)) { if (*message == STR_NOT_ENOUGH_CASH_REQUIRES || *message == STR_CAN_ONLY_BUILD_THIS_ON_WATER) { @@ -1958,7 +1958,7 @@ static void window_top_toolbar_scenery_tool_down(const ScreenCoordsXY& windowPos break; } - if (auto message = res->ErrorMessage.AsStringId()) + if (const auto* message = std::get_if(&res->ErrorMessage)) { if (*message == STR_NOT_ENOUGH_CASH_REQUIRES || *message == STR_CAN_ONLY_BUILD_THIS_ON_WATER) { diff --git a/src/openrct2/actions/GameActionCompat.cpp b/src/openrct2/actions/GameActionCompat.cpp index d33c82df83..6aebf1433c 100644 --- a/src/openrct2/actions/GameActionCompat.cpp +++ b/src/openrct2/actions/GameActionCompat.cpp @@ -144,11 +144,12 @@ money32 maze_set_track( // NOTE: ride_construction_tooldown_construct requires them to be set. // Refactor result type once there's no C code referencing this function. - if (auto title = res->ErrorTitle.AsStringId()) + if (const auto* title = std::get_if(&res->ErrorTitle)) gGameCommandErrorTitle = *title; else gGameCommandErrorTitle = STR_NONE; - if (auto message = res->ErrorMessage.AsStringId()) + + if (const auto* message = std::get_if(&res->ErrorMessage)) gGameCommandErrorText = *message; else gGameCommandErrorText = STR_NONE; diff --git a/src/openrct2/actions/GameActionResult.cpp b/src/openrct2/actions/GameActionResult.cpp index b00fc39416..7bdfb75812 100644 --- a/src/openrct2/actions/GameActionResult.cpp +++ b/src/openrct2/actions/GameActionResult.cpp @@ -25,32 +25,28 @@ namespace GameActions std::copy_n(args, ErrorMessageArgs.size(), ErrorMessageArgs.begin()); } + struct StringVariantVisitor + { + const void* ErrorMessageArgs{}; + + std::string operator()(const std::string& str) const + { + return str; + } + std::string operator()(const rct_string_id strId) const + { + return format_string(strId, ErrorMessageArgs); + } + }; + std::string GameActions::Result::GetErrorTitle() const { - std::string title; - if (auto error = ErrorTitle.AsString()) - { - title = *error; - } - else - { - title = format_string(ErrorTitle.GetStringId(), ErrorMessageArgs.data()); - } - return title; + return std::visit(StringVariantVisitor{ ErrorMessageArgs.data() }, ErrorTitle); } std::string GameActions::Result::GetErrorMessage() const { - std::string message; - if (auto error = ErrorMessage.AsString()) - { - message = *error; - } - else - { - message = format_string(ErrorMessage.GetStringId(), ErrorMessageArgs.data()); - } - return message; + return std::visit(StringVariantVisitor{ ErrorMessageArgs.data() }, ErrorMessage); } } // namespace GameActions diff --git a/src/openrct2/actions/GameActionResult.h b/src/openrct2/actions/GameActionResult.h index 9daab74d27..d480dfe698 100644 --- a/src/openrct2/actions/GameActionResult.h +++ b/src/openrct2/actions/GameActionResult.h @@ -19,59 +19,7 @@ #include #include #include - -class StringVariant -{ -private: - rct_string_id StringId = STR_NONE; - std::string String; - -public: - StringVariant() = default; - - StringVariant(rct_string_id stringId) - : StringId(stringId) - { - } - - StringVariant(const std::string& s) - : String(s) - { - } - - StringVariant(std::string&& s) - : String(std::move(s)) - { - } - - StringVariant(const char* s) - : String(s) - { - } - - const std::string* AsString() const - { - if (!String.empty()) - { - return &String; - } - return {}; - } - - const rct_string_id* AsStringId() const - { - if (String.empty()) - { - return &StringId; - } - return {}; - } - - rct_string_id GetStringId() const - { - return String.empty() ? StringId : STR_NONE; - } -}; +#include namespace GameActions { @@ -114,6 +62,7 @@ namespace GameActions { public: using Ptr = std::unique_ptr; + using StringVariant = std::variant; GameActions::Status Error = GameActions::Status::Ok; StringVariant ErrorTitle; diff --git a/src/openrct2/actions/LandSetHeightAction.cpp b/src/openrct2/actions/LandSetHeightAction.cpp index e4cbc8ccbb..02c6c35b02 100644 --- a/src/openrct2/actions/LandSetHeightAction.cpp +++ b/src/openrct2/actions/LandSetHeightAction.cpp @@ -130,9 +130,8 @@ GameActions::Result::Ptr LandSetHeightAction::Query() const CREATE_CROSSING_MODE_NONE); if (clearResult->Error != GameActions::Status::Ok) { - return std::make_unique( - GameActions::Status::Disallowed, STR_NONE, clearResult->ErrorMessage.GetStringId(), - clearResult->ErrorMessageArgs.data()); + clearResult->Error = GameActions::Status::Disallowed; + return clearResult; } tileElement = CheckUnremovableObstructions(reinterpret_cast(surfaceElement), zCorner); diff --git a/src/openrct2/actions/LandSmoothAction.cpp b/src/openrct2/actions/LandSmoothAction.cpp index 0bee3b6253..7e8c432099 100644 --- a/src/openrct2/actions/LandSmoothAction.cpp +++ b/src/openrct2/actions/LandSmoothAction.cpp @@ -608,7 +608,7 @@ GameActions::Result::Ptr LandSmoothAction::SmoothLand(bool isExecuting) const } default: log_error("Invalid map selection %u", _selectionType); - return MakeResult(GameActions::Status::InvalidParameters, res->ErrorTitle.GetStringId()); + return MakeResult(GameActions::Status::InvalidParameters, std::get(res->ErrorTitle)); } // switch selectionType // Raise / lower the land tool selection area diff --git a/src/openrct2/actions/MazeSetTrackAction.cpp b/src/openrct2/actions/MazeSetTrackAction.cpp index 3605a425d7..2d740b4de6 100644 --- a/src/openrct2/actions/MazeSetTrackAction.cpp +++ b/src/openrct2/actions/MazeSetTrackAction.cpp @@ -109,9 +109,8 @@ GameActions::Result::Ptr MazeSetTrackAction::Query() const auto constructResult = MapCanConstructAt({ _loc.ToTileStart(), baseHeight, clearanceHeight }, { 0b1111, 0 }); if (constructResult->Error != GameActions::Status::Ok) { - return MakeResult( - GameActions::Status::NoClearance, res->ErrorTitle.GetStringId(), constructResult->ErrorMessage.GetStringId(), - constructResult->ErrorMessageArgs.data()); + constructResult->ErrorTitle = STR_RIDE_CONSTRUCTION_CANT_CONSTRUCT_THIS_HERE; + return constructResult; } if (constructResult->GroundFlags & ELEMENT_IS_UNDERWATER) diff --git a/src/openrct2/actions/PlaceParkEntranceAction.cpp b/src/openrct2/actions/PlaceParkEntranceAction.cpp index 5ef8207e34..0603218380 100644 --- a/src/openrct2/actions/PlaceParkEntranceAction.cpp +++ b/src/openrct2/actions/PlaceParkEntranceAction.cpp @@ -86,9 +86,8 @@ GameActions::Result::Ptr PlaceParkEntranceAction::Query() const if (auto res2 = MapCanConstructAt({ entranceLoc, zLow, zHigh }, { 0b1111, 0 }); res2->Error != GameActions::Status::Ok) { - return std::make_unique( - GameActions::Status::NoClearance, STR_CANT_BUILD_THIS_HERE, res2->ErrorMessage.GetStringId(), - res2->ErrorMessageArgs.data()); + res2->ErrorTitle = STR_CANT_BUILD_THIS_HERE; + return res2; } // Check that entrance element does not already exist at this location diff --git a/src/openrct2/actions/WaterSetHeightAction.cpp b/src/openrct2/actions/WaterSetHeightAction.cpp index bf4eb860c2..523ab0b358 100644 --- a/src/openrct2/actions/WaterSetHeightAction.cpp +++ b/src/openrct2/actions/WaterSetHeightAction.cpp @@ -85,8 +85,7 @@ GameActions::Result::Ptr WaterSetHeightAction::Query() const if (auto res2 = MapCanConstructAt({ _coords, zLow, zHigh }, { 0b1111, 0b1111 }); res2->Error != GameActions::Status::Ok) { - return MakeResult( - GameActions::Status::NoClearance, STR_NONE, res2->ErrorMessage.GetStringId(), res2->ErrorMessageArgs.data()); + return res2; } if (surfaceElement->HasTrackThatNeedsWater()) { diff --git a/src/openrct2/peep/Peep.cpp b/src/openrct2/peep/Peep.cpp index 620498252c..dd18031c90 100644 --- a/src/openrct2/peep/Peep.cpp +++ b/src/openrct2/peep/Peep.cpp @@ -579,9 +579,10 @@ std::unique_ptr Peep::Place(const TileCoordsXYZ& location, { tileElement = reinterpret_cast(map_get_surface_element_at(location.ToCoordsXYZ())); } - if (tileElement == nullptr) + { return std::make_unique(GameActions::Status::InvalidParameters, STR_ERR_CANT_PLACE_PERSON_HERE); + } // Set the coordinate of destination to be exactly // in the middle of a tile. @@ -595,13 +596,13 @@ std::unique_ptr Peep::Place(const TileCoordsXYZ& location, if (auto res = MapCanConstructAt({ destination, destination.z, destination.z + (1 * 8) }, { 0b1111, 0 }); res->Error != GameActions::Status::Ok) { - if (res->ErrorMessage.GetStringId() != STR_RAISE_OR_LOWER_LAND_FIRST) + const auto stringId = std::get(res->ErrorMessage); + if (stringId != STR_RAISE_OR_LOWER_LAND_FIRST) { - if (res->ErrorMessage.GetStringId() != STR_FOOTPATH_IN_THE_WAY) + if (stringId != STR_FOOTPATH_IN_THE_WAY) { return std::make_unique( - GameActions::Status::NoClearance, STR_ERR_CANT_PLACE_PERSON_HERE, res->ErrorMessage.GetStringId(), - res->ErrorMessageArgs.data()); + GameActions::Status::NoClearance, STR_ERR_CANT_PLACE_PERSON_HERE, stringId, res->ErrorMessageArgs.data()); } } } From 5bf3513156890c1566247121afe431cf6c8a9dee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 20 Oct 2021 01:10:25 +0300 Subject: [PATCH 2/4] Default initialize ErrorTitle and ErrorMessage with STR_NONE --- src/openrct2/actions/GameActionResult.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openrct2/actions/GameActionResult.h b/src/openrct2/actions/GameActionResult.h index d480dfe698..e50e756ba7 100644 --- a/src/openrct2/actions/GameActionResult.h +++ b/src/openrct2/actions/GameActionResult.h @@ -65,8 +65,8 @@ namespace GameActions using StringVariant = std::variant; GameActions::Status Error = GameActions::Status::Ok; - StringVariant ErrorTitle; - StringVariant ErrorMessage; + StringVariant ErrorTitle = STR_NONE; + StringVariant ErrorMessage = STR_NONE; std::array ErrorMessageArgs{}; CoordsXYZ Position = { LOCATION_NULL, LOCATION_NULL, LOCATION_NULL }; money32 Cost = 0; From 4d72c29ca31c30cd89b22db00ffb94652583e196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 20 Oct 2021 01:50:22 +0300 Subject: [PATCH 3/4] Specify underlying type for rct_string_id --- src/openrct2/localisation/StringIds.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openrct2/localisation/StringIds.h b/src/openrct2/localisation/StringIds.h index b99e4d82f8..4a442a83d7 100644 --- a/src/openrct2/localisation/StringIds.h +++ b/src/openrct2/localisation/StringIds.h @@ -14,7 +14,7 @@ constexpr const rct_string_id STR_NONE = 0xFFFF; constexpr const rct_string_id STR_VIEWPORT = 0xFFFE; -enum +enum : uint16_t { STR_EMPTY = 0, STR_RIDE_NAME_DEFAULT = 1, From 3325898e2584a74c6d62afe021e41b3157df4f84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Wed, 20 Oct 2021 16:37:32 +0300 Subject: [PATCH 4/4] Apply review suggestions --- src/openrct2/peep/Peep.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/openrct2/peep/Peep.cpp b/src/openrct2/peep/Peep.cpp index dd18031c90..e5b1ba67d8 100644 --- a/src/openrct2/peep/Peep.cpp +++ b/src/openrct2/peep/Peep.cpp @@ -597,13 +597,10 @@ std::unique_ptr Peep::Place(const TileCoordsXYZ& location, res->Error != GameActions::Status::Ok) { const auto stringId = std::get(res->ErrorMessage); - if (stringId != STR_RAISE_OR_LOWER_LAND_FIRST) + if (stringId != STR_RAISE_OR_LOWER_LAND_FIRST && stringId != STR_FOOTPATH_IN_THE_WAY) { - if (stringId != STR_FOOTPATH_IN_THE_WAY) - { - return std::make_unique( - GameActions::Status::NoClearance, STR_ERR_CANT_PLACE_PERSON_HERE, stringId, res->ErrorMessageArgs.data()); - } + return std::make_unique( + GameActions::Status::NoClearance, STR_ERR_CANT_PLACE_PERSON_HERE, stringId, res->ErrorMessageArgs.data()); } }