Let a cached route field notice the ground moved, wherever it moved - #232
Conversation
Giszmo
changed the title
|
Reviewed Reproduction code: probe and detailed instructions, C++ harness. This separate branch adds only the probe, build target, and documentation on top of the reviewed PR head; it does not change this PR or fix production behavior. git fetch origin codex/review-building-invalidation
git worktree add --detach ../glob2-pr232-repro bfb0db9e93611b1fc0c3f05161a82c5612f114e1
cd ../glob2-pr232-repro
scons -j6 release=1 server=0 invalidation-review
python3 test/run-savegame-safety-tests.py --check-preferences build/src/BuildingInvalidationReview .Normal client build dependencies are required. On a Homebrew Mac, prefix the final command with Each case starts with a fresh 128×128 grass map, two teams, and a team-0 inn at (8,8). Cache its walking field at tick 100. Change a team-1 obstacle inn's footprint, advance to tick 130 (past the dirty-rebuild throttle), and compare the normal lookup with an explicitly dirtied/rebuilt field at the obstacle's origin.
The corresponding gaps are:
Actual output: The probe asserts this known PR-head behavior: exit 0 means the reproduction succeeded, not that the stale cases are correct. In a final regression suite those three cases should require cached == fresh. These are cache-correctness reproductions, not a claim about measured starvation or unfinished construction in a full match. About savestates: at this PR head, Building::load calls freeGradients(), so saving/reloading loses the stale cache that demonstrates the problem. The in-process fixture is the reliable reproducer here. Suggested next step: cover placement and both destruction paths, and make invalidation account for all affected cached building/flag fields across teams rather than proximity alone. A topology generation checked lazily by caches is one possible approach, but rebuild costs would need checking. The eventual simulation change also needs the appropriate replay-version bump after rebasing onto current master. |
Giszmo
changed the title
Giszmo
changed the title
genixpro
left a comment
genixpro
left a comment
There was a problem hiding this comment.
Findings from review of #232:
- Blocking: topology invalidation can be bypassed during load-time healing in
src/Game_io.cpp(Game::load).
The new caching work relies on Map::topologyGeneration changes when topology is mutated, but the load-time repair path still writes directly into case storage (for example setting map.getCase(...).building = gid and clearing path state directly). This can leave route/gradient caches with stale generation values after a repair/reload and reintroduce incorrect route field reuse.
Please route map mutations that affect topology through existing invalidation paths (or otherwise increment topologyGeneration) in load to force correct cache invalidation/rebuild.
- Follow-up: review terrain mutation paths in this PR (notably
Map::setTerrain) to confirm they should also trigger topology invalidation if terrain edits can occur after cached fields are created.
If you want, I can convert this into a request-changes review and split into line-level comments.
|
I merged current master ( Merging master needs more than the textual conflictsMaster took save-format 94 (#223) and 95 (#244) while this was open. The conflicts in
I can push that merge as a branch if it saves you the resolution. Results on the merged treeEnvironment: macOS 26.6.2, Apple M3, Apple clang 21.0.0, Simulation and save regressions from Negative control. With the Determinism. I ran
Each sidecar regenerates in a few seconds with Not covered: x86_64, and a Windows/MinGW checksum comparison. Both builds were arm64. Against master, as expected for an intentional simulation change: the first differing tick is 116 on SmallForTwo, 259 on Archipelago and 71 on Oazis. Cost after #223/#244. Whole-process CPU time, including load, on the same fixtures. Median of 3 runs alternating with master. The trajectories diverge early, so these are not identical games.
Run-to-run spread was under 1%. On Oazis that is roughly 1.9 → 2.3 ms/tick against the 40 ms budget. The +18% in the description was measured before #223/#244 landed; I haven't profiled where the difference comes from. Correction to my review
Small, non-blocking notes
|
VerdictChanges requested — one blocking item, the rest are notes. Taking the disclosure seriously: I wrote the generation design, so read this What blocks it is the rebase, not the idea. 1. BLOCKING — the format version has to be 96, and both gates with itIndependently confirmed, same conclusion genixpro reached.
if (versionMinor>=94)
topologyGeneration=stream->readUint32("topologyGeneration");
...
building->gradientGeneration[sw]=versionMinor>=94 ? stream->readUint32("generation") : topologyGeneration;A save written by master at 94 or 95 never wrote either field, so those reads Evidence. I resolved the merge myself (four conflicts: //! The map's topology generation and each cached field's generation stamp.
static constexpr int FILE_FORMAT_VERSION_TOPOLOGY_GENERATION = 96; // src/FileFormatVersions.hConfirmed both directions on x86_64 Linux (the last review's runs were arm64 loaded with
The third row is the same binary with a one-line difference in
2. The round-trip fields are derived from the field this invalidates, and they routeCorrecting the note in the last review that they "only price trips". They do not.
So after a footprint change the parent rebuilds within To be precise about what is and is not stale: How bad: bounded, and not a new hole. What I would ask for, cheapest first. Master already documents the lag at If you want the behaviour rather than the comment, stamping the parent's 3. The new harness does not test the three things the description claimsThe description says the generation "covers distance, flags, other teams and any Those are the cases Leo hit, so they are the right ones to pin. But the gap the Bob confirms there was no reason for the omission — the harness was written to 4. Load-time healing — genixpro's first review flagged this as blockingNoting it because it is easy to miss: it is in a review (2026-09-10 23:15
The ordering matters, and it is the opposite of what you might expect. Inside Net effect: a save whose building grid was already inconsistent gets healed, and I am not asking for it in this PR. I am asking for one sentence saying it is 5. Save/load round trip — checked, no finding
What I ranAll on the merge I resolved (branch
Negative control. With the Restored, it passes. So the new regression does test the fix, not the scaffolding Not covered by me: Windows/MinGW, the render, UI, LAN and server CI steps, and CostI reproduced the last review's numbers on x86_64 Linux and then measured where Timing. Wall clock for the whole process including load, median of 3 runs
Same shape as the arm64 numbers: the 11-team map is the expensive case, the rest Where it goes. I counted Attribution is checked dirty-flag-first, so the "generation mismatch" column
Two things fall out of that.
The cost is inherent to the guarantee — one placement anywhere invalidates every Nits, none blocking
|
|
I measured this branch rather than argue about it, and the headline is that the design is right but the constant was wrong. I've pushed one commit (8f4fca1) raising Everything below is macOS/arm64, First: your numbers reproduce exactlyIndependent instrumentation, same fixtures: generation-caused rebuilds came out 2862 / 268 / 690 on gd-bigarena-long / gd-archipelago / cross-replay — matching your reported figures to the digit. Clean-binary wall clock also lands where you and I both had it: +21.2% on Oazis at 4000 ticks (9.02s → 10.93s), +8.0% on archipelago, and no measurable change on cross-replay. I also confirmed the attribution claim: the cost is entirely the BFS. On Oazis The finding that changes the picture: cost scales superlinearly with game lengthOazis, doubling the horizon:
Buildings accumulate, so every bump invalidates more live fields — cost grows with the product of placements and fields. The +20% we both measured was a 4000-tick figure and understates it. Late game is both where this is most expensive and where people fast-forward. Worth knowing before picking a number. The severity number nobody hadRebuilds don't sum to the attributed causes; the remainder is
The bug is real, frequent, and this branch essentially eliminates it. That is the strongest evidence in favour of merging, and it wasn't in the PR. Your dead-weight finding is right, and stronger than you stated
The throttle sweep, and why 100Oazis, 8000 ticks ("stuck" = the symptom above):
The cost curve flattens almost exactly where the correctness curve breaks. At 100 the fix is fully retained; past ~200 stuck rebuilds climb back toward master and the branch stops earning its cost. Same shape at 4000 ticks and on gd-large-4ai (master 67 → 25:3 → 100:6 → 200:49). Worst-case staleness becomes 100 ticks, 4s at normal speed, typically about half that, and it only ever costs a detour — Two optimisations I tried that failedReporting the negative results so nobody repeats them:
The boring constant beat both comprehensively. Still outstanding (not from me)
CaveatsEvery config produces a different game (all checksums differ), so these are comparisons across workloads, not a controlled A/B — the trends are monotonic and replicate across three maps and two horizons, but individual figures shouldn't be over-read. The stuck counter is noisy at the low end (1/5/0 for ticks 25/50/100). I did not pin the boundary between 100 and 200 precisely. macOS/arm64 only; no Windows or cross-architecture checksum comparison. |
A 3x3 block of inn sites: when the centre is placed first, its cached route field is never told that the ring closed around it, so units outside are still offered the site (ring-after fails on master). When the ring stands first, the centre's field is built after the wall and is locked at once (ring-before passes). Clearing the ring restores the site in both cases through Team::syncStep's existing invalidation. Opus 5 helped authoring this commit.
A building's global gradient is recomputed only when it is missing or has been marked dirty, and dirtyBuildingGradients fires on destruction and on flag and area orders. Nothing marks it dirty when the ground around the building opens up, so a field computed while the site was walled off stays frozen: on a four-player FourSquares game a swarm construction site went unreachable to all 28 workers of its colony at tick 35328, its gradient covering 882 cells and ageing past 25000 ticks without a rebuild, and the site never finished. pathfindBuilding already rebuilds a gradient older than STUCK_REBUILD_TICKS when a unit cannot make progress, but that only runs for a unit already walking to the building, and a building nobody can reach is offered no worker. The only path that could unstick it required it not to be stuck. Give buildingAvailable the same rebuild at the same rate limit. Over 26000 ticks of that game it takes the buildings no worker can reach from 13 to 0, for about a second of extra work. Opus 5 helped authoring this commit.
Replaces the timed rebuild with the invalidation that was missing. Map::setBuilding writes a footprint into the tile grid and nothing else happens. Every building whose gradient routes through those tiles still believes they are walkable, and a building that has just been placed is computed against neighbours that do not know about it. dirtyBuildingGradients was only ever called for flag ranges, area orders and demolition, so placement, completion and the upgrade that moves a footprint were all silent. dirtyBuildingGradientsAround marks every affected field, for every team - a building blocks tiles for everyone, and the existing helper only ever dirtied the caller's own team. It is called from the four places in the simulation that move a footprint. Not from setBuilding itself: that also runs while a map is loading, before the teams exist. The timed rebuild this replaces was justified by a count of buildings whose field reported them unreachable, and that count is not a measure of the bug: a stale field is a generous field, so correcting one *raises* it. On the same six-seed bed the invalidation leaves the number of construction sites that stop progressing unchanged, peaking at one either way - the AI does not pack buildings tightly enough to reproduce what a human player hits. It is offered as a correctness fix: a field that has not been told about a building is wrong, whatever the count says. Also says what buildingAvailable's nine-cell probe is for, which the previous comment did not. test/TestsRunner: 170 tests pass. Opus 5 helped authoring this commit.
Game::addBuilding is the one path every placement takes: player orders, queued build projects, map generators, the editor and story scripts. It stamped the footprint on the map and told nobody, so a field built before the site went up kept routing through it and units were hired for a building they could no longer reach, until one of them got stuck long enough to force a rebuild. The call has to come after the building is stored in myBuildings: the dirty walk looks the fresh gid up there, and a null slot crashes it. That is what made an earlier attempt from inside Map::setBuilding fail. Opus 5 helped authoring this commit.
Invalidation walked the tiles inside the changed footprint widened by sixteen, and dirtied whatever buildings it found there. Route fields span the map, so proximity was never the right question: a building sixty tiles away routes through the changed cell just the same and was never told. The same walk reads the building tile grid, which virtual buildings are never written into, so no flag's field was reachable by it at all, and ordinary demolition dirtied only the owner's buildings. Map::topologyGeneration is bumped when a footprint is stamped or lifted and when a forbidden area is painted. Each field records the value it was built at, and buildingGradient compares the two. Distance, flags, other teams and any future way of moving a footprint are covered by construction, and nothing has to remember to call anything. Resources growing or being cleared, and units becoming immobile, also change what a field may route through and are deliberately left out. Counting the changes over one 8-player match: 50594 immobile and 10152 resource against 1010 structural. Bumping on all of them costs 47% of simulation time, against 18% for the structural ones alone - 0.6 ms per tick becoming 0.7 against a 40 ms budget. Neither has ever invalidated a field, so leaving them out is the behaviour that was already there. Verified against the review probe from codex/review-building-invalidation: the placement, distant-removal and demolition cases all read CORRECT, the last two with no dirty flag set at all. building-gradient-invalidation-test, immobile-unit-gradient-test, building-footprint-test and TestsRunner (178) pass. Opus 5 helped authoring this commit.
Every footprint change already reaches Map::setBuilding, and every forbidden mask change now reaches the three Map setters, so the counter sees all of them. The proximity walk that dirtied buildings within sixteen tiles of a footprint is gone, together with its five call sites: it flagged a subset of what the generation rebuilds anyway. removeForbidden bumps like its two siblings; lifting a forbidden area opens ground a field may have marked impassable. The build-project and area orders wrote the forbidden mask straight into the tile array, bypassing the setters and therefore the counter; they use the setters now. The two rebuild conditions in buildingGradient shared the same throttle and are one condition. Opus 5 helped authoring this commit.
Opus 5 helped authoring this commit.
The runtime routing state kept each field's dirty flag and last-update tick but neither the map's generation nor the value a field was built at. A loaded game therefore saw every restored field as stale, rebuilt it at its next use, and drifted from the uninterrupted run from that tick on. The savegame safety harness caught it in its continuation check on every CI job. Both values now travel with the routing state under FILE_FORMAT_VERSION_TOPOLOGY_GENERATION, which is 97: master took 94, 95 and 96 for other formats while this branch was open, and a save written by any of them never wrote these fields. Reading them behind a lower gate would consume the next values in the stream and fail the load. A save from before 97 restores its fields as current, which is what it did. Opus 5 helped authoring this commit.
The topology generation invalidates every cached route field of every team on any structural map change. That is what makes it correct by construction, and it is also why the branch is expensive: DIRTY_REBUILD_TICKS, not the bump itself, is what bounds the resulting rebuild load. Measured with per-cause counters on gd-bigarena-long (Oazis, 11 teams), master vs this branch, macOS/arm64. "stuck" counts rebuilds forced by a unit that could not make progress -- the symptom this branch exists to remove. 8000 ticks rebuilds generation-caused stuck bfs time wall master 2906 0 195 2.091s 16.73s ticks=25 12603 9755 1 9.183s 23.86s (+42.6%) ticks=100 5296 2480 0 3.860s 18.46s (+10.3%) ticks=200 4178 1222 57 3.058s 18.24s Raising the throttle to 100 keeps the fix intact and cuts the regression by roughly three quarters. Past 200 the fix erodes as stuck rebuilds climb back toward master. The same shape holds at 4000 ticks and on gd-large-4ai (master 67 stuck, 25 -> 3, 100 -> 6, 200 -> 49). Worst-case staleness becomes 100 ticks (4s at normal speed), and only ever costs a detour: Map::directionByGradient re-tests live passability on every candidate step, and STUCK_REBUILD_TICKS remains as a backstop. Simulation results change, but this branch already bumps the save/replay version for that reason. Claude-Session: https://claude.ai/code/session_01A5GEG5t9ithd47SFm6PTDZ Opus 5 helped authoring this commit.
Raising DIRTY_REBUILD_TICKS from 25 to 100 left behind a copy of the old
value in the regression that exists to catch this class of bug.
BuildingGradientInvalidationHarness advances 25 ticks and then asks
whether a walled-in site is still offered; with the engine rebuilding no
sooner than 100 ticks after a field was built, no rebuild can have
happened yet, so the harness reads the stale field it is there to reject
and fails:
FAIL: a site walled in by new construction is no longer offered to a
unit outside
The interval moves to EngineTiming.h, whose whole purpose is that
anything expressed in ticks lives in one place "to avoid drift between
Engine, Map, Team, Building, Unit, and AI subsystems". The gradient
cadence beside it, CLEARING_FLAG_REFRESH_TICKS, is already there and
already read from MapPathfindBuilding.cpp. The harness now reads the
same constant, so the next person to tune it cannot silently stop
covering the interval.
The measurement that chose 100 travels with the constant.
Opus 5 helped authoring this commit.
The description claims the generation covers distance, flags and other teams. The harness covered none of that: one team, real buildings only, both scenarios placement-around-a-cached-field for team 0. Those are the cases Leo hit, so they are the right ones to pin, but the two gaps the review found are exactly the two that pass by construction today and would break silently if invalidation were ever re-narrowed. ring-other-team builds the ring as team 1 around team 0's site. The walk this replaced dirtied only the buildings of the team that made the change, so another team's wall went unnoticed. It also pins a real asymmetry: Team::syncStep frees the demolishing team's fields, so when the rival clears the ring the owner's field is invalidated but not freed, and comes back on the next rebuild the interval allows rather than on the next lookup. ring-flag puts an exploration flag at the centre, with its attraction radius set to 1 so its goal disc stays inside the ring. A flag is never written into the building tile grid, so a walk over the changed footprint could not discover its field at any distance at all. Both fail with the generation test removed from Map::buildingGradient, as does ring-after; ring-before passes either way, since nothing is cached before the ring exists. That is what makes it insufficient alone. test/README.md gains the section every other harness has. Opus 5 helped authoring this commit.
Map::dirtyBuildingGradients walks a rectangle around a change, looks up the building gid on each tile, and marks that building's fields dirty. Instrumentation across gd-bigarena-long, gd-archipelago and cross-replay found no rebuild anywhere that the dirty flag caused and the generation would not have - not on this branch, and not on master either. It was already inert before this branch existed, which is plausibly why the bug it was meant to prevent was there at all. It is redundant by construction, not just by measurement. Each of its five callers pairs it with a Team::dirtyGlobalGradient() on the same team in the same path, and that frees every field of every building of that team through resetPathfindGradients. Marking a subset of the same team's buildings dirty afterwards cannot change an outcome: executeModifyFlag dirtyGlobalGradient() immediately before executeMoveFlag dirtyGlobalGradient() under the same condition executeAlterForbidden dirtyGlobalGradient() immediately before executeDelete dirtyGlobalGradient() immediately before Team::syncStep isDirtyGlobalGradient, applied after the loop Two of them are flag paths, where the walk could only ever have found real buildings anyway: a virtual flag is not in the building tile grid. Building::dirtyGradient stays. Map::pathfindBuilding still sets it when a clearing flag is standing on a resource that is gone, which is the one remaining producer, and it is still saved and restored. GRADIENT_DIRTY_BORDER_TILES goes with its only caller. Opus 5 helped authoring this commit.
Both were found in review, both are out of scope here, and both are the kind of thing the next reader rediscovers as a blocker. Round-trip fields are children of the walking field this invalidates: Map::updateRoundTripGradient seeds every resource tile from building->globalGradient[swimClass]. A generation bump rebuilds the parent on its next use and does not propagate to the child, which refreshes only on its own 120-tick timer, and fetchers walk it. What goes stale is the carry cost, not reachability - the child takes its obstacle marks from the resource gradient, and directionByGradient re-tests live passability on every candidate step - so the worst case is a detour. Stamping the parent's generation on the child would rebuild every live child on every structural change, one full-map pass each, on top of the parent rebuilds already measured. On these beds that is the wrong side of the trade. Game::checkBuildingsDoNotOverlapAndHealMissing writes into the building tile grid directly, so it does not bump the generation - and bumping it there would not help, because integrity() runs before Map::loadRuntimeState, which restores the saved generation and the per-field stamps over anything the heal set. It fires only for saves whose grid was already inconsistent, and the same gap predates this change: fields were restored after the heal without being dirtied before it too. A fix has to record that the heal touched a cell and bump after loadRuntimeState. Opus 5 helped authoring this commit.
addForbidden and removeForbidden bumped unconditionally, including when the team's bit was already in the state being written. A bump throws away every cached route field of every team in the game, so repainting a tile that was already forbidden cost a full rebuild of everything and bought nothing. The three callers that matter - executeCreate, buildProjectSyncStep and executeAlterForbidden - always make a real change on the tiles they touch, so this is not a measured regression. It is a whole class of surprise removed from the forbidden brush, which is the one place a player can repaint the same tile as fast as they can drag. Map::setForbidden goes: it has no caller anywhere under src/, and the setForbidden in test/MapQueryTest.cpp is a different method on that file's own test double. Leaving it would mean maintaining a bump inside a function nothing calls. Opus 5 helped authoring this commit.
8f4fca1 to
d9c885b
Compare
|
Status update, since master has moved a long way under this branch today — #252, #253, #250, #259 and #262 have all landed. The blocking item from the last round has resolved itself favourably. master is now at One thing to watch while doing it: #256 also claims 97. Whichever of the two lands second needs 98, along with every gate that references it. #257 claims 98 today, so it will need to move as well. Three branches are currently converging on the same two numbers. The The design question you raised for someone other than you to answer — whether excluding resource changes and immobile units from topology-generation bumps is the right trade — I'd answer yes. So from my side this is rebase-and-merge, not rethink. |
2 participants
A building's route field records which cells were obstacles when it was computed, and nothing reliably told it when that stopped being true.
Game::addBuilding— the one path every placement takes: player orders, queued build projects, map generators, the editor, story scripts — stamped the footprint onto the map and told nobody. A field built while the ground was open kept routing through it, units were hired for a building they could no longer reach, and they only gave up once one of them was stuck long enough to force a rebuild, at which pointlockedwas set and the whole crew dropped the job at once.Removal was never the problem:
ORDER_DELETEreachesTeam::syncStep, which dirties the rectangle and then callsdirtyGlobalGradient(), freeing every field of the team.Why proximity was the wrong criterion
The first version of this branch dirtied the buildings whose tiles fell inside the changed footprint widened by
GRADIENT_DIRTY_BORDER_TILES. Route fields span the map, so distance is not the question — a building sixty tiles away routes through the changed cell just the same. The same walk reads the building tile grid, which virtual buildings are never written into, so no flag's field was reachable by it at all. And ordinary demolition dirtied only the owner's buildings.Thanks to @genixpro for the review that found all three, with a reproduction fixture rather than an argument.
The change
Map::topologyGenerationis bumped when a footprint is stamped or lifted and when a forbidden area is painted. Each cached field records the value it was built at;buildingGradientcompares the two. Invalidation is O(1), reads no tiles, and covers distance, flags, other teams and any future way of moving a footprint by construction.How much it is worth, and what it costs
@genixpro measured the thing the branch is actually for —
pathfindBuilding's stuck-unit path callingupdateGlobalGradientdirectly, i.e. a unit stuck long enough to force its own rebuild, which is the user-visible symptom:The cost is one number:
GRADIENT_DIRTY_REBUILD_TICKS, how long an invalidated field may be reused before it is rebuilt. At the original 25 the branch cost +42.6% simulation time on Oazis over 8000 ticks; at 100 that is +10.3% with the symptom still fully suppressed, and the fix erodes past ~200 (57 stuck at 200, 76 at 400, converging back on master's 195). 100 sits inside the safe range rather than at its edge. Worst-case staleness is 100 ticks, about 4 s at normal speed, and it can only ever cost a detour:Map::directionByGradientre-tests live passability on every candidate step, andSTUCK_REBUILD_TICKSremains as a backstop.The constant now lives in
src/EngineTiming.hbeside the other tick cadences, including theCLEARING_FLAG_REFRESH_TICKSthat the same function already reads from there, andtest/BuildingGradientInvalidationHarness.cppreads it rather than keeping a copy. That copy is why8f4fca1as pushed failed the regression it was raising the constant under: the harness advanced 25 ticks, no rebuild could have happened, and it read the stale field it exists to reject.Deliberately not covered
Resources and immobile units. They also change what a field may route through. Counting every passability change over one 8-player match:
A glob stepping into a building is 82% of them. Bumping on everything cost 47% of simulation time across three seeds. Neither has ever invalidated a field, so leaving them out keeps the behaviour that was already there rather than opening a new hole, and
directionByGradientrechecks live passability, so the exclusion cannot walk a unit into a wall.Round-trip fields. They are children of the field this invalidates —
Map::updateRoundTripGradientseeds every resource tile frombuilding->globalGradient[swimClass]— and a parent generation bump does not propagate to them; they refresh only on their ownROUND_TRIP_REFRESH_TICKStimer, and fetchers walk them. What goes stale is the carry cost, not reachability: the child takes its obstacle marks from the resource gradient, so the worst case is a detour. Stamping the parent's generation on the child would rebuild every live child on every structural change, one full-map pass each, which is the wrong side of the trade on these beds. Now written down atMapPathfindBuilding.cpp.Load-time healing.
Game::checkBuildingsDoNotOverlapAndHealMissingwrites into the building tile grid directly, so it does not bump the generation — and bumping it there would not work, becauseintegrity()runs beforeMap::loadRuntimeState, which restores the saved generation over anything the heal set. It fires only for saves whose grid was already inconsistent, and master has the same gap today: it restores cached fields after the heal without dirtying them. A fix has to record that the heal touched a cell and bump afterloadRuntimeState. Out of scope here, now written down atGame_io.cpp.The proximity path is gone
Map::dirtyBuildingGradientsis deleted. Instrumentation found no rebuild across three fixtures that its dirty flag caused and the generation would not have — and none on master either, so it was already inert before this branch existed. It is also redundant by construction: each of its five callers pairs it with aTeam::dirtyGlobalGradient()on the same team in the same path, which frees every field of that team throughresetPathfindGradients.Building::dirtyGradientstays;Map::pathfindBuildingstill sets it for a clearing flag standing where its resource used to be.Map::setForbiddenis deleted with it — no caller undersrc/.addForbidden/removeForbiddennow bump only when the tile's bit actually changes, so repainting an already-forbidden tile no longer throws away every cached field in the game.Save format
FILE_FORMAT_VERSION_TOPOLOGY_GENERATION = 97, named insrc/FileFormatVersions.hbeside master'sFILE_FORMAT_VERSION_ROUND_TRIP_FIELDS, and bothMapIO.cppgates read it.VERSION_MINORandREPLAY_MINIMUM_VERSION_MINORgo to 97. 94 (#223), 95 (#244) and 96 (#253) all landed while this was open and none of them wrote these fields, so a lower gate would consume the next values in the stream and fail the load.Verification
Debian x86_64, g++ 15.3.0,
release=1 server=0, each harness with a disposableGLOB2_USER_DIR, on this head.Regressions from
build.ymlthat this change can reach: 20/20 pass —building-gradient-invalidation,immobile-unit-gradient,building-footprint(plain and--load),entering-unit-save(plain and--load),trapped-unit,building-expel,hiring-bucket,resource-fetch-target,round-trip-hunger-gate,inn-swap,savegame-safety,buffered-file,team-stats-save(plain and both legacy fixtures),global-gradient,terrain-resources,selection-lifetime.test/TestsRunnerisOK (187 tests)and every harness undertest/passes. The client, the software client and the YOG server all build clean.The harness now covers what the description claims.
test/BuildingGradientInvalidationHarness.cppgained two scenarios on the same fixture:ring-other-teambuilds the ring as team 1 around team 0's site, andring-flagputs an exploration flag with its goal disc inside the ring at the centre. Those are the two cases a footprint walk structurally could not reach.Negative control. With
gradientGeneration[swimClass] != topologyGenerationremoved fromMap::buildingGradientand nothing else changed:So the regression tests the fix, and the two new scenarios are not passing on scaffolding.
Not covered by me: Windows/MinGW, the render, UI, LAN and server-run CI steps, and a cross-architecture checksum comparison. The arm64 and macOS side is in the reviews above.
Not included
Nothing. The CI step for the new harness is in
.github/workflows/build.ymlin this branch.🤖 Generated with Claude Code