AI trainer support: refactor, infrastructure, and one desync fix#129
Draft
kylelutze wants to merge 151 commits into
Draft
AI trainer support: refactor, infrastructure, and one desync fix#129kylelutze wants to merge 151 commits into
kylelutze wants to merge 151 commits into
Conversation
…ents Migrates three widely-used Boost libraries to their standard C++11 replacements, eliminating these dependencies entirely: - boost::shared_ptr/weak_ptr → std::shared_ptr/weak_ptr (~140 files) - boost::lexical_cast → std::to_string/std::stoi (14 files) - boost::tuple → std::tuple with std::get<N>() syntax (13 files) Removes configure checks for boost/shared_ptr.hpp, boost/tuple, and boost/lexical_cast.hpp from SConstruct. Remaining Boost deps are date_time, tribool, random, integer, and functional.
Carried forward from the feat/modernization working tree onto the
ai-trainer-support branch. Adds a binary sidecar writer that records
per-tick, per-entity checksum field vectors, gated by the
GLOB2_CHECKSUM_SIDECAR env var. Used by the Rust port's cross-replay
test to pinpoint field-level divergence between C++ and Rust simulation.
Includes the new ChecksumSidecar.{h,cpp} module, SCons wiring,
init/teardown hooks in Engine.cpp, and accompanying notes in
docs/headless-replays.md and docs/duplicate-functions.md.
Previously the replay writer always emitted to a hardcoded "replays/last_game.replay", overwriting on every game. The AI-trainer pipeline (and any external tool driving headless games for data collection) needs distinct per-game output paths and the ability to run concurrent instances without clobbering each other. When GLOB2_REPLAY_PATH is set, it replaces the hardcoded path for both the ReplayWriter and the checksum-sidecar base. Default behavior is unchanged. Documented alongside GLOB2_CHECKSUM_SIDECAR in docs/headless-replays.md.
After the existing "automaticEndingGame ended" log, print a single machine-parseable line with the final tick count, the winner team (first team with hasWon set, or -1 on timeout), and the list of team/player-type pairs. Lets the AI-trainer pipeline label and filter generated replays by outcome and matchup without re-parsing the replay header. Format: GLOB2_GAME_END ticks=2483 winner_team=1 players=team0:local,team1:Nicowar,team2:Warrush Documented in docs/headless-replays.md.
createRandomGame previously drew uniformly from NUMBI..NICOWAR for every AI slot. This makes generated -test-games-nox replays diversity-balanced but heavy on the weaker AIs, which dilutes the training signal for the AI-trainer pipeline. The new --ai-types flag accepts a comma-separated, case-insensitive list of AI names (numbi, castor, warrush, reachtoinfinity, nicowar, toubib). When set, createRandomGame draws uniformly from that list instead. Empty / unset preserves legacy behavior. Examples: ./glob2 -test-games-nox 100 --ai-types nicowar,warrush ./glob2 -test-games-nox 50 --ai-types nicowar # self-play Documented in docs/headless-replays.md and in --help.
Toolkit::getFileManager()->openFP() unconditionally prepends each
search dir from its dirList onto the requested filename. For an
absolute path like "/tmp/foo.replay", the result becomes
"~/.glob2//tmp/foo.replay" (and similar for the other search dirs),
none of which exist — fopen returns NULL and the init() assertion
fires.
When GLOB2_REPLAY_PATH is set to an absolute path (the natural use
case for the AI-trainer pipeline writing per-game replays to an
arbitrary directory), we want the path used as-given. Detect the
leading '/' and call fopen directly in that case; relative paths
keep their existing FileManager-resolved behavior.
Verified end-to-end with:
GLOB2_REPLAY_PATH=/tmp/test.replay \
./build/src/glob2 -test-games-nox 1 --ai-types nicowar
Replay (860 KB) landed at the requested absolute path; previously
this crashed in ReplayWriter::init.
Three additional sidecar fields the AI-trainer pipeline can consume
without re-parsing the replay binary:
seed=<u32> from GameHeader::getRandomSeed() — for reproducibility
and dedup across runs
map="<name>" from MapHeader::getMapName() — quoted to handle map
names with spaces (e.g. "Auto save")
orders=<u32> from a new ReplayWriter::getOrderCount() — useful for
filtering out low-activity / early-timeout games at the
bash/jq level before paying the cost of parsing the
replay binary
ReplayWriter now tracks ordersWritten in pushOrder, incremented after
the existing voice/null early-returns so the count matches what is
actually persisted.
Updated example in docs/headless-replays.md. End-to-end verified:
GLOB2_GAME_END ticks=50594 winner_team=4 seed=1777220686
map="Auto save" orders=9547 players=team0:local,team1:Nicowar,...
Lets the AI-trainer pipeline drive curated matchups (specific AI per
team on a specific map) instead of the legacy fully-random
-test-games-nox flow. Random behavior is preserved as default; nothing
existing breaks.
New flags:
--map <name> Pin the map. Bare filename, no .map extension;
resolved as maps/<name>.map. Fails fast on missing
file (the legacy retry loop in createRandomGame()
would otherwise spin forever on a typo'd name).
--matchup <list> Comma-separated AI names; matchup[k] plays team k.
Validated against the loaded map's
getNumberOfTeams() before launch. Requires --map
(we need the team count to validate). Mutually
exclusive with --ai-types (pool vs. exact).
Pre-refactor: extracted AINames::parseAIName() so --ai-types and
--matchup share one canonical name->id table. The hardcoded table is
now in AINames.cpp next to getAIText/getAIDescription.
Implementation notes:
- chooseRandomMap() short-circuits to loadMapHeader("maps/<map>.map")
when --map is set.
- createRandomGame() (parameterless) skips the legacy retry loop on
--map and detects load failure via numberOfTeams==0 (loadMapHeader
doesn't throw; it logs and returns a default-constructed header).
- createRandomGame(int) uses matchup[teamColor] when set; the existing
i==0 P_LOCAL placeholder stays so engine code paths that depend on
gui.getLocalTeam() still work — the local player is a passive
watcher and matchup[0]'s AI plays team 0 via the wrap-around at
i==numberOfTeams.
Validation tested end-to-end:
- --matchup nicowar,warrush,numbi --map A_big_pond → 3-team game,
AIs assigned correctly to teams 0/1/2.
- --map ThisDoesNotExist → exit 1 with "cannot load" message.
- --matchup nicowar,warrush --map (3-team map) → exit 1 with
team-count mismatch message.
- --matchup ... --ai-types ... → exit 1 with mutually-exclusive
message at parseArgs.
- --matchup bogus → exit 1 with "unknown AI" message.
Documented in docs/headless-replays.md.
macOS Finder writes .DS_Store metadata files into every directory that's been browsed. They shouldn't be tracked.
Game::save() unconditionally called
mapHeader.setMapName(name) and setIsSavedGame(!fileIsAMap) to shape
the on-disk record, then never restored them. Every save() therefore
clobbered the live mapHeader's mapName — observable in two places:
- ReplayWriter::init writes the initial state header via
gui.save(buffer, "replayHeader") at game start, leaving live
mapName = "replayHeader" for the rest of the game.
- GameGUI::syncStep auto-saves every 256 ticks via
save(stream, "[auto save]"), leaving live mapName = "Auto save"
after the first auto-save.
The second clobber landed in the AI-trainer pipeline's
GLOB2_GAME_END "map=" field — every game reported map="Auto save"
regardless of what was actually loaded.
Fix: snapshot mapName + isSavedGame at the top of Game::save and
restore them at the bottom. The on-disk header still has the
intended name; only the in-memory state is left untouched.
MapEdit::save() relied on the post-save mutation to keep its
"current map name" UI updated (LoadSaveScreen reads
mapHeader.getMapName() to populate the dialog default). Re-apply
the rename explicitly there to preserve the editor UX.
End-to-end verified:
GLOB2_GAME_END ticks=34050 winner_team=0 seed=1777223357
map="A big pond" orders=3741 players=...
(Previously map="Auto save" in this exact scenario.)
Triggered by GLOB2_DATASET_PATH env var (mirrors GLOB2_REPLAY_PATH and
GLOB2_CHECKSUM_SIDECAR). Writes one binary record per executed action
order, alongside the .replay. The trainer pipeline consumes these
directly without having to re-simulate the replay (the Rust port's
simulator isn't yet complete enough to drive a re-simulation pass).
Format (little-endian, see docs/headless-replays.md):
HEADER: magic "GDS1", u32 format_version, u32 num_records,
u32 flags
RECORD: u32 tick, u8 sender, u8 order_type, u16 padding,
u32 state_blob_len + bytes, u32 payload_len + bytes
State blob is empty in format version 0 — the schema is wired up but
observation features are intentionally deferred to a follow-up commit.
The trainer-side parser rejects unknown versions explicitly so v1
(features online) won't silently produce junk.
writeRecord skips ORDER_NULL and ORDER_VOICE_DATA matching
ReplayWriter::pushOrder's filter — without that, every per-tick
"do nothing" order would be logged and num_records would dwarf the
actual decision count (4 players × 30k ticks ≈ 120k records, of
which only ~4k are real AI decisions). With the skip, num_records
matches the GLOB2_GAME_END `orders=` field exactly.
End-to-end verified:
GLOB2_DATASET_PATH=/tmp/g.dataset GLOB2_REPLAY_PATH=/tmp/g.replay \
./glob2 -test-games-nox 1 --map A_big_pond --matchup nicowar,warrush,numbi
→ /tmp/g.dataset header.num_records = 4533, matches orders=4533
Hooked in Game::executeOrder alongside the existing ReplayWriter
push, initialised in Engine::initGame next to the checksum sidecar,
torn down at game end. Absolute paths bypass FileManager (matching
ReplayWriter's prior fix).
These were defensive-engineering for a migration scenario that doesn't
exist: there's a single producer (this writer) and a single consumer
(the trainer's dataset.rs parser), and regenerating any dataset takes
seconds. We never need to support multiple wire formats in flight.
Header shrinks from 16 bytes to 8:
HEADER (8 bytes)
[4B] magic "GDS1"
[4B] u32 num_records (patched at close)
Per-record drops the 2-byte alignment-cosmetic padding.
If the schema ever changes wire-incompatibly, bump the magic to GDS2
and parsers reject by magic mismatch — that's the actual safety net,
not a version field.
Verified end-to-end: 3739-record dataset, parses cleanly, sidecar
shows dataset_records=3739 matching orders=3739.
writeRecord now takes Game& and serializes the sender's view of the world before the action half: bot-team scalars (prestige, alive/won/lost flags, teamRessources, unit/building counts by type) plus a FOW-filtered 32x32x7 spatial grid downsampled from the live map. Per-cell channels are terrain, resource amount, my/enemy unit counts, my/enemy building type, and discovery state (unknown / previously seen / currently visible). Vision is masked through team.me | sharedVisionExchange | sharedVisionFood | sharedVisionOther so the supervision signal matches what the source AI saw when it made the decision. Records grow from action-only to ~7.3 KB; verified against A_big_pond with nicowar/warrush/numbi (4056 records, discovery channel evolves from mostly-unknown to fully-explored as expected). Layout doc lives in DatasetWriter.h.
Map.cpp is too large to navigate or port piecemeal. Split it into behaviour-grouped translation units (lifecycle, IO, step, terrain, resources, query, view, gradient global/local/building/area, minigrad, pathfind ressource/building/area, misc, log) so each file stays under ~500 lines, matching the porting tracker's chunking and the Rust port's file-size convention. Map.h is unchanged — this is purely an implementation-side split. Two new private headers: - MapInternal.h — extern const direction tables (deltaOne, tabClose, tabFar), UPDATE_MAX, fill<>, clip_0_31 inline. Shared across the Map*.cpp family. - MapGradientImpl.h — bodies of updateGlobalGradient<T> and the VersionSimple/Simon/Kai helpers. Forbidden/guard/clear/building gradient templates (in three separate .cpp files) call into these templates, so the bodies must be visible at every instantiation site. Side cleanup: dropped dead helper fillGradientCircle (defined in the original Map.cpp but never called anywhere in the codebase). Open follow-up: MapLog.cpp is 602 lines because Map::logAtClear is one 567-line function. Decomposing it into per-stat-category helpers is a separate refactor and not done here.
Splits the monolithic GameGUI.cpp along behavioral boundaries: - GameGUIDraw.cpp frame-level draw + small primitives - GameGUIDrawPanels.cpp right-panel content (drawBuildingInfos, etc.) - GameGUIInput.cpp processEvent, handleKey, handleMenuClick, etc. - GameGUISelection.cpp selection mode + iterate/center - GameGUIParticles.cpp particle generation/movement/draw - GameGUIScript.cpp script API surface (enable/disable, hilights) - GameGUIInternal.h shared #defines + InGameTextInput declaration GameGUI.cpp keeps lifecycle/core (ctor, init, step, sync, save/load, executeOrder, addMessage). No header changes; all methods remain GameGUI members so the split is purely physical. Also removes dead bookkeeping the split surfaced: - handleKey: 'modifier' int set to +1/-1 but never read. - drawBuildingInfos / handleMenuClick: 'j' counters that walked alongside ypos but were never consulted (vestiges of an older drawValueAlignedRight pattern still used in the upgrade preview).
Splits the monolithic Game.cpp along behavioral boundaries:
- Game_orders.cpp executeOrder (the per-order-type switch)
- Game_io.cpp load, save, integrity,
checkBuildingsDoNotOverlapAndHealMissing, checkSum
- Game_sync.cpp syncStep + buildProject/won/script/prestige sync,
dirtyWarFlagGradient
- Game_editor.cpp queries (isTeamAlive, unitsCount, ...) and editor
utilities (addTeam, addUnit, addBuilding,
checkRoomForBuilding, removeUnit..., getUnit)
- Game_render.cpp drawPointBar, drawUnit, drawMap*, drawUnitPathLine*,
drawUnitOffScreen, drawMap
Game.cpp keeps lifecycle/core (ctor/dtor, init, clearGame, set*Header,
setAlliances, setWaitingOnMask, dumpAllData, prestige getters). No
header changes; all methods remain Game members so the split is purely
physical. Bodies are byte-identical slices of the original Game.cpp.
src/SConscript updated to add the 5 new TUs to source_files. The
server build is unaffected (Game.cpp was not in server_source_files).
Reduces clutter in src/ (116 YOG files among 426). SConscript paths prefixed with yog/; SConstruct CPPPATH gains #src and #src/yog so existing #include "YOG..." (and YOG files' own includes of sibling headers) keep resolving with no source edits. Also guards the macOS CFBundle chdir block in Glob2.cpp main() with !YOG_SERVER_ONLY, matching the existing pattern in the file. Fixes a pre-existing `scons server=1` link error on macOS arm64 where CoreFoundation wasn't pulled in for the server target.
Mirrors the earlier YOG move (cbace7e4). Adds #src/AI to CPPPATH so existing flat #include "AIEcho.h" / "AI.h" etc. across ~25 consumers keep working unchanged. SConscript, vcproj/vcxproj, and debian/copyright updated for new paths. Sets up for splitting AIEcho.cpp (5931 lines) and other large AIs into per-namespace TUs under src/AI/AIEcho/, etc.
Pure extraction along existing namespace boundaries — no content changes, only the 44-line prelude (copyright + includes + using directives) duplicated into each file: Management.cpp (1247) ManagementOrder subclasses + RessourceTracker Construction.cpp (1006) Constraints + BuildingOrder + FlagMap + BuildingRegister Conditions.cpp (989) Condition + BuildingCondition subclasses ReachToInfinity.cpp (915) ReachToInfinity EchoAI implementation Echo.cpp (680) signature_*, MapInfo, Echo class Entities.cpp (604) Entity factory + 8 Entity subclasses Gradient.cpp (437) GradientInfo, Gradient, GradientManager SearchTools.cpp (361) Building/team/enemy iterators + BuildingSearch AIEcho.h is unchanged; #include "AIEcho.h" still works for all consumers. SConscript and vcxproj updated for the new source list.
Pure extraction along internal responsibility boundaries — no content
changes, only the 38-line prelude (copyright + includes + AI_FILE_*
defines + using directive) duplicated into each file:
Maps.cpp (770) all compute*Map routines: obstacle/space/
neighbour/work/hydratation/notGrass/wheat/enemy
Projects.cpp (585) addProject + addProjects + continueProject
Control.cpp (577) controlSwarms/expandFood/controlFood/
controlUpgrades/controlStrikes
Placement.cpp (557) findGoodBuilding + computeRessourcesCluster
+ updateGlobalGradient[NoObstacle]
Lifecycle.cpp (381) Project + Strategy + ctors + init + dtor +
load + save
GetOrder.cpp (366) getOrder + defineStrategy
State.cpp (242) enoughFreeWorkers + computeCanSwim/NeedSwim/
BuildingSum/WarLevel
AICastor.h is unchanged; #include "AICastor.h" still works for all
consumers. SConscript, vcxproj, and vcproj updated for the new source
list.
`Gradient::get_height` returns negative sentinels for non-distance states (-1 for obstacle, -2 for BFS-unreached). Callers using the `< dist` pattern silently treated those tiles as "in zone" — meaning the AINicowar farming heuristic and ReachToInfinity's farm-spot selector were placing farms on obstacle/unreached tiles in addition to genuinely-near-water tiles. On waterless maps this caused every wood/wheat tile to be classified as a farm spot. Add `Gradient::within_dist(x, y, max)` — a guarded wrapper that returns `0 <= get_height < max`. Replace the 11 buggy call sites in AINicowar.cpp (farming zone + cardinal expansions) and ReachToInfinity.cpp (farm placement). Other call sites were already correctly guarded with explicit `>= 0` checks (see e.g. AINicowar.cpp:1996-2039), confirming the original author knew about the negative sentinels — these 11 sites were just oversights. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pure extraction along internal responsibility boundaries — no content
changes, only the 35-line prelude (copyright + includes + using
directives) duplicated into each file:
Buildings.cpp (806) queue_buildings + queue_inns/swarms/racetracks/
swimmingpools/schools/barracks/hospitals +
order_buildings + order_regular_* +
manage_buildings/manage_inn/manage_swarm
Flags.cpp (495) compute_defense_flag_positioning + modify_points +
compute_explorer_flag_attack_positioning
Lifecycle.cpp (383) NewNicowar ctor + load + save + tick +
handle_message + selectStrategy + initialize
Attack.cpp (318) choose_building_to_attack + attack_building +
control_attacks + choose_enemy_target +
dig_out_enemy
Upgrade.cpp (213) choose_building_upgrade_type_level1/2 +
choose_building_upgrade_type +
choose_building_for_upgrade + upgrade_buildings
Phases.cpp (204) check_phases
Farming.cpp (179) update_farming + update_fruit_flags +
update_fruit_alliances
Strategy.cpp (123) NicowarStrategy + NicowarStrategyLoader
AINicowar.h is unchanged; #include "AINicowar.h" still works for all
consumers. SConscript, vcxproj, and vcproj updated for the new source
list.
| File | Lines | Contents | |-------------------------------|------:|--------------------------------------------------------------------------------------------------------| | src/Building/Misc.cpp | 539 | kill, unit/ressource management, exits, level, flag stats, eatOnce, happiness, conversion, integrity, checkSum | | src/Building/Step.cpp | 525 | step, subscribeToBringRessourcesStep, subscribeForFlagingStep, subscribeUnitForInside | | src/Building/Lifecycle.cpp | 527 | constructors, destructor, freeGradients, load/save, loadCrossRef/saveCrossRef | | src/Building/Update.cpp | 470 | building-site / units / general updates, upgrade-area forbidden zones, isHardSpaceForBuildingSite | | src/Building/Construction.cpp | 415 | resource needs/wishes, launch/cancel construction & delete, updateCallLists, updateConstructionState | | src/Building/TypeSteps.cpp | 382 | swarmStep, turretStep, clearingFlagStep | Pure extraction: every line is a byte-exact copy of the original. Each TU duplicates the original 38-line prelude (copyright + includes). The mid-file `#include "GameGUI.h"` (was line 1333) travels with Update.cpp. Building.h is unchanged.
- Add anonymous-namespace templates countUnitsIf/findUnitIf and countBuildingsIf/findBuildingIf; rewrite the 9 query methods as one-liners (drops ~150 lines of for-loop boilerplate). - Fix bitwise-and where modulo was intended in placeGuardAreas: `(b->posY+y) & map->getH()` -> `% map->getH()`. Every other place in the codebase uses `& map->wMask`/`& map->hMask`; this was the only `& getH()` and is inconsistent with the `% getW()` next to it. - Drop duplicate `#include "Building.h"` and unused `<valarray>`.
- Move the 8-neighbor BFS expansion out of Gradient::recalculate
into a new private method Gradient::expand_bfs(queue<position>&).
Definition lives in a new GradientBFS.cpp (along with the small
Gradient/GradientInfo ctors) so tests can link without dragging
in Map / Game / GlobalContainer.
- Replace the 8 near-identical neighbor branches with a single loop
over a fixed-order 8-element offset table. Push order is preserved
exactly -- lockstep networking depends on it; flagged in a comment.
- Delete dead commented blocks at Gradient.cpp:251-253 (old malloc
pattern, superseded by gradient.resize()) and ReachToInfinity.cpp
:255-257 (stale BuildingSearch snippet replaced by team_stats).
Tests (test/GradientBFSTest.{h,cpp}) drive expand_bfs directly on a
real Gradient instance via friend access, asserting against toroidal
8-connected Chebyshev distance. Covers: empty queue, single source,
wrap-around, obstacle preservation, multi-source minimum distance,
fully-enclosed unreachable cell, queue-drained postcondition.
test/SConstruct gains a darwin branch with the include paths needed
to compile against AIEcho.h transitively (SDL2, libgag, glob2 root,
HAVE_CONFIG_H). Standard switched to gnu++14 so PerlinNoise.cpp's
`register` keyword stays a warning rather than a c++17 error.
Bug fixes: - Lifecycle.cpp: fix `seenByMaskk` typo on load; `save` writes "seenByMask" but `load` was reading "seenByMaskk", which silently dropped the field on TextStream loads (BinaryStream ignores the name, so binary saves were unaffected). - Misc.cpp: log message in `kill` labeled "unitsWorking" but printed `unitsInside.size()` (copy-paste from preceding line). - Misc.cpp: guard divide-by-zero in REPAIR construction state when `totRessources == 0`. - Misc.cpp: renumber `checkSum` index comments to close the [6] gap (was [0..5], [7..25]; now [0..24]). Documentation: Add `// PORT:` comments at four sites so the Rust port doesn't miss the non-obvious `clearingFlagStep` timer behavior: - Building/TypeSteps.cpp:406-407 — pointer to where the timer actually resets, plus the +=16 escalation channel. - Building/TypeSteps.cpp:414-415 — open question on whether `standardRandomActivity()` does proper unit detachment. - MapGradientBuilding.cpp:275-277 — flags this as the sole reset point, reachable on both return paths. - MapPathfindRessource.cpp:189-191 — flags the 125/128 threshold mismatch and the escalation pattern.
Predicate cleanups: collapse trivial if/return-true patterns; drop the
dead totalPrestige local in Prestige; factor the mutual-ally check
(Allies + OpponentsDefeated) into teamsAreMutuallyAllied; share a
maximumPrestige helper; replace the 5-way factory switch boilerplate
with a decodeAs<T> template.
API cleanups: add virtual ~WinningCondition() = default (eliminates
-Wdelete-non-abstract-non-virtual-dtor on shared_ptr<Derived> destruction
through the base) and constify hasTeamWon/hasTeamLost on the base + 5
derived classes (Game* -> const Game*, methods -> const). Mark
MapScriptSGSL::{hasTeamWon,hasTeamLost,testMainTimer} const to match.
Verification: new test/WinningConditionsHarness, a standalone driver
that exercises every WC predicate over a deterministic state lattice
(factory order, all 8 alive masks for Death, ally/winner combinations
plus a one-way alliance for Allies, 9 hand-picked Prestige cases, all
64 won/lost masks for Script, ally x lost combinations plus one-way for
OpponentsDefeated). 722 lines of golden text identical to the
pre-cleanup baseline.
On-disk format unchanged: encodeData/decodeData and section names are
untouched.
Decomposes six AI/echo translation units that exceeded the 500-line porting threshold into 23 PascalCase files. Behavior-preserving: all five replay baselines (G2 + gd-small-2ai, gd-large-4ai, gd-archipelago, gd-bigarena-long) remain byte-equal. ReachToInfinity::tick() — a 757-line method composed of independent timer-modulo branches — is decomposed into 12 private helpers (declared in Echo.h). Each helper carries its own timer guard; tick() is now a flat sequence of helper calls in the original order. Files split: Entities.cpp (589) -> 4 files (max 237) Echo.cpp (665) -> 3 files (max 327) Conditions.cpp (669) -> 4 files (max 325) Construction.cpp (991) -> 4 files (max 435) Management.cpp (1232) -> 5 files (max 443) ReachToInfinity.cpp (897) -> 3 files (max 482) Factory dispatchers (Constraint::load_constraint, ManagementOrder::load_order, Condition::load_condition with its LOAD_CASE macro, Entities::Entity::load_entity) stay in their original files. SConscript updated with 17 new sources.
….txt The /* ... */ block inside ReachToInfinity::tick() was always disabled demonstration code labeled "advanced use of Conditions" — a daisy-chain of 15 inns showcasing every advanced predicate (EitherCondition, RessourceTrackerAge/Amount, SendMessage with BuildingDestroyed, mid-build UpgradeRepair). It was never a strategy the AI should run. Moved verbatim to doc/aiEchoExamples.txt with prose context on what it demonstrates and why it was disabled. Source file shrinks by ~120 lines. All five replay baselines (G2 + gradient corpus) remain byte-equal.
The echo .cpp split inherited a 27-line shared preamble (17 includes + 8 using directives) verbatim across 23 files. Most files used only a small subset. clangd flagged the rest as unused-includes after the split landed. Trimmed each file to its actual usage. Echo.h stays in every file as the umbrella AIEcho header. Other headers (Building.h, BuildingType.h, IntBuildingType.h, Game.h, GlobalContainer.h, Order.h, Utilities.h, Brush.h) are kept only where their symbols are referenced. C++ stdlib headers (stack, queue, map, limits, algorithm, iterator, tuple) are kept only where used. using-directives are kept only for namespaces the file actually pulls from. Also: fixed one tab-only blank line in BuildingRegister.cpp. 365 lines removed across 23 files. All five replay baselines (G2 + gradient corpus) remain byte-equal.
removePlayer's manual slot-compaction was setting numberMask via `1u >> bp.number` (right-shift), which always yields 0. The canonical setter at BasePlayer::setNumber uses `1 << number`. Route through setNumber instead of duplicating the bit-twiddling, so the same typo can't reappear. Effect was benign in practice — numberMask is never consumed as a real bitmask anywhere; its only consumer is BasePlayer::checkSum, and the wrong value was consistent across all peers (server- authoritative GameHeader). But the field travels in save files and the network checksum, and a future use of numberMask as an actual mask would have silently observed 0 for any compacted slot. Also drops an unused #include "YOGServerGame.h" flagged by clangd.
getNetMessage's switch on the inbound message-type byte covered all 62 known opcodes but had no default arm. netType is read as Uint8, so values 62-255 fell through with the local shared_ptr left null, and the next line (`message->decodeData(stream)`) dereferenced it. A single byte from any peer was enough to crash the receiving thread (YOG client, YOG server, or any LAN peer). Add a default arm that logs the unknown opcode to std::cerr and returns an empty shared_ptr. Existing call sites already guard returned messages with `if(!message) return;`, so the null threads through cleanly. Side benefit: forward-compat with future protocol revisions. A client receiving an opcode it doesn't know about now drops the packet instead of crashing.
The harvest branch tested `distance < GRADIENT_FORBIDDEN_BORDER`, algebraically equivalent to `g > GRADIENT_UNREACHABLE` (255 - g < 254 iff g > 1), but the comment said GRADIENT_FORBIDDEN_BORDER means "nothing found" — wrong. The "nothing found" sentinels are 0 (obstacle) and 1 (chamfer never propagated), not 254 (forbidden-zone border). Rewrite as the explicit g > GRADIENT_UNREACHABLE filter and document each sentinel case inline. Behavior preserved. Also drops a stale `// hungry : maxfood = 100000` annotation on Unit::hungry that doesn't match the field's Uint16 storage range.
…bugs
Pre-port readability cleanup of the AIEcho Management* order classes.
Replay-verified: G2 + the four-game gradient corpus all byte-equal.
Bug fixes (latent — masked by BinaryStream's no-op enter/leaveSection):
- ManagementOrder::load_order missed the readLeaveSection paired with
its readEnterSection("ManagementOrder"); ChangeAlliances::load had
the same omission. Both would corrupt section depth on TextStream.
- AdjustPriority::save wrote the priority field as "priority" but
load read it as "p". Names are ignored in BinaryStream but the
inconsistency would break TextStream round-trip.
Refactors (behavior-preserving):
- Extracted ManagementOrder::wait_for_building static helper; the 11
building-targeted orders' wait() bodies collapse to one-liners.
- Extracted apply_area_modification helper; AddArea / RemoveArea
modify() collapse to one-liners (only differ in MODE_ADD/MODE_DEL).
- Extracted priority_to_int / int_to_priority helpers in
ManagementFlag.cpp; AdjustPriority modify/load/save no longer
repeat the same enum<->int ladder three times.
- Replaced ChangeAlliances::modify's "if (mask&t->me) mask^=t->me"
pattern with "mask &= ~t->me" for all five alliance bits. Verified
Team::me is always single-bit (teamNumberToMask = 1<<teamNumber,
only assignment to ->me is in Team.cpp:143).
- Moved ChangeAlliances/UpgradeRepair/SendMessage modify+wait from
public to protected to match the rest of the order hierarchy.
Only callers go through the base ManagementOrder pointer (Echo is
declared friend), so visibility doesn't affect dispatch.
- Removed unreachable "return true;" tail in passes_conditions.
- Tests section now points at test/README.md for the Map subclass test pattern (the trick MapQueryTest.cpp uses to unit-test Map predicates without dragging globalContainer/Bullet/Team into the test binary). - New Conventions section: PascalCase filenames, #pragma once preference, macOS case-only-rename caveat. - New "Logging is dead — do not restore" section: documents the #define fprintf if(false)fprintf macro in LogFileManager.h and the cleanup pattern when removing FILE* logFile members. - New "Pathfinding gotcha — chamfer pass cap" section: records why the 256-pass cap (Uint8 value range) is the right bound, not the Borgefors 1-pass result, since obstacles bend the propagation path.
getBrushDimXMinus/getBrushDimYMinus had dead parity branches (both arms returned w/2); collapse the four functions to single-line floor(w/2) and ceil(w/2). Drop the commented-out getBrushDimX/Y block — 17-year-old reference code from when even-width brushes were added in 2008 (commit 2958802), kept as "in case" and never restored. getBrushValue rebuilt eight stack arrays plus a pointer table per call. Hoist the patterns + index table to static constexpr (data now lives in .rodata, initialised once). Drop the commented-out 5x5 brush4/brush5 reference. Replace the magic `if (figure == 4 || figure == 5)` checkerboard-alignment branch with a needsParityAlignment[BRUSH_COUNT] flag table so the rule is data, not a hardcoded index list. Behaviour-preserving: same return values across every defined figure (0..7), so the BrushAccumulator bitmaps and the orders they feed are byte-identical. No determinism impact.
load() previously fell into the parser/version-read paths even for empty or corrupt files, then resize()'d maps to whatever uninitialized memory the istream extract returned -- silently producing a Campaign with random-count entries and an empty name. Two of three callers ignored the bool return entirely, rendering an empty menu in the failure case. Switch failure detection to backend->isValid() (semantic match for the documented FileStreamBackend(NULL) sentinel), reject versionMinor outside [MINIMUM_VERSION_MINOR, VERSION_MINOR], restore the cerr log on failure, and have CampaignMenuScreen / CampaignEditor fall back to setName(name) when load() returns false. Drop the dead Game.h / GlobalContainer.h includes in favor of a forward decl for glob2NameToFilename so the TU no longer pulls Map / Team / WinningConditions transitively. Verified by a new CampaignLoadHarness modeled on WinningConditionsHarness: synthesizes 6 fixtures in /tmp (valid, missing, empty, garbage, versionMinor=0, versionMinor=999) and prints deterministic golden output. Pre-fix tree returns ok=1 for the four broken cases (empty/garbage/version0/version+); post-fix flips them to ok=0 while preserving valid + missing rows byte-identical. TestsRunner unchanged (51 tests pass). Closes BH-012.
LIST_ELEMENT_SELECTED handler used campaign.getMap(displayedListIndex), but the displayed list contains only unlocked maps while campaign.maps[] holds locked entries too. Any non-linear unlock graph (locked map preceding an unlocked one) caused clicks to preview/describe the wrong mission. The START handler hid the same bug behind a name-based lookup (getMissionName) whose assert(false) fallback would crash on stale selection. Add Campaign::findUnlockedMap(name) and route both handlers through it. Extract repopulateAvailableMissions() to remove the constructor/post-run loop duplication. Drop getMissionName() and its assertion. Add test/CampaignSelectionHarness.cpp: in-memory non-linear-unlock fixture that asserts the old getMap(index) algorithm picks the wrong map on this fixture (regression tripwire) and the new findUnlockedMap algorithm picks the right one. Self-contained, no widget dependency, follows the CampaignLoadHarness pattern.
Replace six magic 128s with PreviewSize = 128 on the class. Rename the local int x, y, w, h returned by getScreenPos to sx/sy/sw/sh so they no longer shadow the inherited RectangularWidget::x/y/w/h members (the original code worked, but reading it required noticing that the locals shadowed the class state). Hoist parent->getSurface() to a local, drop a commented-out debug block, mark string locals const, and swap (w-128)>>1 for the equivalent integer-division form. Behavior preserving. Also: ignore the test/CampaignSelectionHarness binary added in 4ce567f.
Campaign::save was void and called writeUint32 unconditionally on a stream wrapping the result of openOutputStreamBackend, which returns a backend wrapping NULL FILE* on fopen failure. The first write hits assert(fp) in debug or fwrite(NULL) UB in release. Even when the file did open, callers could not react to failures. Mirror Campaign::load's pattern: check backend->isValid(), log to stderr, return false. Switch the stream/backend to std::unique_ptr so we don't leak on a future write throw. Update the three callers: editor's OK path shows a MessageBox and stays on screen so the user can retry; post-mission save in CampaignMenuScreen shows the same MessageBox (silent loss here would re-lock progressed missions); menu-exit save proceeds with stderr only since the player is leaving anyway. Add string-table key [ERROR_CANT_SAVE_CAMPAIGN] in texts.keys.txt and the English translation in texts.en.txt; other languages fall back to English via StringTable's defaultLang path.
…[] UB Three sites manually freed Building::globalGradient[] / localRessources[] ad-hoc, and two of them used scalar `delete` on the array-allocated localRessources buffer (Game_orders.cpp:303 in ORDER_MOVE_FLAG and MapPathfindBuilding.cpp:224 in Map::dirtyLocalGradient) — undefined behavior that fires every flag move and every footprint-tile change. Add Building::resetPathfindGradients() (drop both buffers; used after move/range change) and Building::resetLocalRessources() (drop only the local buffer; used when a footprint tile changes — global gradient is still valid). Refactor freeGradients() to call resetPathfindGradients() plus the destructor-only auxiliary-field resets. Behavior-preserving: same heap operations, same field writes. delete[] on NULL is a defined no-op so the `if (ptr)` guards collapse. dirtyLocalGradient/locked were already set unconditionally at all three sites. Verified byte-equal against tests/baselines/cpp-refactor.replay.
…leak
The three `new Type*[32]` allocations in the constructor had no matching
delete[] in the (empty) destructor — Screen::~Screen only deletes the
widgets added via addWidget, not the pointer-arrays themselves. ~768
bytes leaked per dialog open/close.
Replace with `std::array<Type*, Team::MAX_COUNT>{}` members. The {}
also closes a latent uninit: the previous init loop nulled only
playerNames/color, leaving allyTeamNumbers[getNumberOfPlayers()..32)
holding garbage.
Two symmetric bugs on the same widget: - init used setNth(descriptor.extraIslands+1) so default 0 displayed as "1"; the value shown on screen disagreed with what the generator used. - the only assignment back to descriptor.extraIslands lived in the RATIO_CHANGED handler, but extraIslands is a Number, which fires NUMBER_ELEMENT_SELECTED. Result: spinning the widget never updated the descriptor. Drop the +1 (list values 0..8 already match indices, like oldBeach), move the read into NUMBER_ELEMENT_SELECTED, and remove the bogus RATIO_CHANGED line.
…heck call sites
Adds an optional-returning accessor alongside the existing int getSelectionIndex()
and migrates every call site that used the !=-1 / >=0 sentinel-check pattern.
Eliminates the implicit signed->unsigned cast hazard at the getText boundary
and removes a class of port-time transcription risk. CampaignSelectorScreen's
getCampaignName now asserts the precondition instead of relying on the
out-of-bounds-returns-empty-string accident.
Sites that round-trip the raw int through min-clamp (LANFindScreen,
YOGClient{Options,Lobby,MapDownload}Screen save-and-restore), pass to
int-typed APIs (centerOnItem, setAction), or directly cast to enum
(NewMapScreen, separate latent-bug territory) intentionally retain
getSelectionIndex().
Verified byte-equal to tests/baselines/cpp-refactor.replay.
The two LIST_ELEMENT_SELECTED handlers cast getSelectionIndex() directly to TerrainType / MapGenerationDescriptor::Methode. In every live path the event only fires after a click has set nth >= 0, so the cast is safe today - but the unguarded sentinel-to-enum conversion is fragile and a drop-in target for an LLM port that "translates literally" against future call sites where the selection might be cleared. Switch both branches to use the new optional-returning selection() and no-op when the list reports no selection. Behavior preserved on the live path; eliminates the (TerrainType)(-1) UB in any new path that fires the event programmatically. Verified byte-equal to tests/baselines/cpp-refactor.replay.
The same-priority branch was guarded by oldPriority == priority, the opposite of when a re-register is actually needed. When a player issued ORDER_CHANGE_PRIORITY for a building already in Team::buildingsNeedingUnits, the entry stayed in the old priority bucket and unit dispatch ignored the change. Drop the predicate so any listed building falls through to the remove-and-re-add path, which now handles both the original same-priority re-sort and the missing cross-bucket move. Preserves the mid-iteration bucket churn that Team::updateAllBuildingTasks -> subscribe*Step -> updateCallLists relies on, so cpp-refactor.replay and the gradient corpus stay byte-equal (no AI emits OrderChangePriority).
Sim was pulling Toolkit::getStringTable() in two places: getUnitName
(declared in unit/UnitConsts.cpp despite having no sim callers) and
Team::getFirstPlayerName (which did a localized "[Uncontrolled]" lookup
and was called from UnitActivity to populate GameEvent::teamName for
conversion events). The latter baked the player's locale at sim time
into queued events, so a language change mid-game would leave stale
strings in pending notifications.
- Move getUnitName from src/unit/UnitConsts.cpp (deleted) to new
src/gui/UnitDisplayNames.{h,cpp}. unit/UnitConsts.h drops <string>,
Toolkit.h, and StringTable.h.
- Make Team::getFirstPlayerName return empty for uncontrolled teams
(matches its name); team/Team.cpp drops Toolkit/StringTable.
- Add displayPlayerName(const Team&) in src/gui/TeamDisplay.{h,cpp}
for UI callers wanting the localized "[Uncontrolled]" fallback.
- GameEvent now stores Uint8 otherTeamNumber instead of std::string
teamName; formatMessage(const Game&) resolves the team and calls
displayPlayerName at render time. unitLostConversion /
unitGainedConversion factories take a team number; UnitActivity
passes the teamNumber field.
- Replace four dead-code "if (textT.empty()) textT = '[Uncontrolled]'"
blocks (the empty branch was unreachable under the old contract)
with single displayPlayerName() calls. Switch three other UI sites
that called getFirstPlayerName directly to displayPlayerName so the
Uncontrolled substitution is preserved.
Verified: byte-identical replay against tests/baselines/cpp-refactor.replay
over 71948 ticks. Team::events and eventCooldownTimers are not in
Team::checkSum and events are not serialized, so this is a pure
refactor on the sim side.
Sector mixed sim state (bullets) with render-only animation lists (BulletExplosion, UnitDeathAnimation). Sim sites had to gate pushes on globalContainer->runNoX, and a Rust porter reading Sector couldn't tell sim fields from render fields. Move both lists into a new GameAnimations container owned by Game; the runNoX gate is now an internal enabled flag. Sim sites in Sector::step and UnitMedical::syncStep call onBulletImpact / onUnitDeath unconditionally. Map::load resizes the per-sector buckets since that path bypasses Map::setGame. Verified replay byte-equal against tests/baselines/cpp-refactor.replay.
…date Building::modifyForbiddenZoneForUpgradeArea consulted game->gui->getLocalTeam() to decide whether to refresh the per-client localForbiddenMap cache — the only sim->UI reach in src/building/. Map now carries the locally-displayed team id alongside its existing local*Map caches; GameGUI::adjustLocalTeam mirrors it. Also fix a pre-existing test build break (MapQueryTestStubs missing UnitDeathAnimation/GameAnimations::resize symbols) and add three tests for Map::setLocalTeam/getLocalTeam/NO_LOCAL_TEAM. Verified: TestsRunner 54/54 pass; predicate trace byte-identical across G2 + 4 gradient corpus replays (354 evaluations, 112 local-branch firings); all 5 replay baselines remain byte-equal.
…r index mayUpgrade was 184 lines of two near-identical 5-clause blocks reading 15 parallel C arrays. The duplication hid BH-220 — the level-1→2 block fed exemplar[0] (level-0 buildings) into OrderConstruction, so AINumbi's tech tree stalled at level 1. Collapse to: - UpgradeKind enum + UpgradeInventory struct + kUpgradeKindTolerance[] table - collectUpgradeInventory(Team*) replaces the 5-way else-if inventory loop - tryUpgradeRung(inv, srcLevel) replaces both 5-clause upgrade blocks - exemplar[srcLevel] read uniformly (BH-220 fix) Net -21 lines; helpers are file-local. Logically divergent for AINumbi phase-4+ games but no verification corpus uses AINumbi — cpp-refactor.replay and all four gradient baselines pass byte-equal.
Four files in the sim subfolders were pure render concerns, blurring the
sim/render boundary a Rust porter relies on when triaging directories:
- map/Minimap.{cpp,h} included GraphicContext.h, owned a DrawableSurface,
and called gfx->drawFilledRect/drawPixel/drawSurface throughout.
- unit/UnitSkin.{cpp,h} loaded the sprite atlas via Toolkit::getSprite.
- unit/UnitEditorScreen.{cpp,h} was an OverlayScreen subclass with widget
construction — a GUI screen, not unit logic.
- map/MapView.cpp held Map's tile<->screen-pixel coordinate methods,
called only from render/, gui/, map/edit/, and MarkManager.
Pure file relocations, no API churn:
- Minimap, UnitSkin → render/
- UnitEditorScreen → top-level src/ alongside other *Screen.{cpp,h}
- MapView.cpp → render/ (Map.h declarations unchanged; the methods stay
Map:: members to avoid rewriting ~30 callers).
Cross-directory #include callers updated to the render/ prefix (Minimap
in 2 files, UnitSkin in 6 files). UnitEditorScreen callers needed no
edit since #src is on CPPPATH. SConscript paths updated in both client
and server source lists.
The free-function extraction sub-step from the auditor's Option 1
(making mapCaseToDisplayable and siblings take const Map&) is deferred:
the .cpp relocation alone resolves the "Map looks like it has rendering
logic" port-noise without touching call sites.
Verified replay byte-equal against tests/baselines/cpp-refactor.replay.
The two-arg overload's parameter shared the name and type of the Building::constructionResultState member it shadowed. Behavior is unchanged; the rename removes the ambiguity at the source so the Rust port maps each in-body reference unambiguously.
Replace the hardcoded `Uint8 data[14]` with `Uint8 data[2+4*NB_UNIT_TYPE]` so the storage tracks the same formula as `getDataLength()`. Today NB_UNIT_TYPE=3 and the sizes coincide, but a future bump would have silently overflowed the buffer in getData()/setData() outside debug mode (the existing `assert(sizeof(data) == getDataLength())` is a debug-only tripwire). Wire format and checksums unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch is mostly a long-running refactor + infrastructure pass on the C++ codebase to make it portable, testable, and ready for the Rust port and AI-trainer work. The great majority are behavior-preserving cleanups validated against the deterministic replay-diff harness.
Broadly:
src/{ai,team,unit,building,map,gui,net,render,yog}/;#pragma onceeverywhere; lowercased directory names.shared_ptr,lexical_cast,tuple, threads); C++20 build; SPDX headers replacing GPL boilerplate.logFilemembers + thefprintfs feeding them (game/team/unit/building/AICastor); unused MapLog; orphaned text-loader infra; verbose pathfinding printfs.tests/baselines/cpp-refactor.replay); new four-game gradient corpus (tests/baselines/gradient/gd-*.{game,replay}) covering map-size/team-count/AI-mix variation; MapQuery characterization suite;ChecksumSidecarper-tick checksum dump for cross-replay debugging.DatasetWriterfor (state, action) records;--save-game-as,--map,--matchup,--ai-types;GLOB2_GAME_ENDsummary line;GLOB2_REPLAY_PATHenv var; absolute-path handling in FileManager / ReplayWriter.tryToBuildingSiteRoom(setBuilding(decLeft, decLeft)), unguardedget_height < distin farming AI, AIWarrush wrap typo,Game::savemutating livemapHeader, MapGenerator dead-code + 4 bugs, latent farming AI fixes. All validated against the replay baseline.Two commits on this branch deliberately change simulation behavior. Both are pre-existing-bug fixes that canonicalize previously-incorrect or implementation-defined behavior; everything else is behavior-preserving.
1.
team: stable gid tiebreak in prioritize_building(419eb4b)std::sortis unstable. When two buildings tied on every comparator field (priority, type/level bucket, unit ratio, ressource ratio), their relative order inTeam::buildingsNeedingUnitswas implementation-defined. Different binaries — different libstdc++/libc++ versions, optimization levels, ABIs (32-bit vs 64-bitsize_t) — could put tied buildings in different positions, then callsubscribeToBringRessourcesStep()in different orders, which consumessyncRand()in different orders, which desyncs lockstep multiplayer. This is a longstanding source of the multiplayer desyncs that have plagued the game for years.The fix adds
gidas a final stable tiebreaker (gidis unique within a team and identical across all clients).Consequences:
2.
map/gradient: chamfer DT replaces BFS, fixes Uint8 underflow in flag seeding (1e228ce → 487027a)The pathfinding gradient algorithm was swapped from a BFS work-queue approach (
updateGlobalGradientVersionSimple/Simon) to a chamfer distance transform (forward+backward raster sweeps until stable).The bug: war/guard/clearing flag seeding pushed every cell within
unitStayRangeintolistedAddrat value 255, but the obstacle scan that ran next could overwrite some of those cells back to 0 without removing them from listedAddr. BFS later popped a stale entry, computedgradient[addr] - 1 = 255(Uint8 underflow), and propagated phantom 255s outward from obstacle cells. Visible in-game as warriors/guards/clearers loitering one cell outside the real flag zone near obstacles. The fix compactslistedAddrafter the obstacle scan so only true sources reach the propagation pass — fully deterministic, removes a Uint8 underflow that produced different gradient values depending on which cells the obstacle scan happened to overwrite.Consequences:
Test plan
scons release=1 -j16clean build on macOS arm64 (Darwin 25.4)./build/src/glob2 --nox games/G2.game 0 1runs to completion (126,950 ticks / 84 min sim time, 4 AI players: Warrush + 2× Castor + Nicowar)G2.gameproduce byte-identicallast_game.replay(=tests/baselines/cpp-refactor.replay)gd-small-2ai,gd-large-4ai,gd-archipelago,gd-bigarena-long) byte-equal againsttests/baselines/gradient/*.replayafter the chamfer cleanup commitprioritize_buildingchange atsrc/team/team_step.cppand confirm the gid tiebreaker reasoningsrc/map/gradient/MapGradientGlobal.cppand the listedAddr underflow fix in1e228ce0