From 7cc44cda8428e28eae19fafed810580bcdb769cc Mon Sep 17 00:00:00 2001 From: Jonas Reith Date: Sun, 30 Aug 2026 18:28:21 +0200 Subject: [PATCH] Fix Territory/Culture/Wealth view toggles reverting instantly + orphaned overlay lines Two independent bugs, both found by reading the raygui source rather than relying on live clicks: 1. The toolbar's view-mode dropdown re-applied its (stale) selected index back onto Viewer::mode every single frame whenever mode was Territory/Culture/ Wealth/Ecoregion/Habitability -- none of which are in the dropdown's plain 13-entry list, so this fired unconditionally and reverted the view within the same frame it was toggled on, regardless of pause state. Fixed by only applying a mode change when the dropdown's active index actually changed as a result of that frame's GuiDropdownBox call (verified against raygui's own source: it only writes back *active on a genuine item click). 2. showNationBorders/showCultureBorders/showTradeRoutes were separate bools set true only inside their own P/X/Z toggle method, with nothing resetting them when the user switched views by any other means (number keys, the dropdown, another civ toggle) -- so realm borders, war fronts, alliance arcs and trade routes, once ever turned on, kept drawing over every other view forever. Fixed by deriving all three from `mode` instead of storing them separately, removing the possibility of drift entirely. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TMfyZv91tonDnPJqbaTVJE --- CLAUDE.md | 20 ++++++++++++++++++++ src/render/Toolbar.cpp | 14 +++++++++++--- src/render/Viewer.cpp | 7 ++----- src/render/Viewer.hpp | 14 +++++++++++--- src/render/ViewerRender.cpp | 26 +++++++++++++------------- 5 files changed, 57 insertions(+), 24 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c85c41a..14c28ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -386,6 +386,26 @@ on the Live World clock). **Steps 1–7 of the roadmap are done (plus a derived exported; a gazetteer should read the same everywhere. Saves a timestamped `atlas_export_YYYY-MM-DD_HHMMSS.png`; the status line reports how many labels made it in. Purely `src/render` — no save-format or `src/sim` change. +- **Bugfix: Territory/Culture/Wealth (`P`/`X`/`Z`) toggles didn't stick, and their border/route + overlays never turned off** — two independent bugs, both in `src/render`, found by reading the + actual raygui source rather than trusting live clicks (this session's X11 input became severely + delayed/queued after enough interaction; see the `gui-verification-xdotool` memory). (1) The + toolbar's view-mode `GuiDropdownBox` sync code re-derived `v.mode` from `toolbarModeActive` **every + frame**, not just on a real pick — `Territory`/`Culture`/`Wealth`/`Ecoregion`/`Habitability` aren't + in the dropdown's 13-entry list, so whenever `mode` was one of those, the code found no match, left + `toolbarModeActive` at its last plain-mode index, and then unconditionally forced `v.mode` back to + that stale plain mode and called `recolor()` — reverting the just-toggled view **within the same + frame**, independent of pause state (`drawToolbar()` runs every render frame regardless). Fixed by + only applying the picked mode when `toolbarModeActive` actually changed as a result of *this* call + (before/after compare, confirmed against raygui's `GuiDropdownBox` source: it only writes `*active` + on a genuine item click). (2) `showNationBorders`/`showCultureBorders`/`showTradeRoutes` were + separate `bool` fields set `true` only inside their own P/X/Z toggle method and never reset when + switching away by any *other* means (a number key, the dropdown, another civ toggle) — so realm + borders / war fronts / alliance arcs / trade routes, once ever turned on, stayed drawn over every + subsequent view forever. Fixed by making all three **derived** from `mode` (`showNationBorders()` + etc., now zero-arg methods on `Viewer`, e.g. `mode == ColorMode::Territory`) instead of stored, + independently-mutable state — the two can no longer drift apart because there's only one source of + truth. `src/render` only, no save-format or `src/sim` change; ctest unaffected (`src/sim` untouched). ## Current state diff --git a/src/render/Toolbar.cpp b/src/render/Toolbar.cpp index 03caa83..7bd924b 100644 --- a/src/render/Toolbar.cpp +++ b/src/render/Toolbar.cpp @@ -240,14 +240,22 @@ void drawToolbar(Viewer& v) { // View-mode dropdown, drawn LAST so its open list renders on top of the scroll body below it // (see Toolbar.hpp / the gotcha this avoids: raygui doesn't auto-exclude overlapping controls). + // v.mode can be one of 5 "special" side-effecting views (Territory/Culture/Wealth/Ecoregion/ + // Habitability) that aren't in kViewModes at all -- the sync loop below then finds no match and + // leaves toolbarModeActive at whatever plain index it last held. That used to be applied back + // unconditionally every frame ("if picked != v.mode, force v.mode = picked") -- which reverted a + // just-toggled special view back off within the very same frame, since it never matches any + // dropdown entry. Now it's only applied when the dropdown itself just changed (a real pick), + // tracked via before/after like the toggle-button shadow pattern elsewhere in this file. if (!v.toolbarModeEditMode) { for (int i = 0; i < kViewModeCount; ++i) if (kViewModes[i] == v.mode) { v.toolbarModeActive = i; break; } } + int activeBefore = v.toolbarModeActive; if (GuiDropdownBox(dropRect, kViewModeList, &v.toolbarModeActive, v.toolbarModeEditMode)) v.toolbarModeEditMode = !v.toolbarModeEditMode; - if (!v.toolbarModeEditMode) { - ColorMode picked = kViewModes[std::clamp(v.toolbarModeActive, 0, kViewModeCount - 1)]; - if (picked != v.mode) { v.mode = picked; v.recolor(); } + if (!v.toolbarModeEditMode && v.toolbarModeActive != activeBefore) { + v.mode = kViewModes[std::clamp(v.toolbarModeActive, 0, kViewModeCount - 1)]; + v.recolor(); } if (wasModal) GuiEnable(); diff --git a/src/render/Viewer.cpp b/src/render/Viewer.cpp index 37636e4..b70d7c7 100644 --- a/src/render/Viewer.cpp +++ b/src/render/Viewer.cpp @@ -348,10 +348,10 @@ void Viewer::regenWorld() { // after generate(): geometry change liveWorld = false; followId = 0; wxUndo.clear(); events.clear(); nextEventId = 1; // reseed/regen drops back to World Creation liveInfoTab = 0; eventRowRects.clear(); eventRowIndices.clear(); // Civilization overlays are viewer-side segment lists; buildGeometry() cleared the engine data but not - // these, so drop them (and their toggles) on a reseed or they keep drawing over the new world. + // these, so drop them on a reseed or they keep drawing over the new world (their draw toggles are + // derived from `mode`, reset right below, so nothing separate needs resetting here). nationBorders.clear(); cultureBorders.clear(); warFrontier.clear(); allyLinks.clear(); rivalLinks.clear(); tradeSea.clear(); tradeLand.clear(); - showNationBorders = showCultureBorders = showTradeRoutes = false; lastTerritoryYear = -1; if (mode == ColorMode::Territory || mode == ColorMode::Culture || mode == ColorMode::Wealth) mode = ColorMode::Biome; // civ colour views have no data until settlements are placed @@ -913,7 +913,6 @@ void Viewer::toggleTerritoryView() { // P: territory/realms colour vie if (!planet.settlementsPlaced()) { setStatus("Press U for the dawn of civilization first"); return; } if (!planet.nationsBuilt() || (int)planet.cellNation().size() != (int)planet.cells.size()) rebuildTerritory(); mode = (mode == ColorMode::Territory) ? ColorMode::Biome : ColorMode::Territory; - showNationBorders = (mode == ColorMode::Territory); recolor(); setStatus(mode == ColorMode::Territory ? "Territory / realms on" : "Territory off"); } @@ -923,7 +922,6 @@ void Viewer::toggleCultureView() { // X: culture/faiths colour view if (!planet.settlementsPlaced()) { setStatus("Press U for the dawn of civilization first"); return; } if (!planet.culturesBuilt() || (int)planet.cellCulture().size() != (int)planet.cells.size()) rebuildTerritory(); mode = (mode == ColorMode::Culture) ? ColorMode::Biome : ColorMode::Culture; - showCultureBorders = (mode == ColorMode::Culture); recolor(); setStatus(mode == ColorMode::Culture ? "Cultures / faiths on" : "Cultures off"); } @@ -933,7 +931,6 @@ void Viewer::toggleTradeView() { // Z: wealth/trade colour view + if (!planet.settlementsPlaced()) { setStatus("Press U for the dawn of civilization first"); return; } if (!planet.tradeBuilt() || (int)planet.cellWealth().size() != (int)planet.cells.size()) rebuildTerritory(); mode = (mode == ColorMode::Wealth) ? ColorMode::Biome : ColorMode::Wealth; - showTradeRoutes = (mode == ColorMode::Wealth); recolor(); setStatus(mode == ColorMode::Wealth ? "Wealth / trade on" : "Wealth off"); } diff --git a/src/render/Viewer.hpp b/src/render/Viewer.hpp index 8337d2b..c3091ab 100644 --- a/src/render/Viewer.hpp +++ b/src/render/Viewer.hpp @@ -107,14 +107,22 @@ struct Viewer { bool showSettlements = true; // civilization settlement markers (key U seeds + toggles) std::vector atlasRowCells; // cell to focus per visible Atlas/Eco/Civ/Realms-tab row (parallel to the list) std::vector nationBorders; // political border segments (rebuilt on year tick / placement / load) - bool showNationBorders = false; // draw nation/realm borders (on with the Territory view) std::vector cultureBorders; // cultural-region border segments (civ Step 4, rebuilt with territory) - bool showCultureBorders = false; // draw cultural-region borders (on with the Culture view) std::vector warFrontier; // red frontier segments between realms currently at war (civ Step 5) std::vector allyLinks, rivalLinks; // civ Step 6: capital-to-capital arcs (allies green, rivals red) std::vector tradeSea, tradeLand; // civ Step 7: trade routes (sea cyan / river+land amber) - bool showTradeRoutes = false; // draw trade routes (on with the Wealth view) long lastTerritoryYear = -1; // sim year territory was last recomputed (recompute when it ticks) + // Whether to draw the political/cultural/trade overlays is *derived* from `mode` -- deliberately + // not a separately-stored bool. It used to be (showNationBorders et al., set only inside the P/X/Z + // toggle methods), and every other way of changing `mode` (number keys, the toolbar's view-mode + // dropdown) left it stale: switching away from the Territory view by any means other than pressing + // P again left showNationBorders stuck true forever (realm borders/war fronts/alliance arcs kept + // drawing over every other view), while pressing P/X/Z never actually stuck because of a second, + // compounding bug in the dropdown sync (see drawToolbar()) -- so it looked like the toggle + // "immediately turned back off". Deriving it removes the class of bug entirely. + bool showNationBorders() const { return mode == ColorMode::Territory; } + bool showCultureBorders() const { return mode == ColorMode::Culture; } + bool showTradeRoutes() const { return mode == ColorMode::Wealth; } // World event journal: currently Live World events, shaped to be reused by later phases. struct WorldEvent { diff --git a/src/render/ViewerRender.cpp b/src/render/ViewerRender.cpp index 0226798..4e4b6d4 100644 --- a/src/render/ViewerRender.cpp +++ b/src/render/ViewerRender.cpp @@ -84,7 +84,7 @@ void Viewer::renderGlobe3D() { } rlEnd(); rlSetLineWidth(1.0f); } - if (showNationBorders && !nationBorders.empty()) { // political / realm borders (dark, over the tint) + if (showNationBorders() && !nationBorders.empty()) { // political / realm borders (dark, over the tint) rlSetLineWidth(2.5f); rlBegin(RL_LINES); rlColor4ub(18, 18, 26, 235); for (size_t i = 0; i + 1 < nationBorders.size(); i += 2) { rlVertex3f(nationBorders[i].x, nationBorders[i].y, nationBorders[i].z); @@ -92,7 +92,7 @@ void Viewer::renderGlobe3D() { } rlEnd(); rlSetLineWidth(1.0f); } - if (showCultureBorders && !cultureBorders.empty()) { // cultural-region borders (pale, distinct from political) + if (showCultureBorders() && !cultureBorders.empty()) { // cultural-region borders (pale, distinct from political) rlSetLineWidth(3.0f); rlBegin(RL_LINES); rlColor4ub(245, 240, 220, 220); for (size_t i = 0; i + 1 < cultureBorders.size(); i += 2) { rlVertex3f(cultureBorders[i].x, cultureBorders[i].y, cultureBorders[i].z); @@ -100,7 +100,7 @@ void Viewer::renderGlobe3D() { } rlEnd(); rlSetLineWidth(1.0f); } - if (showNationBorders && !warFrontier.empty()) { // civ Step 5: war fronts (bright red, with the territory view) + if (showNationBorders() && !warFrontier.empty()) { // civ Step 5: war fronts (bright red, with the territory view) rlSetLineWidth(3.5f); rlBegin(RL_LINES); rlColor4ub(235, 40, 30, 255); for (size_t i = 0; i + 1 < warFrontier.size(); i += 2) { rlVertex3f(warFrontier[i].x, warFrontier[i].y, warFrontier[i].z); @@ -108,7 +108,7 @@ void Viewer::renderGlobe3D() { } rlEnd(); rlSetLineWidth(1.0f); } - if (showNationBorders) { // civ Step 6: diplomacy arcs (allies green, rivals dark red) + if (showNationBorders()) { // civ Step 6: diplomacy arcs (allies green, rivals dark red) rlSetLineWidth(2.0f); rlBegin(RL_LINES); rlColor4ub(70, 220, 120, 200); for (size_t i = 0; i + 1 < allyLinks.size(); i += 2) { @@ -122,7 +122,7 @@ void Viewer::renderGlobe3D() { } rlEnd(); rlSetLineWidth(1.0f); } - if (showTradeRoutes) { // civ Step 7: trade routes (sea cyan, land/river amber) + if (showTradeRoutes()) { // civ Step 7: trade routes (sea cyan, land/river amber) rlSetLineWidth(1.5f); rlBegin(RL_LINES); rlColor4ub(90, 200, 235, 200); for (size_t i = 0; i + 1 < tradeSea.size(); i += 2) { @@ -391,13 +391,13 @@ void Viewer::drawMapOverlays(Rectangle vr, double lonOffset, float scale, float if (showGrat) { drawGraticule2D(graticule, vr, lonOffset); drawGraticuleLabels2D(vr, lonOffset); } if (showBorders && !borders.empty()) drawSegments2D(borders, Color{255, 235, 90, 255}, 2.0f * scale, vr, lonOffset); if (showBorders && !ridgeBorders.empty()) drawSegments2D(ridgeBorders, Color{220, 70, 60, 255}, 2.0f * scale, vr, lonOffset); - if (showNationBorders && !nationBorders.empty()) drawSegments2D(nationBorders, Color{18, 18, 26, 235}, 2.0f * scale, vr, lonOffset); - if (showCultureBorders && !cultureBorders.empty()) drawSegments2D(cultureBorders, Color{245, 240, 220, 230}, 2.5f * scale, vr, lonOffset); - if (showNationBorders && !warFrontier.empty()) drawSegments2D(warFrontier, Color{235, 40, 30, 255}, 2.5f * scale, vr, lonOffset); - if (showNationBorders && !allyLinks.empty()) drawSegments2D(allyLinks, Color{70, 220, 120, 220}, 1.5f * scale, vr, lonOffset); - if (showNationBorders && !rivalLinks.empty()) drawSegments2D(rivalLinks, Color{150, 40, 60, 220}, 1.5f * scale, vr, lonOffset); - if (showTradeRoutes && !tradeSea.empty()) drawSegments2D(tradeSea, Color{90, 200, 235, 220}, 1.2f * scale, vr, lonOffset); - if (showTradeRoutes && !tradeLand.empty()) drawSegments2D(tradeLand, Color{230, 180, 90, 220}, 1.2f * scale, vr, lonOffset); + if (showNationBorders() && !nationBorders.empty()) drawSegments2D(nationBorders, Color{18, 18, 26, 235}, 2.0f * scale, vr, lonOffset); + if (showCultureBorders() && !cultureBorders.empty()) drawSegments2D(cultureBorders, Color{245, 240, 220, 230}, 2.5f * scale, vr, lonOffset); + if (showNationBorders() && !warFrontier.empty()) drawSegments2D(warFrontier, Color{235, 40, 30, 255}, 2.5f * scale, vr, lonOffset); + if (showNationBorders() && !allyLinks.empty()) drawSegments2D(allyLinks, Color{70, 220, 120, 220}, 1.5f * scale, vr, lonOffset); + if (showNationBorders() && !rivalLinks.empty()) drawSegments2D(rivalLinks, Color{150, 40, 60, 220}, 1.5f * scale, vr, lonOffset); + if (showTradeRoutes() && !tradeSea.empty()) drawSegments2D(tradeSea, Color{90, 200, 235, 220}, 1.2f * scale, vr, lonOffset); + if (showTradeRoutes() && !tradeLand.empty()) drawSegments2D(tradeLand, Color{230, 180, 90, 220}, 1.2f * scale, vr, lonOffset); if (showDrift && !driftArrows.empty()) drawSegments2D(driftArrows, Color{90, 230, 255, 255}, 2.0f * scale, vr, lonOffset); if (liveWorld && showTides && !coastCols.empty()) drawColoredSegments2D(coast, coastCols, 2.0f * scale, vr, lonOffset); if (showCurrents && !currentCols.empty()) drawColoredSegments2D(currentSegs, currentCols, 1.6f * scale, vr, lonOffset); @@ -1190,7 +1190,7 @@ void Viewer::renderFrame() { } } // 3D realm labels (with the territory view): name kingdoms/empires at their capital. - if (showNationBorders && !planet.nationList().empty()) { + if (showNationBorders() && !planet.nationList().empty()) { Vec3 camPos{cam.position.x, cam.position.y, cam.position.z}; Vec3 forward = (Vec3{cam.target.x, cam.target.y, cam.target.z} - camPos).normalized(); Vec3 right = forward.cross(Vec3{cam.up.x, cam.up.y, cam.up.z}).normalized();