Reimpl/tile map state

AGENTS.md

@@ -141,6 +141,20 @@ Diff patterns that were reliable (more in the cheat sheet):
   not a source difference.
 - A mismatching argument count or `ret N` usually means the generated header is wrong; report it instead of working around it.
 - Diffs can reveal real bugs in existing reimplementations (wrong constants, wrong strides); fix those.
+- A decompiled `goto` that lands *after* a shared block (a clamp ladder, a common tail) is usually not a
+  jump at all: put the block once, after the if/else that feeds it, and give the skipped arm the value the
+  block maps to itself (`step = 0` for a clamp that leaves 0 alone). MSVC then emits the original's
+  "jump straight past the block" for that arm on its own. Duplicating the block into each arm instead does
+  *not* get tail-merged and scores far worse - worth ~38 points on `updateShowHiLayerOrResetChangedLayer`.
+- Thresholds in such a ladder are the literals the `cmp` shows, not what the decompiler printed:
+  `cmp eax, 0x3c; jle` is `step > 60`, and Ghidra renders the same code as `iVar1 < 60`.
+- A folded address constant where the original has an `imul` means a named local: we wrote
+  `(y - 1) * 400 + x` and got one constant, while the original multiplied a `y - 1` local by 400.
+  Give each reused coordinate its own local and reassign it as the original walks the neighbours.
+- An extra 4 bytes of frame with no local to explain it is usually a value the decompiler folded into an
+  expression but the original held in a variable (a copy taken across a call, a mask computed once).
+- A mask applied inside a loop over a value that cannot change was hoisted in the original; look for
+  `and reg, MASK` before the first call and keep the masked result in a local.
 
 ## Agent Skills
 

IMPLEMENTATION_CHEAT_SHEET.md

@@ -50,6 +50,34 @@ This also applies when putting instructions inside the condition branches.
 
 Naturally, the logic flow needs to be kept, so the update of a variable can not move before or after another usage of the same variable.
 
+### Statement Order
+
+The order of instructions in the assembly is not the order of the statements in the original source.
+Within a boundary the compiler schedules freely, so do not copy the instruction order into the source.
+
+This matters most for long runs of independent stores into one object, as in `AICState::setAICParameters_NN`:
+the whole body is `this->aics[aicIndex].<field> = <constant>;` over ~130 fields, and the compiler emits
+those `mov dword ptr [eax + <offset>], <reg>` instructions in an order of its own. Rewriting all 16 of
+those functions so their statements followed the original's store order made every one of them *worse*
+(e.g. `setAICParameters_14` 82.4% -> 50.0%).
+
+That does not make the order unrecoverable. The compiler moves only the stores whose value sits in a **register**:
+when a constant's register is about to be reloaded with another constant, the stores still waiting for the old value
+are pulled up in front of the reload, and stores of a newly loaded constant are gathered behind it. Stores of an
+**immediate** (`mov dword ptr [eax + off], 0x46`) stay where the source put them. So:
+
+1. Take the immediate stores of the original as the skeleton of the source order. Across a family of sibling
+   functions the skeleton is the same template, so vote the precedence of every pair of fields over all siblings -
+   a field that is a register store in one function is an immediate in another.
+2. Put each register store back at its place in that template (mostly struct order, with the template's own local
+   swaps: `unknown002` before `unknown001`, `populationPerFarm` before `unknown011`, `recruitProbDefWeak` before
+   `recruitProbDefDefault`, `RecruitIntervalWeak` before `RecruitInterval`, `AttMaxAssassins` before `AttUnit2`).
+3. Compile and move the few statements that still sit elsewhere next to their neighbour in the original.
+
+All 16 `setAICParameters_NN` went from 75-95% to 100% this way, with no type or header change; the earlier attempt
+failed because it copied the *instruction* order, hoisted stores included. Which constant lands in which register
+then follows by itself - it was a consequence of the statement order, not of register pressure.
+
 ## Structure
 
 ### Bitwise Operations
@@ -99,6 +127,56 @@ SEC_RNG::ptr->currentNumber1 % 4
 - The assembly may contain multiple assigns of the same value to the same variable within the same boundary. This might be optimization suggesting there was only one assign.
 - The decompiler may suggest multiple assigns of the same value to the same variable within the same boundary. If this is not present in the assembly, it might be an artifact.
 - If a variable is stored in a temporary, before it is directly being incremented or decremented by one, suggests a post-increment/decrement, even more if the temporary is used after this.
+- Do not hoist a repeated sub-expression into a local just because it appears several times. `int const halfWidth = barWidth / 2;`
+  used at four call sites lost against the original, which recomputes `barWidth / 2` at every use (SHC_3BB0A8C1_0x004B20B0, 67.6% -> 82.4%).
+  Write the expression out again at each use and let the compiler decide.
+- **A local that only copies a field lets the compiler fold what the original kept separate.** MSVC forward-substitutes
+  an expression built from a register value (`int aiType = pd.aiType; ... int aicIndex = aiType - 1;`) into its use, so
+  `aics[aicIndex].x` becomes `[reg*0x2a4 + this + (off - 0x2a4)]` with no decrement left. It does **not** substitute
+  an expression that contains a memory read across a store, a call or a branch. The original's
+  `test eax, eax; je; add eax, -1; imul eax, eax, 0x2a4; ... [eax + ecx + off]` therefore needs the field tested
+  directly and read again for the index:
+  ```cpp
+  if (DAT_GameState::instance.playerDataArray[playerID].aiType == AIT_NULL)
+      return FALSE;
+  int aicIndex = DAT_GameState::instance.playerDataArray[playerID].aiType - 1;
+  ```
+  `determineAIPlayerHelp`, `determineAIPlayerAttackRequestResponse`, `addUnitToSmallestPatrolTribe`,
+  `setCurrentAttackStrength` all went to 100% on this alone. Fifteen other spellings of the index (casts, pointer
+  arithmetic, an inlined accessor, in-place decrement, unsigned) all folded - only the missing local matters. The
+  same applies to any other decompiler local for a field (`tracker`, `outerPatrolGroupsCount`): reading the field at
+  every use took `getTargetableBuildingForPlayerID` from 65% to 100%.
+- **A constant index held in a local stops the per-struct multiply being shared.** When the original recomputes
+  `playerID * 0x39f4` in every basic block (`mov eax, ecx; imul eax, eax, 0x39f4` three times, with `playerID` staying
+  in its register) while we multiply once, the array index in the source was a *variable* that the optimizer only
+  later found to be constant. With a variable index MSVC factors each access by its own element size
+  (`playerID * 0x1cfa` for a `short` array, `* 0xe7d` for an `int` array), so the three accesses are three different
+  expressions and nothing is common. Write `int tribeIndex = 11;` and index with it:
+  ```cpp
+  int tribeIndex = 11;
+  if (DAT_GameState::instance.playerDataArray[playerID].aiType == AITA_NULL)
+      return;
+  int tribeID = DAT_GameState::instance.playerDataArray[playerID].aiTribeIDs[tribeIndex];
+  ```
+  Eight `AI::AICState` tribe-command functions went from 38-62% to 100% (`setTribe0xbToAggressiveAndAttack`,
+  `sendTribeToAttack`, `aiCommandTribe11/13`, ...). The same happens when the index is a field the code has just
+  compared against a constant: `pd.aivCurrentPauseIndex == 1 && pd.aivPauses[pd.aivCurrentPauseIndex] > 0`, not
+  `aivPauses[1]`. It is not universal - on functions whose index is a loop sum (`aiTribeIDs[166 + i]`) the local made
+  things worse.
+- A constant kept in a callee-saved register from the top of the function (`mov ebx, 1` before the first branch, later
+  `mov [field], ebx`) is a **result variable** initialised at the top and stored once at the end, not two literal
+  stores: `int isNotNervous = 1; ... if (cond) isNotNervous = 0; else {...} field = isNotNervous;`
+  (`computeNervousness` 77% -> 100%).
+- `add reg, -N` where we emit `sub reg, N` or `cmp reg, N` means two constants were merged: the source compared a
+  derived value, `aicIndex == 7` with `aicIndex = aiType - 1`, not `aiType == 8`
+  (`removeOrganismsAndSetMoveDestinationPairs` 97.9% -> 100%).
+- Where two arguments are loaded on either side of a store (`mov ecx, [x]` ... `mov [stance], 1` ... `mov eax, [y]`),
+  give each a local and put the statements in that order: `int x = pd.someX; tribe.unitStance = ...; int y = pd.someY;`
+  (`makeUnitsGoDefensiveAndBackToSomeLocation` 95.7% -> 100%). Without a store in between, the *declaration order*
+  of two such locals picks which one gets `ecx` (`sendUnitsToAttackBreachedCastle` 98.7% -> 100%).
+- The opposite does happen for the operands of a single statement: in `SHC_3BB0A8C1_0x00522520` the original evaluates
+  `unitID / 16` and `unitID % 16` into locals before the compound assignment. If a `|=`/`&=` statement with computed index
+  and mask does not match, give the index and the mask a local each and keep the declaration order the assembly shows.
 
 ### Conditionals
 
@@ -110,6 +188,14 @@ SEC_RNG::ptr->currentNumber1 % 4
 - If a condition suddenly uses a normally signed variable as unsigned value (e.g. `ja` compared to signed `jg`) and most times also contains a subtraction, it suggests a range condition optimization. The actual value was moved to 0 to allow expressing this via a single check:
   - Decomp code example: `DAT_GameCore.missionNumber1to20 - 1U < 0x14`
   - Original (likely): `1 <= DAT_GameCore.missionNumber1to20 && DAT_GameCore.missionNumber1to20 <= 20`
+- A two-sided range check that is the whole body of a `BOOL`-returning function is compiled branchless. Write it as one
+  expression, `return lo <= value && value <= hi;`, not as nested `if`s with early returns, and use the literals the
+  assembly shows rather than a `short`/`ushort` local holding the field (SHC_3BB0A8C1_0x00530FD0, 78.3% -> 100%).
+- Which form a boolean result takes is **not** predictable and has to be tried both ways. The folded
+  `return (x & MASK) == VALUE;` matched at only 40% for SHC_3BB0A8C1_0x004549C0; spelling it out as
+  `if ((x & MASK) == VALUE) { return TRUE; } else { return FALSE; }`, with locals for the computed index and the loaded
+  byte, reached 100%. Together with the range-check case above this means: folded expression and explicit
+  `TRUE`/`FALSE` branches are two separate candidates, measure both.
 - A switch with fewer then 4 cases is often simplified and uses subtractions in assembly to compare the value:
   ```
   MOV          EAX,[DAT_SoundEffectsHelperData1.SEC_Section1079.  volumeLevel]
@@ -196,6 +282,48 @@ you might have found an unrolled loop. Therefore, try to reproduce the logic in
 
 If GOTOs are present that clearly jump to the start of a loop, but the logic does not allow to do this without a GOTO, for example from a loop inside a loop, you might be able to move the continue or break condition to the outside. Methods could be placing a fitting condition related to the contained loop conditions after the loop or using a boolean flag that then functions as conditional. Both can sometimes be optimized away.
 
+A global used as a running counter alongside the loop index is usually incremented inside the loop, even when its final
+value is a constant. Ghidra shows the folded result: `DAT_CurrentUnitSlotID = 2500;` placed around the loop is really
+`DAT_CurrentUnitSlotID = 1;` before it and `DAT_CurrentUnitSlotID += 1;` as the first statement of the body. This was
+worth the last 8-10% on the whole `Map::Version::UpgradeMapUnitsTo_*` family (0x0053B310, 0x0053B530, 0x0053B570,
+0x0053B5E0 all 90% -> 100%).
+
+`!=` as the loop bound suppresses MSVC's unrolling. If the original ends the loop with `jne` while we emit `jl` and an
+unrolled body, write `for (int i = 1; i != 2500; ++i)` instead of `i < 2500` (SHC_3BB0A8C1_0x0053B340, 53% -> 91%).
+
+- A loop whose early exit returns the same value as the code after the loop was a `break`, not a `return`. The tell is
+  callee-saved registers pushed *after* the loop's entry test and one shared `mov eax, result` behind the loop, where
+  an early `return result;` makes us push everything in the prologue (`assignRequiredIdleEngineersToNewTribe`,
+  `addEngineersToSelection`, both ~88% -> 100%).
+- Do not add a guard the loop condition already expresses. `if (id == 0) return 0;` in front of
+  `for (; id != 0; id = next(id))` moved one `push` across a branch; without it the compiler derives the same early
+  exit itself and places it where the original does (`aiRequiresExtraOxtethers`). Likewise put the block the
+  original keeps inline first: `if (best > 20) { store; return 1; } return 0;` rather than the inverted guard.
+
+- **Several exits that end in the same statement were usually one statement.** MSVC copies a short shared tail
+  (a store or a call followed by the epilogue) into each predecessor, so three `mov word ptr [tribe.unitStance], 2`
+  + `ret` blocks in the original do not mean three assignments in the source. Writing the assignment once after an
+  `if / else if` ladder instead of `stance = X; return;` in every arm changes how often each value is *used*, and the
+  use count is what ranks values for the callee-saved registers - so this is the fix when a function matches
+  instruction for instruction except that two registers are swapped (`tribeID` in `ebx` and the tribe offset in `ebp`
+  on one side, the reverse on the other). `giveMoveCommandToSortieUnits` 79.6% -> 100%, `instructTribe166ToMove`
+  81.8% -> 100%, `assignUnitToATribe` 86.1% -> 100% (one `addUnitToTribe(unitID, tribeID)` after the branches
+  instead of three calls). When one of the arms must skip a call the others share, a `bool` set in the arms and
+  tested once afterwards compiles away and gives the single call site the original has.
+- A search loop whose "found a free slot" exit must skip the check that follows the loop has its end test **inside
+  the body**: `for (;; i++) { if (i >= count) { if (best == 0) return 0; break; } ... if (free) { best = create();
+  bestIndex = i; break; } ... }`. A `for (i < count)` loop with the check after it tests `best` on both paths, and
+  duplicating the tail into the create path changes the frame. With the test in the body the compiler rotates it to
+  the bottom and lays the blocks out as the original does (`getSmallestPatrolTribe` 65.2% -> 100%). The same
+  function needed `int i = 0;` declared *first*, before the other zero-initialised locals, to get the original's
+  `xor` order - declaration order did not matter anywhere else it was tried.
+
+- `mov edx, esi; add esi, 1` at the **top** of a loop body, with the body then indexing by the copy, is a `while`
+  loop that takes the index and advances the counter first: `while (i < count) { int index = i; i++; if (...[index]...)
+  { ...; return; } }`. A `for (...; i++)` with the same body does not produce it. Together with `int i = 0;`
+  declared where the original's `xor esi, esi` sits (here before an earlier early-return test, which then compares
+  against that zero register) this took `generateSiegeCreationInformation` from 39.4% to 100%.
+
 ### GOTO
 
 A function may contain multiple GOTOs.
@@ -230,6 +358,19 @@ It is hard to find these cases. I one situation, local variables that were used
 
 ## Functions
 
+### Return Values
+
+A function declared `void` whose assembly still leaves a value in EAX at the return usually returns it. The most common
+case is a function that returns the ID it was passed: changing `void EntityState::activateProjectileEntity(int entityID)`
+to `int` with a trailing `return entityID;` took SHC_3BB0A8C1_0x004039B0 from 36.4% to 100%. Fix the signature in the
+generated header rather than working around the difference.
+
+### Link-Time Code Generation
+
+A volatile register (usually ECX or EDX) that stays live across a call in the original, without being reloaded, means
+that call was compiled with LTCG. This is fixable, not a permanent blocker: add the `.cpp` to `cmake/compiler-flags-gl.txt`
+so it is built with `/GL`. SHC_3BB0A8C1_0x00504EE0 went from 38.5% to 100% and SHC_3BB0A8C1_0x0045B7F0 to 100% that way.
+
 ### Parameters
 
 Parameters might not be pushed like normal in certain cases.

build.bat

@@ -23,16 +23,21 @@ if "%~1"=="" (
 set "PRESET=%~1"
 set "TARGET=%~2"
 
-:: --- Kill mspdbsrv.exe if it's running ---
-tasklist | find /I "mspdbsrv.exe" >nul
-if not errorlevel 1 (
-    if not defined WRAP_QUIET echo Stopping mspdbsrv.exe...
-    taskkill /f /t /im mspdbsrv.exe >nul
-    if errorlevel 1 (
-        if not defined WRAP_QUIET echo [WARNING] Failed to stop mspdbsrv.exe, continuing...
-    )
+:: --- Give this worktree its own PDB server ---
+:: mspdbsrv.exe is a per-user singleton keyed on _MSPDBSRV_ENDPOINT_, and every worktree
+:: carries its own copy of the toolchain. Sharing one endpoint across worktrees makes
+:: cl.exe talk to whichever copy started first and fail with
+::   fatal error C1090: PDB API call failed, error code '23'
+if not defined _MSPDBSRV_ENDPOINT_ (
+    for %%I in ("%~dp0.") do set "_MSPDBSRV_ENDPOINT_=openshc_%%~nxI"
 )
 
+:: --- Kill this worktree's mspdbsrv.exe if it's running ---
+:: Only ours: killing every instance by image name takes down the PDB server of any other
+:: worktree that is building at the same time, which fails their build with C1090.
+powershell -NoProfile -Command ^
+    "Get-Process mspdbsrv -ErrorAction SilentlyContinue | Where-Object { $_.Path -like '%~dp0*' } | Stop-Process -Force -ErrorAction SilentlyContinue" >nul 2>&1
+
 :: --- Run cmake configure using preset ---
 if not defined WRAP_QUIET echo [INFO] Configuring with preset "%PRESET%"...
 call .\cmakew --preset "%PRESET%"

cmake/compiler-flags-gl.txt

@@ -7,3 +7,23 @@ src/OpenSHC/IO/ResourceManager/swapMapDataWithNextMap.cpp
 src/OpenSHC/UI/Rendering/WindowAndDirectDraw/takeScreenshot.cpp
 src/OpenSHC/IO/ResourceManager/readFirstPartOfCurrentResourceIntoMemory.cpp
 src/OpenSHC/IO/ResourceManager/readNextPartOfCurrentResourceIntoMemory.cpp
+src/OpenSHC/Map/TileMapState/getBuildingSizeIndexMappingData.cpp
+src/OpenSHC/Map/TileMapState/recountTotalOwnedMoats.cpp
+src/OpenSHC/Map/TileMapState/setSignpostDistanceForCampaignMission.cpp
+src/OpenSHC/Map/TileMapState/determineBuildingPlacementRotation.cpp
+src/OpenSHC/Map/TileMapState/updateBuildingPlacementRotationPreview.cpp
+src/OpenSHC/Map/TileMapState/updateMoatCountdownTimers.cpp
+src/OpenSHC/Map/TileMapState/updateGameRelatedValue.cpp
+src/OpenSHC/Map/TileMapState/clearDrawBridgeWater.cpp
+src/OpenSHC/Map/TileMapState/clearFixedSizeTwoBuildingFootprint.cpp
+src/OpenSHC/Map/TileMapState/clearRockFootprintFlags.cpp
+src/OpenSHC/Map/TileMapState/clearSizeFiveBuildingFootprintAndMoats.cpp
+src/OpenSHC/Map/TileMapState/clearTreeFootprintFlags.cpp
+src/OpenSHC/Map/TileMapState/floodMoatUnderRemovedBuilding.cpp
+src/OpenSHC/Map/TileMapState/isUnitBlockingSizeFiveFootprint.cpp
+src/OpenSHC/Map/TileMapState/markBuildingFootprintFlag.cpp
+src/OpenSHC/Map/TileMapState/placeOilsmelter.cpp
+src/OpenSHC/Map/TileMapState/renderWallPlacementPreview.cpp
+src/OpenSHC/Map/TileMapState/setMiscDisplayLayer.cpp
+src/OpenSHC/Map/TileMapState/storeMinAndMaxHeightOfArea.cpp
+src/OpenSHC/Map/TileMapState/unmarkBuildingFootprintFlag.cpp

reccmp/dll/requirements.txt

@@ -1 +1 @@
-git+https://github.com/isledecomp/reccmp
+git+https://github.com/sourcehold/reccmp