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.

7 files changed, 65 insertions, 48 deletions

@@ -675,7 +675,9 @@ bool AP_IsPaintingShuffle() {
   return GetState().painting_shuffle;
 }
 
-const std::map<std::string, std::string>& AP_GetPaintingMapping() {
+std::map<std::string, std::string> 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<std::string>& AP_GetCheckedPaintings() {
+std::set<std::string> AP_GetCheckedPaintings() {
   return GetState().GetCheckedPaintings();
 }
 
@@ -778,7 +780,7 @@ bool AP_IsSunwarpShuffle() {
   return GetState().sunwarp_shuffle;
 }
 
-const std::map<int, SunwarpMapping>& AP_GetSunwarpMapping() {
+std::map<int, SunwarpMapping> AP_GetSunwarpMapping() {
   return GetState().sunwarp_mapping;
 }
 
@@ -68,13 +68,11 @@ bool AP_IsColorShuffle();
 
 bool AP_IsPaintingShuffle();
 
-// WARNING: Not thread-safe!
-const std::map<std::string, std::string>& AP_GetPaintingMapping();
+std::map<std::string, std::string> AP_GetPaintingMapping();
 
 bool AP_IsPaintingMappedTo(const std::string& painting_id);
 
-// WARNING: Not thread-safe!
-const std::set<std::string>& AP_GetCheckedPaintings();
+std::set<std::string> 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<int, SunwarpMapping>& AP_GetSunwarpMapping();
+std::map<int, SunwarpMapping> AP_GetSunwarpMapping();
 
 bool AP_HasReachedGoal();
 
@@ -103,7 +103,7 @@ struct IPCState {
     return player_position;
   }
 
-  const std::set<std::string>& GetSolvedPanels() {
+  std::set<std::string> GetSolvedPanels() {
     std::lock_guard state_guard(state_mutex);
 
     return solved_panels;
@@ -383,6 +383,6 @@ std::optional<std::tuple<int, int>> IPC_GetPlayerPosition() {
   return GetState().GetPlayerPosition();
 }
 
-const std::set<std::string>& IPC_GetSolvedPanels() {
+std::set<std::string> IPC_GetSolvedPanels() {
   return GetState().GetSolvedPanels();
 }
@@ -20,6 +20,6 @@ bool IPC_IsConnected();
 
 std::optional<std::tuple<int, int>> IPC_GetPlayerPosition();
 
-const std::set<std::string>& IPC_GetSolvedPanels();
+std::set<std::string> IPC_GetSolvedPanels();
 
 #endif /* end of include guard: IPC_STATE_H_6B3B0958 */
@@ -16,28 +16,6 @@ constexpr int OWL_ACTUAL_SIZE = 32;
 
 enum class ItemDrawType { kNone, kBox, kOwl };
 
-namespace {
-
-std::optional<int> 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<std::string, std::string> painting_mapping =
+        AP_GetPaintingMapping();
+    std::set<std::string> 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<int> from_id = GD_GetSubwayItemForPainting(painting_id);
-          std::optional<int> to_id = GD_GetSubwayItemForPainting(
-              AP_GetPaintingMapping().at(painting_id));
+          std::optional<int> 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<std::string, std::string> 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<int> 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<float> SubwayMap::GetItemBox::operator()(const int &id) const {
   const SubwayItem &subway_item = GD_GetSubwayItem(id);
   return {static_cast<float>(subway_item.x), static_cast<float>(subway_item.y),
@@ -15,6 +15,7 @@
 
 #include <quadtree/Quadtree.h>
 
+#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<int> 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<std::string> checked_paintings_;
+
+  // Cached from APState.
+  std::map<int, SunwarpMapping> sunwarp_mapping_;
 };
 
 #endif /* end of include guard: SUBWAY_MAP_H_BD2D843E */
@@ -192,6 +192,10 @@ class StateCalculator {
       : options_(options) {}
 
   void Calculate() {
+    painting_mapping_ = AP_GetPaintingMapping();
+    checked_paintings_ = AP_GetCheckedPaintings();
+    sunwarp_mapping_ = AP_GetSunwarpMapping();
+
     std::list<int> panel_boundary;
     std::list<int> painting_boundary;
     std::list<Exit> 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<std::tuple<int, int>>(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<int, std::list<int>> paths_;
+
+  std::map<std::string, std::string> painting_mapping_;
+  std::set<std::string> checked_paintings_;
+  std::map<int, SunwarpMapping> sunwarp_mapping_;
 };
 
 }  // namespace