From 17a29dfe87c44525de797b0358ce48cf9d349301 Mon Sep 17 00:00:00 2001 From: Ted John Date: Sun, 28 Feb 2021 01:13:43 +0000 Subject: [PATCH] Fix #14190: Game crash likely related to plug-in hotkeys Do not cache references to RegisteredShortcut as they can be invalidated when new shortcuts are registered / removed. Use a map to improve query performance of shortcut by ID. Store a separate list of strings for the map to use as a key. --- src/openrct2-ui/input/InputManager.cpp | 18 ++++++--------- src/openrct2-ui/input/InputManager.h | 7 +----- src/openrct2-ui/input/ShortcutManager.cpp | 28 ++++++++++++----------- src/openrct2-ui/input/ShortcutManager.h | 6 ++++- src/openrct2-ui/windows/ShortcutKeys.cpp | 4 ++-- 5 files changed, 30 insertions(+), 33 deletions(-) diff --git a/src/openrct2-ui/input/InputManager.cpp b/src/openrct2-ui/input/InputManager.cpp index 014cf7f35a..115eef910e 100644 --- a/src/openrct2-ui/input/InputManager.cpp +++ b/src/openrct2-ui/input/InputManager.cpp @@ -291,21 +291,17 @@ void InputManager::ProcessHoldEvents() auto& shortcutManager = GetShortcutManager(); if (!shortcutManager.IsPendingShortcutChange()) { - ProcessViewScrollEvent(ShortcutId::ViewScrollUp, _scrollUpShortcut, { 0, -1 }); - ProcessViewScrollEvent(ShortcutId::ViewScrollDown, _scrollDownShortcut, { 0, 1 }); - ProcessViewScrollEvent(ShortcutId::ViewScrollLeft, _scrollLeftShortcut, { -1, 0 }); - ProcessViewScrollEvent(ShortcutId::ViewScrollRight, _scrollRightShortcut, { 1, 0 }); + ProcessViewScrollEvent(ShortcutId::ViewScrollUp, { 0, -1 }); + ProcessViewScrollEvent(ShortcutId::ViewScrollDown, { 0, 1 }); + ProcessViewScrollEvent(ShortcutId::ViewScrollLeft, { -1, 0 }); + ProcessViewScrollEvent(ShortcutId::ViewScrollRight, { 1, 0 }); } } -void InputManager::ProcessViewScrollEvent( - std::string_view shortcutId, RegisteredShortcut*& shortcut, const ScreenCoordsXY& delta) +void InputManager::ProcessViewScrollEvent(std::string_view shortcutId, const ScreenCoordsXY& delta) { - if (shortcut == nullptr) - { - auto& shortcutManager = GetShortcutManager(); - shortcut = shortcutManager.GetShortcut(shortcutId); - } + auto& shortcutManager = GetShortcutManager(); + auto shortcut = shortcutManager.GetShortcut(shortcutId); if (shortcut != nullptr && GetState(*shortcut)) { _viewScroll.x += delta.x; diff --git a/src/openrct2-ui/input/InputManager.h b/src/openrct2-ui/input/InputManager.h index 7cd88e08d6..a25776cfb9 100644 --- a/src/openrct2-ui/input/InputManager.h +++ b/src/openrct2-ui/input/InputManager.h @@ -53,11 +53,6 @@ namespace OpenRCT2::Ui uint32_t _mouseState; std::vector _keyboardState; - RegisteredShortcut* _scrollLeftShortcut{}; - RegisteredShortcut* _scrollUpShortcut{}; - RegisteredShortcut* _scrollRightShortcut{}; - RegisteredShortcut* _scrollDownShortcut{}; - void CheckJoysticks(); void HandleViewScrolling(); @@ -67,7 +62,7 @@ namespace OpenRCT2::Ui void ProcessInGameConsole(const InputEvent& e); void ProcessChat(const InputEvent& e); void ProcessHoldEvents(); - void ProcessViewScrollEvent(std::string_view shortcutId, RegisteredShortcut*& shortcut, const ScreenCoordsXY& delta); + void ProcessViewScrollEvent(std::string_view shortcutId, const ScreenCoordsXY& delta); bool GetState(const RegisteredShortcut& shortcut) const; bool GetState(const ShortcutInput& shortcut) const; diff --git a/src/openrct2-ui/input/ShortcutManager.cpp b/src/openrct2-ui/input/ShortcutManager.cpp index 364a2c3570..d729ea8b30 100644 --- a/src/openrct2-ui/input/ShortcutManager.cpp +++ b/src/openrct2-ui/input/ShortcutManager.cpp @@ -119,22 +119,24 @@ void ShortcutManager::RegisterShortcut(RegisteredShortcut&& shortcut) { if (!shortcut.Id.empty() && GetShortcut(shortcut.Id) == nullptr) { - Shortcuts.push_back(shortcut); + auto id = std::make_unique(shortcut.Id); + auto idView = std::string_view(*id); + _ids.push_back(std::move(id)); + Shortcuts[idView] = shortcut; } } RegisteredShortcut* ShortcutManager::GetShortcut(std::string_view id) { - auto result = std::find_if(Shortcuts.begin(), Shortcuts.end(), [id](const RegisteredShortcut& s) { return s.Id == id; }); - return result == Shortcuts.end() ? nullptr : &(*result); + auto result = Shortcuts.find(id); + return result == Shortcuts.end() ? nullptr : &result->second; } void ShortcutManager::RemoveShortcut(std::string_view id) { - Shortcuts.erase( - std::remove_if( - Shortcuts.begin(), Shortcuts.end(), [id](const RegisteredShortcut& shortcut) { return shortcut.Id == id; }), - Shortcuts.end()); + Shortcuts.erase(id); + _ids.erase( + std::remove_if(_ids.begin(), _ids.end(), [id](const std::unique_ptr& x) { return *x == id; }), _ids.end()); } bool ShortcutManager::IsPendingShortcutChange() const @@ -153,9 +155,9 @@ void ShortcutManager::ProcessEvent(const InputEvent& e) { for (const auto& shortcut : Shortcuts) { - if (shortcut.Matches(e)) + if (shortcut.second.Matches(e)) { - shortcut.Action(); + shortcut.second.Action(); } } } @@ -334,15 +336,15 @@ void ShortcutManager::SaveUserBindings(const fs::path& path) for (const auto& shortcut : Shortcuts) { - auto& jShortcut = root[shortcut.Id]; - if (shortcut.Current.size() == 1) + auto& jShortcut = root[shortcut.second.Id]; + if (shortcut.second.Current.size() == 1) { - jShortcut = shortcut.Current[0].ToString(); + jShortcut = shortcut.second.Current[0].ToString(); } else { jShortcut = nlohmann::json::array(); - for (const auto& binding : shortcut.Current) + for (const auto& binding : shortcut.second.Current) { jShortcut.push_back(binding.ToString()); } diff --git a/src/openrct2-ui/input/ShortcutManager.h b/src/openrct2-ui/input/ShortcutManager.h index 982ca30ab1..88e87fdd7a 100644 --- a/src/openrct2-ui/input/ShortcutManager.h +++ b/src/openrct2-ui/input/ShortcutManager.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -118,8 +119,11 @@ namespace OpenRCT2::Ui void LoadUserBindings(const fs::path& path); void SaveUserBindings(const fs::path& path); + // We store the IDs separately so that we can safely use them for string_view in the map + std::vector> _ids; + public: - std::vector Shortcuts; + std::unordered_map Shortcuts; ShortcutManager(const std::shared_ptr& env); ShortcutManager(const ShortcutManager&) = delete; diff --git a/src/openrct2-ui/windows/ShortcutKeys.cpp b/src/openrct2-ui/windows/ShortcutKeys.cpp index 8012b15654..8bddd6ebc8 100644 --- a/src/openrct2-ui/windows/ShortcutKeys.cpp +++ b/src/openrct2-ui/windows/ShortcutKeys.cpp @@ -403,9 +403,9 @@ private: auto& shortcutManager = GetShortcutManager(); for (const auto& shortcut : shortcutManager.Shortcuts) { - if (IsInCurrentTab(shortcut)) + if (IsInCurrentTab(shortcut.second)) { - result.push_back(&shortcut); + result.push_back(&shortcut.second); } } return result;