Skip to content

Reimplement 124 BuildingsState functions that are 100% - #274

Merged
gynt merged 6 commits into
sourcehold:mainfrom
6and8:buildingsstate-100
Oct 24, 2026
Merged

gynt merged 6 commits into
sourcehold:mainfrom
6and8:buildingsstate-100

Conversation

@6and8

@6and8 6and8 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

The 124 Map::Buildings::BuildingsState functions from #273 that measure 100% with reccmp --resolve-wrapped-calls (struct and function resolvers enabled locally, not committed), with their status entries. The 41 that are not 100% yet stay in #273. This branch was also built and measured on its own, without those 41: all 124 are 100% there too, except that processWallBuildingLoss tail-calls processResourceLoss from #273, so on its own only that call target differs.

  • About ten come from reimpl/BuildingsState, picked per function by compiling both versions and keeping the better match (e.g. swapOwnersOfCastle, growHops, setTileRefsForOilSmelter, updateLordLadyJesterAndGhostUnits). Some needed old field names mapped to the regenerated headers by offset.
  • Callers whose original keeps ecx/edx live across a call to a finished callee are built with /GL together with that callee (cmake/compiler-flags-gl.txt).
  • Real bugs the comparison caught in early drafts: harmAppleFarmTreesOfPlayer used tileRef1..7 instead of tileRef2..9, and setupEngineersGuildParagroundPositions offset from the entrance instead of the building position.

The first commit fixes seven sources that no longer compile against the regenerated headers on main (ActiveMenuTab union, gameSuspended/musicFadeState/lastMusicChangeTime renames, a missing MapConstants.hpp include). #273 carries the same commit.

Possible header issues (not changed here)

  • UT_S_TOWER / UT_S_BATTERINGRAM look swapped: getBuildingCost reads cost entry 82 for siege towers and 83 for battering rams, so BuildingType or MappersEnum probably has those two names swapped, and the disband price for each uses the other's building cost (commented in the source).
  • Building::numberOfAnimals is decremented when a tethered unit leaves, so it is likely an inside-unit count.
  • setBuildingToAppleFarm only writes renderBlendStrength = 32.
  • Building::tileRef1..tileRef19 are separate fields, but the original walks them as one array (up to 36 entries); several functions index from &tileRef1 (commented). An array would be cleaner.
  • padding_0x280[4] holds four per-orientation entrance flags: they are cleared with one dword store and the two has...FlagForOrientation functions read them. updateAllBuildingsSnoozedState clears dwords over two short fields (the imported version works around it).
  • getResourceSpace declares its second parameter int*, but it is a ResourceType value (the imported version casts it).
  • TileMapState::MiscDisplayLayer is ushort[] in the header, but the original treats its elements as signed short: it masks them with and word ptr [..], imm8 (66 83, sign-extended 8-bit immediate), which MSVC only emits for a signed element; ushort gives 66 81 with a 16-bit immediate. rebuildTileLogicLayerForGatesAndWalls works around it with a commented *(short*)& cast.

Cheat sheet candidates

  • Two branches storing the same constant (merged store, constant kept in a register) means if / else if with identical bodies.
  • Reads of a global that the original does before stores to this-> need hoisting into const locals, since our stores may alias the global (determinePeasantSitPosition 35% -> 97%).
  • A loop with no pre-test is a do/while (rebuildTileLogicLayerForKeeps).
  • and word ptr, imm8 (66 83) on an array element means the element is signed short.
  • A block repeated over consecutive fields with its own base pointer is a loop over an array the header splits into fields (harmAppleFarmTreesOfPlayer).
  • A hoisted local for an index read twice can block MSVC's jump threading and recover the original's block layout (updatePathLinkageTileMap).

Built with the bundled MSVC1400-SP1 under Wine on Linux.

🤖 Generated with Claude Code

loamifi-jake and others added 2 commits October 21, 2026 04:31
Field renames and the ActiveMenuTab union from the latest Ghidra sync broke
seven sources; add a missing MapConstants include in two PathFindingState files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
124 BuildingsState functions that match the original at 100% (with
--resolve-wrapped-calls), their status entries and the /GL pairs they
need. The rest stay open in sourcehold#273.

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

6and8 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@gynt this is ready for review whenever you have time.

@gynt

gynt commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Awesome. I just checked out the branch and indeed everything is 100%. I will go through each file now to see if the code style matches with what we currently do.

@gynt gynt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just small things a handful of while loops to convert to for loops! Great work!!

@6and8

6and8 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Thank you! I'm churning out some more, will PR shortly.

@gynt

gynt commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

You can also pull in half-baked stuff from any branches you can find on https://github.com/gynt/OpenSHC

For this PR, you can cherry-pick the header changes: a10953c and e808f25

loamifi-jake and others added 2 commits October 22, 2026 13:50
wheatGrowStateRelated holds the growth state of both wheat and hops farms.
Every SwapInts caller passes int fields, so the undefined4 casts go.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also clang-format include grouping in two PathFindingState files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
loamifi-jake and others added 2 commits October 22, 2026 20:31
# Conflicts:
#	src/OpenSHC/Map/Buildings/Building.hpp
tileRefs[36] array, BOOLEnum returns with TRUE/FALSE, field162_0x280 entrance/exit
flags, getResourceSpace(int, ResourceType), upgradeBuildingsForMapVersion(int, ...),
renamed field201/202, and casts dropped where MiscDisplayLayer, mapTimeInTicks and
pathLinkageKeepWasUpdatedUnk are now short/int. All functions unchanged in reccmp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@gynt
gynt merged commit 1b8d538 into sourcehold:main Oct 24, 2026
1 check passed
6and8 pushed a commit to 6and8/OpenSHC that referenced this pull request Oct 24, 2026
tileRefs[] array indexing, farmGrowState (now short, casts dropped),
field162_0x280 entrance flags, field192/201 renames, BOOLEnum returns with
TRUE/FALSE, and getResourceSpace(int, ResourceType) - the same follow-ups
sourcehold#274 got in 975e091, for the 41 functions of this PR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6and8 pushed a commit to 6and8/OpenSHC that referenced this pull request Oct 24, 2026
The Ghidra header sync retyped it after sourcehold#274 was last updated, so main no
longer compiles this file. Return type and value follow the header; reccmp
unchanged (100%).

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

gynt commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

@6and8 feel free to open up more PRs!

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.

3 participants