Skip to content

Reimplement 88 Map::TileMapState functions at 100% - #366

Open
gynt wants to merge 18 commits into
mainfrom
reimpl/TileMapState-100
Open

gynt wants to merge 18 commits into
mainfrom
reimpl/TileMapState-100

Conversation

@gynt

@gynt gynt commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Reimplements 88 Map::TileMapState functions that match the original at 100%, taken from reimpl/TileMapState. The 17 TileMapState files already on main are unchanged.

  • Every function here measured 100% (raw, or normalized where only call targets differ) on reimpl/TileMapState, and again in a trial build of this branch with the data and function resolvers enabled.
  • The status lines of all 88 are set to 100.0% | Reimplemented.

Header changes

Each is in its own commit and limited to what these functions use:

  • Moat: tile is int (main's own UpgradeMapLogicToVersion_Unknown3 already reads it as *(uint*)&). x/y are short and fillProgress is char; both are tested signed in the original.
  • Rock::x/y: short, read with movsx.
  • Unit::field_0x83: a byte field rather than padding; triggerLoweredView clears it.
  • TileMapState:
    • signed int counters and flags: maxPitchDitchCount, temporaryTerrainTypeIndex, SEC_Section1052..1054, the forceUpdate* flags and buildingMin/MaxHeight
    • MacroLayer is short, read with movsx everywhere
    • mapping40x40 is char
    • field_0x5548d0 is a field rather than padding
  • validateWallPlacementAtTile takes its mapper command as short. The original compares it as a word, and that is what took the function from 81% to 100%.

These are generated headers, so the same changes need to go into Ghidra too.

LTCG

16 of these functions are added to cmake/compiler-flags-gl.txt, together with their already-finished callees, getBuildingSizeIndexMappingData among them.

  • The original keeps this in ecx across those calls. Only /GL with the callee compiled and enabled reproduces that; without it these functions measure 40-65%.
  • Only entries that measured better are listed.

Behaviour fixes

  • spawnEraserTileEffect no longer clears L_BUILDING; the original's mask is ~0x410b00.
  • placeDefensiveStructureTile gets its missing M_MAPPER_STAIR5 arm.
  • setupAllMapSections calls the empty 0x469F10 with UnitsState/LandscapeState in ecx, as the original does, through a __fastcall cast of DoNothing (as processGameTick does).

Note

updateBuildingPlacementRotationPreview, which is already on main, measures 46% in the trial build. It needs its /GL callee determineBuildingPlacementRotation, which is not finished yet and so is not part of this PR. This PR does not change that function.

Functions

Constructor_TileMapState, advanceMoatDigProgress, applyRockGraphicsToFootprint, changeMapOrientation, clearBuildingDisplayFlagsAndEntities, clearBuildingFootprintAndRemoveSiegeTower, clearBuildingFootprintAndResetUnits, clearBuildingFootprintWithEdgeRubble, clearBuildingFromTerrain, clearBuildingTilesAndTrees, clearDrawBridgeWater, clearFixedSizeTwoBuildingFootprint, clearInvalidMoatEntries, clearMoat, clearMoatDataAtTile, collectCliffEdgeTilesForClimbData, computeClimbRampRotation, computeTileAlongAxisOffset, computeWallCornerRenderRotation, countLogicPropertyInSurroundingTiles, countMoatsOwnedByEachPlayer, countUnfinishedMoatTilesForPlayer, countUnownedPitchDitches, createMoatData, destroyPitchDitch, destroyWallsOfPlayer, findNearestValidDigTileNearTarget, findTileInSameAreaAndNoTooHeightDifference, getBuildingHurtSFXID, getBuildingSizeForCommandBuildingType, getBuildingSizeIndexMappingData, getCastleBuildRangeForMapSize, getMaxWallHeightInBrushArea, getNonFarmFieldBuildingHealthAtTileOr1000, getOrientationThatIsWallTowerOrGatehouse, getPitchDitchIDForTile, getRubbleGraphicStageForDamageLevel, getTileForBrush, getTotalHeightAtTile, getWallFlagForOrientedDirection, hasHigherNeighborWithStairs, hasHigherPlainNeighborWithWallOrGatehouse, hasOnlyTowerNeighborsNoWalls, increaseHeightForTunnelSingleTile, increaseHeightForTunnelWithBrush, isCliffDropInDirection, isFootprintIndexOnEdge, isTileEnclosedByWalls, isTileEnclosedByWallsOrGates, isWallConnectionHeightValid, isWallCornerForCardinalDirection, isWallCornerForDiagonalDirection, markBuildingFootprintFlag, noop1, placeDefensiveStructureTile, planMoat, rebuildShowHiLayerFromHeights, removeMoat, resetAreaBasedOnLogicalLayer, resetHeightAndMapBorders, resetTileAndClearMoat, resetTileToDefaultState, returnMoatIDForPlayerIDs, returnOwnedMoatAtTile, rotateFearFactorBuildingVariations, setChangedLayerToThreeAndMapping0x40x40, setMapSize, setSignpostDistanceForCampaignMission, setupAllMapSections, setupBuildingSizeIndexMapping, setupLogicalMapBorders, setupTileMapSections, spawnEraserTileEffect, storeMinAndMaxHeightOfArea, swapMoatOwnership, swapPitchOwnership, swapWallOwnership, triggerLoweredView, unmarkBuildingFootprintFlag, updateAreaBasedOnSurrounding, updateGameRelatedValue, updateMacroLayer, updatePathLinkagesForBuilding, updatePitchDitches, updateShowHiLayerOrResetChangedLayer, upgradeMapFormatLogicLayer, validateBuildingPlacementAtTile, validateWallPlacementAtTile

🤖 Generated with Claude Code

https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC

gynt and others added 6 commits November 7, 2026 14:07
The original reads tile as a dword (main's own UpgradeMapLogicToVersion_Unknown3
already does *(uint*)&moats[i].tile), x and y with movsx, and tests
fillProgress signed (test al, al; jg / cmp dl, 4; jl).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
applyRockGraphicsToFootprint reads both with movsx.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
triggerLoweredView clears it next to heightDiv10.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
maxPitchDitchCount, temporaryTerrainTypeIndex, SEC_Section1052..1054, the
forceUpdate flags and buildingMin/MaxHeight are compared signed in the
original; MacroLayer is read with movsx everywhere; mapping40x40 holds
char; the dword at 0x5548D0 is a field (setupAllMapSections clears it),
not padding.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
The original compares it as a word; with the short type its
reimplementation reaches 100% (from 81%).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
From reimpl/TileMapState: every function that measures 100% there (raw or
normalized, i.e. only call targets differ), and again 100% in a trial
build of this branch with the data and function resolvers enabled.

- 88 sources in src/OpenSHC/Map/TileMapState/ (the 17 already on main are
  unchanged), with their status lines set to 100.0% Reimplemented
- cmake/compiler-flags-gl.txt: the 16 of them whose original keeps this
  in ecx across an already finished callee (getBuildingSizeIndexMappingData,
  setSignpostDistanceForCampaignMission, ...); /GL plus the enabled callee
  is what reproduces that, without it they measure 40-65%

Behaviour fixes carried over: spawnEraserTileEffect no longer clears
L_BUILDING (the original mask is ~0x410b00), setupAllMapSections calls the
empty 0x469F10 with UnitsState/LandscapeState in ecx as the original does,
placeDefensiveStructureTile has its missing M_MAPPER_STAIR5 arm.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
@github-actions github-actions Bot added the declaration/type change This requires changes to the generated/exported types or definitions. label Nov 7, 2026
@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

Style guide reading (advisory, posted once)

30 possible deviation(s) from REIMPLEMENTATION_STYLE_GUIDE.md across 21 of the 88 reimplemented .cpp file(s) in this PR.

The match comes first. Where the ugly form is what matched, it stays -- that is the guide's own overriding rule, and rule 21 asks for a one-line comment saying what was measured so the next reader does not retry it. This report is posted once, when the PR opens, and is not repeated on later pushes.

rule what it asks hits
1 Drop the decompiler trailing return; 1
8 Let the compiler unroll 4
9 Prefer for over while/do 13
18 Use the name, not the number 12
Where

Rule 1 — Drop the decompiler trailing return;

  • src/OpenSHC/Map/TileMapState/getBuildingSizeIndexMappingData.cpp:31 — trailing return; in a void function

Rule 8 — Let the compiler unroll

  • src/OpenSHC/Map/TileMapState/rebuildShowHiLayerFromHeights.cpp:11 — loop steps by 8; try the plain loop and let MSVC unroll
  • src/OpenSHC/Map/TileMapState/setupAllMapSections.cpp:61 — loop steps by 6; try the plain loop and let MSVC unroll
  • src/OpenSHC/Map/TileMapState/setupLogicalMapBorders.cpp:14 — loop steps by 2; try the plain loop and let MSVC unroll
  • src/OpenSHC/Map/TileMapState/swapWallOwnership.cpp:11 — loop steps by 6; try the plain loop and let MSVC unroll

Rule 9 — Prefer for over while/do

  • src/OpenSHC/Map/TileMapState/applyRockGraphicsToFootprint.cpp:28 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/clearBuildingDisplayFlagsAndEntities.cpp:19 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintAndRemoveSiegeTower.cpp:28 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintAndResetUnits.cpp:25 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintWithEdgeRubble.cpp:24 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/clearDrawBridgeWater.cpp:14 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/clearFixedSizeTwoBuildingFootprint.cpp:21 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/markBuildingFootprintFlag.cpp:14 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/resetHeightAndMapBorders.cpp:29 — do-loop; the guide asks for a for
  • src/OpenSHC/Map/TileMapState/storeMinAndMaxHeightOfArea.cpp:21 — do-loop; the guide asks for a for
  • ... and 3 more

Rule 18 — Use the name, not the number

  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintAndResetUnits.cpp:34 — hex constant 0xFFDF; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintAndResetUnits.cpp:44 — hex constant 0x2000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintAndResetUnits.cpp:46 — hex constant 0x4000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintWithEdgeRubble.cpp:33 — hex constant 0xFFDF; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintWithEdgeRubble.cpp:43 — hex constant 0x2000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/clearBuildingFootprintWithEdgeRubble.cpp:45 — hex constant 0x4000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/clearFixedSizeTwoBuildingFootprint.cpp:34 — hex constant 0x4000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/destroyPitchDitch.cpp:21 — hex constant 0x2000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/destroyPitchDitch.cpp:23 — hex constant 0x4000; use a name, or decimal
  • src/OpenSHC/Map/TileMapState/markBuildingFootprintFlag.cpp:21 — hex constant 0xFFF3; use a name, or decimal
  • ... and 2 more

gynt and others added 3 commits November 7, 2026 17:55
… sources

Measured against a reccmp baseline of this branch before and after, in two
rounds: all 88 functions come out byte-identical in match, average 99.92,
every one still 100% raw or normalized. Nothing had to be reverted.

- rule 18: 70 hex literals to decimal, bit masks left in hex; 399/400 to
  MC_MAP_XY_LIMIT_INCLUSIVE/MC_MAP_XY_LIMIT, 80400 to MC_MAP_TILES_COUNT and
  398 to MC_MAP_XY_LIMIT - 2, matching what the sibling files already did
- rule 16: param_N named in 14 files. Four of those parameters are unused in
  the body, and since the function matches at 100% the original does not use
  them either, so they are called unused rather than given a guessed meaning
- rule 4: the three byte-sized layer fills in setupAllMapSections take
  sizeof of the layer. PillarGFXLayer's is a short fill, so its 80400 is an
  element count and stays MC_MAP_TILES_COUNT
- rule 1: the trailing return; of getBuildingSizeIndexMappingData and noop1
- rule 23: the ternary in computeWallCornerRenderRotation is another else-if
- rule 9: resetHeightAndMapBorders bounds its loop on a plain local, so its
  do-while became a for
- rule 21: the twelve remaining do-while loops carry a note instead. Their
  bound is this->constructionTileCount, which the getBuildingSizeIndexMappingData
  call inside the body writes, so a leading test would read the previous
  footprint's count. Those loops are required for correctness, not just for
  the match, and converting them would have been a behaviour bug
- block comments to //, which is what main's cpps use; duplicate using blocks
  dropped in three files; clear-masks spelled as the complement of the bits
  they clear, as the same lines already spell the bits they set; clang-format

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
… break

The twelve footprint loops test this->constructionTileCount, which the
getBuildingSizeIndexMappingData call in the body sets, so the test has to
stay at the end of the body. Writing that as while (true) with a trailing
break instead of do-while is codegen-neutral: all 88 functions measure
identical to the previous commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
Takes main's upstream Ghidra header sync, which already carries this
branch's TileMapState type fixes (and makes
forceUpdateLogicalAndMiscDisplayLayers int on top), so the headers are
main's. The /GL lists of both sides are kept. All 88 functions measure
identical to before the merge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
@github-actions github-actions Bot removed the declaration/type change This requires changes to the generated/exported types or definitions. label Nov 7, 2026
gynt and others added 4 commits November 8, 2026 01:07
The six hand-unrolled copies were MSVC's own unrolling of a one-tile loop
body. Written as a plain loop it unrolls six-fold, as in the original, once
the decompiler's (char)playerID - 1U cast is dropped from the writes; with
the cast the body looks bigger to MSVC and it unrolls five-fold (0.905).
Still 100%.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
Addresses the style-lint and cpp-linter reports on the PR:
- setupAllMapSections: the border graphic loop is a plain per-tile loop;
  MSVC unrolls it six-fold by itself.
- rebuildShowHiLayerFromHeights: a plain loop unrolls only six-fold where
  the original has eight, so the outer loop steps by 8 over an inner 0..7
  loop instead of eight hand-written copies.
- updatePathLinkagesForBuilding: trailing whitespace (clang-format).

All 88 functions measure identical to before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
Drop the comments that explained deviations, test BOOLEnum results
without != FALSE, remove casts to the destination's own type, name the
climb-ramp heights, write the setcc idioms in rotateFearFactorBuildingVariations
as if/else, make importHeight a BOOLEnum and let loops replace the
copies in countMoatsOwnedByEachPlayer and getTileForBrush.

All 88 functions measure unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmXCmaNCgNNvTDAKE2qbZC
if ((this->LogicLayer[tile] & L_WALL_OR_GATEHOUSE) == 0) {
if (this->disableElevationRendering == 0) {
this->GfxLayer[tile]
= ((short)GMFirstPictureIndex::instance[DAT_LandscapeState::instance.rocks[rockID].unknownGMID]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary casts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in a50bdcf, together with the other casts in the branch whose removal leaves the per-function asm and the warnings unchanged (50 in total). The ones that stay change codegen (e.g. the (uint) before >>) or are needed to compile.

OpenSHC::Map::Buildings::BuildingsState_Func::refreshAllBuildingTileDisplays, DAT_BuildingsState::ptr)();
DAT_BuildingsState::instance.field_0x10 = 5;
switch (this->mapOrientation) {
case 0:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use MapOrientation enum values for the map orientation (field type will change).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 51479bd, after merging main (6f522f7) where the field became MapOrientationInt: the switch cases and all mapOrientation comparisons in the branch now use MO_NORTH/MO_EAST/MO_SOUTH/MO_WEST. reccmp unchanged.

}
}

if (DAT_BuildingsState::instance.buildings[buildingID].buildingType == OpenSHC::Map::Buildings::BT_STOCKPILE) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't a switch-case be better here? Test if a switch case compiles the same

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested: as a switch on buildingType (same case order) the function drops from 100% to 61.0% (normalized 57.5%). MSVC builds a jump table, while the original is a compare chain, so the if/else-if chain stays.

`a = a op v` becomes `a op= v` (103 sites). Using-declarations that
TileMapState.hpp already provides (BOOLEnum, MappersEnum, BuildingType),
that other includes already make visible, or that were unused are removed.
Codegen-neutral: every function's /FA output is identical.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
void TileMapState::setupTileMapSections()
{
MACRO_CALL_MEMBER(OpenSHC::IO::LowLevelMemory_Func::fillMemory_ByteValue, DAT_LowLevelMemory::ptr)(
sizeof(this->GfxLayer), '\0', this->GfxLayer);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 instead of '\0'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6ad2a6c: all 30 fill bytes passed to fillMemory_ByteValue are now 0 (29 here, 1 in setupAllMapSections.cpp). Codegen unchanged.

gynt and others added 4 commits November 11, 2026 13:03
…bounds

Removes 50 casts whose removal leaves every function's /FA output and the
compiler warnings unchanged; the four (int)(x & 1) casts in
placeDefensiveStructureTile stay, since dropping all of them together splits
a shared tail. Literal and MC_MAP_TILES_COUNT bounds that are the length of
the array being indexed become countof(), and MapConstants.hpp is no longer
included where nothing else used it. Codegen-neutral.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fillMemory_ByteValue takes a uchar, so the char literal only obscured a
plain zero. Codegen-neutral.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With main's headers, mapOrientation is a MapOrientationInt and the
forceUpdate* flags are BOOLEnum: compare against MO_NORTH/EAST/SOUTH/WEST
and assign TRUE/FALSE. PathConnectionLayer is now short, which makes the
(short) read casts redundant. reccmp unchanged (87/88 at 100%).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant