From 3c49081df34fb1801063c0b538d12d4422fcf3f0 Mon Sep 17 00:00:00 2001 From: Star Rauchenberger Date: Fri, 20 Dec 2024 14:33:43 -0500 Subject: Fixed remaining thread unsafe APState/IPCState reads Still would like to add some kind of wrapper object that TrackerState could use to read APState without locking, since it'll only ever be called from the thread that would do the mutating, but this is fine for now. --- src/ap_state.cpp | 8 ++++--- src/ap_state.h | 9 +++----- src/ipc_state.cpp | 4 ++-- src/ipc_state.h | 2 +- src/subway_map.cpp | 61 ++++++++++++++++++++++++++++----------------------- src/subway_map.h | 6 +++++ src/tracker_state.cpp | 23 ++++++++++++------- 7 files changed, 65 insertions(+), 48 deletions(-) diff --git a/src/ap_state.cpp b/src/ap_state.cpp index 908c3a8..4ac0cce 100644 --- a/src/ap_state.cpp +++ b/src/ap_state.cpp @@ -675,7 +675,9 @@ bool AP_IsPaintingShuffle() { return GetState().painting_shuffle; } -const std::map& AP_GetPaintingMapping() { +std::map AP_GetPaintingMapping() { + std::lock_guard state_guard(GetState().state_mutex); + return GetState().painting_mapping; } @@ -685,7 +687,7 @@ bool AP_IsPaintingMappedTo(const std::string& painting_id) { return GetState().painting_codomain.count(painting_id); } -const std::set& AP_GetCheckedPaintings() { +std::set AP_GetCheckedPaintings() { return GetState().GetCheckedPaintings(); } @@ -778,7 +780,7 @@ bool AP_IsSunwarpShuffle() { return GetState().sunwarp_shuffle; } -const std::map& AP_GetSunwarpMapping() { +std::map AP_GetSunwarpMapping() { return GetState().sunwarp_mapping; } diff --git a/src/ap_state.h b/src/ap_state.h index a23b13d..2da0b8e 100644 --- a/src/ap_state.h +++ b/src/ap_state.h @@ -68,13 +68,11 @@ bool AP_IsColorShuffle(); bool AP_IsPaintingShuffle(); -// WARNING: Not thread-safe! -const std::map& AP_GetPaintingMapping(); +std::map AP_GetPaintingMapping(); bool AP_IsPaintingMappedTo(const std::string& painting_id); -// WARNING: Not thread-safe! -const std::set& AP_GetCheckedPaintings(); +std::set AP_GetCheckedPaintings(); bool AP_IsPaintingChecked(const std::string& painting_id); @@ -100,8 +98,7 @@ SunwarpAccess AP_GetSunwarpAccess(); bool AP_IsSunwarpShuffle(); -// WARNING: Not thread-safe! -const std::map& AP_GetSunwarpMapping(); +std::map AP_GetSunwarpMapping(); bool AP_HasReachedGoal(); diff --git a/src/ipc_state.cpp b/src/ipc_state.cpp index cba576c..1f8d286 100644 --- a/src/ipc_state.cpp +++ b/src/ipc_state.cpp @@ -103,7 +103,7 @@ struct IPCState { return player_position; } - const std::set& GetSolvedPanels() { + std::set GetSolvedPanels() { std::lock_guard state_guard(state_mutex); return solved_panels; @@ -383,6 +383,6 @@ std::optional> IPC_GetPlayerPosition() { return GetState().GetPlayerPosition(); } -const std::set& IPC_GetSolvedPanels() { +std::set IPC_GetSolvedPanels() { return GetState().GetSolvedPanels(); } diff --git a/src/ipc_state.h b/src/ipc_state.h index 84f3d29..7c9d68d 100644 --- a/src/ipc_state.h +++ b/src/ipc_state.h @@ -20,6 +20,6 @@ bool IPC_IsConnected(); std::optional> IPC_GetPlayerPosition(); -const std::set& IPC_GetSolvedPanels(); +std::set IPC_GetSolvedPanels(); #endif /* end of include guard: IPC_STATE_H_6B3B0958 */ diff --git a/src/subway_map.cpp b/src/subway_map.cpp index f896693..4ebc56a 100644 --- a/src/subway_map.cpp +++ b/src/subway_map.cpp @@ -16,28 +16,6 @@ constexpr int OWL_ACTUAL_SIZE = 32; enum class ItemDrawType { kNone, kBox, kOwl }; -namespace { - -std::optional GetRealSubwayDoor(const SubwayItem subway_item) { - if (AP_IsSunwarpShuffle() && subway_item.sunwarp && - subway_item.sunwarp->type != SubwaySunwarpType::kFinal) { - int sunwarp_index = subway_item.sunwarp->dots - 1; - if (subway_item.sunwarp->type == SubwaySunwarpType::kExit) { - sunwarp_index += 6; - } - - for (const auto &[start_index, mapping] : AP_GetSunwarpMapping()) { - if (start_index == sunwarp_index || mapping.exit_index == sunwarp_index) { - return GD_GetSunwarpDoors().at(mapping.dots - 1); - } - } - } - - return subway_item.door; -} - -} // namespace - SubwayMap::SubwayMap(wxWindow *parent) : wxPanel(parent, wxID_ANY) { SetBackgroundStyle(wxBG_STYLE_PAINT); @@ -140,7 +118,7 @@ void SubwayMap::OnConnect() { SubwaySunwarp final_sunwarp{.dots = 6, .type = SubwaySunwarpType::kFinal}; int final_sunwarp_item = GD_GetSubwayItemForSunwarp(final_sunwarp); - for (const auto &[index, mapping] : AP_GetSunwarpMapping()) { + for (const auto &[index, mapping] : sunwarp_mapping_) { std::string tag = fmt::format("sunwarp{}", mapping.dots); SubwaySunwarp fromWarp; @@ -197,15 +175,22 @@ void SubwayMap::OnConnect() { } void SubwayMap::UpdateIndicators() { + if (AP_IsSunwarpShuffle()) { + sunwarp_mapping_ = AP_GetSunwarpMapping(); + } + if (AP_IsPaintingShuffle()) { - for (const std::string &painting_id : AP_GetCheckedPaintings()) { + std::map painting_mapping = + AP_GetPaintingMapping(); + std::set remote_checked_paintings = AP_GetCheckedPaintings(); + + for (const std::string &painting_id : remote_checked_paintings) { if (!checked_paintings_.count(painting_id)) { checked_paintings_.insert(painting_id); - if (AP_GetPaintingMapping().count(painting_id)) { + if (painting_mapping.count(painting_id)) { std::optional from_id = GD_GetSubwayItemForPainting(painting_id); - std::optional to_id = GD_GetSubwayItemForPainting( - AP_GetPaintingMapping().at(painting_id)); + std::optional to_id = GD_GetSubwayItemForPainting(painting_mapping.at(painting_id)); if (from_id && to_id) { networks_.AddLink(*from_id, *to_id, false); @@ -600,6 +585,8 @@ void SubwayMap::Redraw() { wxGCDC gcdc(dc); + std::map painting_mapping = AP_GetPaintingMapping(); + for (const SubwayItem &subway_item : GD_GetSubwayItems()) { ItemDrawType draw_type = ItemDrawType::kNone; const wxBrush *brush_color = wxGREY_BRUSH; @@ -641,7 +628,7 @@ void SubwayMap::Redraw() { if (checked_paintings_.count(painting_id)) { has_checked_painting = true; - if (AP_GetPaintingMapping().count(painting_id)) { + if (painting_mapping.count(painting_id)) { has_mapped_painting = true; } else if (AP_IsPaintingMappedTo(painting_id)) { has_codomain_painting = true; @@ -803,6 +790,24 @@ void SubwayMap::SetZoom(double zoom, wxPoint static_point) { zoom_slider_->SetValue((zoom - 1.0) / 0.25); } +std::optional SubwayMap::GetRealSubwayDoor(const SubwayItem subway_item) { + if (AP_IsSunwarpShuffle() && subway_item.sunwarp && + subway_item.sunwarp->type != SubwaySunwarpType::kFinal) { + int sunwarp_index = subway_item.sunwarp->dots - 1; + if (subway_item.sunwarp->type == SubwaySunwarpType::kExit) { + sunwarp_index += 6; + } + + for (const auto &[start_index, mapping] : sunwarp_mapping_) { + if (start_index == sunwarp_index || mapping.exit_index == sunwarp_index) { + return GD_GetSunwarpDoors().at(mapping.dots - 1); + } + } + } + + return subway_item.door; +} + quadtree::Box SubwayMap::GetItemBox::operator()(const int &id) const { const SubwayItem &subway_item = GD_GetSubwayItem(id); return {static_cast(subway_item.x), static_cast(subway_item.y), diff --git a/src/subway_map.h b/src/subway_map.h index feee8ff..6aa31f5 100644 --- a/src/subway_map.h +++ b/src/subway_map.h @@ -15,6 +15,7 @@ #include +#include "ap_state.h" #include "game_data.h" #include "network_set.h" @@ -50,6 +51,8 @@ class SubwayMap : public wxPanel { void SetScrollSpeed(int scroll_x, int scroll_y); void SetZoom(double zoom, wxPoint static_point); + std::optional GetRealSubwayDoor(const SubwayItem subway_item); + wxImage map_image_; wxImage owl_image_; wxBitmap unchecked_eye_; @@ -87,6 +90,9 @@ class SubwayMap : public wxPanel { NetworkSet networks_; std::set checked_paintings_; + + // Cached from APState. + std::map sunwarp_mapping_; }; #endif /* end of include guard: SUBWAY_MAP_H_BD2D843E */ diff --git a/src/tracker_state.cpp b/src/tracker_state.cpp index 4a49fac..eee43e4 100644 --- a/src/tracker_state.cpp +++ b/src/tracker_state.cpp @@ -192,6 +192,10 @@ class StateCalculator { : options_(options) {} void Calculate() { + painting_mapping_ = AP_GetPaintingMapping(); + checked_paintings_ = AP_GetCheckedPaintings(); + sunwarp_mapping_ = AP_GetSunwarpMapping(); + std::list panel_boundary; std::list painting_boundary; std::list flood_boundary; @@ -231,12 +235,12 @@ class StateCalculator { reachable_changed = true; PaintingExit cur_painting = GD_GetPaintingExit(painting_id); - if (AP_GetPaintingMapping().count(cur_painting.internal_id) && - AP_GetCheckedPaintings().count(cur_painting.internal_id)) { + if (painting_mapping_.count(cur_painting.internal_id) && + checked_paintings_.count(cur_painting.internal_id)) { Exit painting_exit; PaintingExit target_painting = GD_GetPaintingExit(GD_GetPaintingByName( - AP_GetPaintingMapping().at(cur_painting.internal_id))); + painting_mapping_.at(cur_painting.internal_id))); painting_exit.source_room = cur_painting.room; painting_exit.destination_room = target_painting.room; painting_exit.type = EntranceType::kPainting; @@ -295,8 +299,8 @@ class StateCalculator { if (AP_IsSunwarpShuffle()) { for (int index : room_obj.sunwarps) { - if (AP_GetSunwarpMapping().count(index)) { - const SunwarpMapping& sm = AP_GetSunwarpMapping().at(index); + if (sunwarp_mapping_.count(index)) { + const SunwarpMapping& sm = sunwarp_mapping_.at(index); new_boundary.push_back( {.source_room = room_exit.destination_room, @@ -317,8 +321,7 @@ class StateCalculator { if (AP_IsPilgrimageEnabled()) { int pilgrimage_start_id = GD_GetRoomByName("Hub Room"); if (AP_IsSunwarpShuffle()) { - for (const auto& [start_index, mapping] : - AP_GetSunwarpMapping()) { + for (const auto& [start_index, mapping] : sunwarp_mapping_) { if (mapping.dots == 1) { pilgrimage_start_id = GD_GetRoomForSunwarp(start_index); } @@ -578,7 +581,7 @@ class StateCalculator { if (AP_IsSunwarpShuffle()) { pilgrimage_pairs = std::vector>(5); - for (const auto& [start_index, mapping] : AP_GetSunwarpMapping()) { + for (const auto& [start_index, mapping] : sunwarp_mapping_) { if (mapping.dots > 1) { std::get<1>(pilgrimage_pairs[mapping.dots - 2]) = start_index; } @@ -649,6 +652,10 @@ class StateCalculator { bool pilgrimage_doable_ = false; std::map> paths_; + + std::map painting_mapping_; + std::set checked_paintings_; + std::map sunwarp_mapping_; }; } // namespace -- cgit 1.4.1