reimplement: AICState methods that are 100% by gynt · Pull Request #261 · sourcehold/OpenSHC

gynt · 2026-10-14T07:37:27Z

No description provided.

gynt · 2026-10-17T12:27:58Z

+namespace AI {
+
+    // FUNCTION: STRONGHOLDCRUSADER 0x004CB3A0
+    undefined4 AICState::aiRequiresExtraOxtethers(int playerID)


Should return BOOLEnum in the future.

gynt · 2026-10-17T12:28:59Z

+    {
+        int tribeID
+            = MACRO_CALL_MEMBER(Map::Units::TribesState_Func::createTribeForPlayer, DAT_TribesState::ptr)(playerID);
+        for (int unitID = 1; unitID < (int)DAT_UnitsState::instance.maxUnitCount; unitID++) {


The (int) cast is old code. maxUnitCount is already int in the headers. Remove cast.

gynt · 2026-10-17T12:31:45Z

+                continue;
+            if (DAT_UnitsState::instance.units[unitID].unknownSiegeTentRelated02 != 3)
+                continue;
+            if (DAT_UnitsState::instance.units[unitID].aiUnitBehaviourType == 21)


Magic value 21 is to be documented in the future

gynt · 2026-10-17T12:32:17Z

+namespace AI {
+
+    // FUNCTION: STRONGHOLDCRUSADER 0x004CB940
+    undefined4 AICState::destroyBuildingIfNoWorker(int buildingID)


BOOLEnum in the future

TheRedDaemon

First batch. Will continue review later.

TheRedDaemon · 2026-10-17T14:36:41Z

+
+        int popularity = DAT_GameState::instance.playerDataArray[playerID].popularity;
+        if (popularity <= this->aics[aicIndex].criticalPopularity)
+            DAT_GameState::instance.playerDataArray[playerID].aiPopularityDecisionValue = 2;


Could it make sense to create enums for this, if this is state? Or are they "numerical" in meaning?

YEs should become enums in the future.

TheRedDaemon · 2026-10-17T14:37:07Z

+        if (DAT_GameState::instance.playerDataArray[playerID].aiPopularityDecisionValue == 2) {
+            DAT_GameState::instance.playerDataArray[playerID].taxesSetting = 2;
+            if (DAT_GameState::instance.playerDataArray[playerID].totalFood <= 0)
+                DAT_GameState::instance.playerDataArray[playerID].rationsSetting = 0;


Same thing with the ration settings: Enum or numerical?
The question can be extended to the taxes setting also, I think.

TheRedDaemon · 2026-10-17T14:49:54Z

+            quarryID = MACRO_CALL_MEMBER(OpenSHC::Map::Buildings::BuildingsState_Func::findNextBuildingForPlayerAndType,
+                DAT_BuildingsState::ptr)(playerID, OpenSHC::Map::Buildings::BT_QUARRY, quarryID)) {
+            int stone = DAT_BuildingsState::instance
+                            .buildings[(short)DAT_BuildingsState::instance.buildings[quarryID].quarryStockpileID]


Is this cast still needed?

TheRedDaemon · 2026-10-17T14:50:28Z

+                continue;
+            if (DAT_UnitsState::instance.units[unitID].owner != playerID)
+                continue;
+            if (DAT_UnitsState::instance.units[unitID].dying != 0)


Is this a boolean check?

If so, we should use the if() and if(!) styles without the != 0

TheRedDaemon · 2026-10-17T14:51:32Z

+            if (tribeID == 0
+                || DAT_TribesState::instance.tribes[tribeID].uid
+                    != DAT_GameState::instance.playerDataArray[playerID].aiTribeUIDs[baseOffset + offset]) {
+                if (checkOnly != FALSE) {


Boolean check?

If so, we should use the if() and if(!) styles without the != 0

TheRedDaemon · 2026-10-17T16:28:09Z

@@ -0,0 +1,41 @@
+#include "OpenSHC/Map/Units/UnitsState.func.hpp"
+#include "OpenSHC/Rendering/Bink/AIMessageQueue.func.hpp"
+#include "OpenSHC/AI/AICState.hpp"


Wrong Import.

TheRedDaemon · 2026-10-17T16:28:23Z

@@ -0,0 +1,36 @@
+#include "OpenSHC/Map/Units/UnitsState.func.hpp"
+#include "OpenSHC/Rendering/Bink/AIMessageQueue.func.hpp"
+#include "OpenSHC/AI/AICState.hpp"


Wrong Import

TheRedDaemon · 2026-10-17T16:28:33Z

@@ -0,0 +1,23 @@
+#include "OpenSHC/Map/Units/UnitsState.func.hpp"
+#include "OpenSHC/Rendering/Bink/AIMessageQueue.func.hpp"
+#include "OpenSHC/AI/AICState.hpp"


Wrong Import.

TheRedDaemon · 2026-10-17T16:28:51Z

@@ -0,0 +1,23 @@
+#include "OpenSHC/Map/Units/UnitsState.func.hpp"
+#include "OpenSHC/Rendering/Bink/AIMessageQueue.func.hpp"
+#include "OpenSHC/AI/AICState.hpp"


Wrong Import.
I stop at this point. You need to check all files again if they use the proper relative func.hpp Import.

TheRedDaemon · 2026-10-17T16:30:44Z

+    // FUNCTION: STRONGHOLDCRUSADER 0x004D0BB0
+    void AICState::playGoodsSentBikFromPlayerToPlayer(int playerID, int targetPlayerID)
+    {
+        if (MACRO_CALL_MEMBER(Map::Units::UnitsState_Func::getAliveLordForPlayer, DAT_UnitsState::ptr)(playerID) == 0) {


Can be transformed to truthiness checks.

If so, we should use the if() and if(!) styles without the == 0

TheRedDaemon

Went through all.

TheRedDaemon · 2026-10-17T17:24:27Z

+        }
+
+        DWORD now = timeGetTime();
+        if (DWORD_00eb0b18::instance != 0 && now - DWORD_00eb0b18::instance < 1200000) {


Is this used somewhere else? If not, then this should be the timestamp of the last time the thanks bik was played... although, does 20 min really seem right?

Will become TIME_LastThanksBikPlaybackTime

TheRedDaemon · 2026-10-17T17:39:21Z

+    using OpenSHC::WindowsHelper::Enums::BOOLEnum;
+
+    // FUNCTION: STRONGHOLDCRUSADER 0x004D30E0
+    undefined4 AICState::sendUnitsToAttackBreachedCastle(int attackingPlayerIndex)


Is this also a BOOLEnum return?

Nice spot, applied it in Ghidra.

TheRedDaemon · 2026-10-17T18:21:14Z

+    // FIXME: The original function never sets a return value (eax is left over from the last computation),
+    // so it was most likely a void function. The header declares int, which forces the "return 0"s below
+    // and costs an extra "xor eax, eax" in the assembly. Change the return type to void in the header.


Is this still valid?

Comment should be removed.

TheRedDaemon · 2026-10-17T18:22:17Z

+                    DAT_GameSynchronyState::ptr)(targetPlayer, 0);
+
+                // "Gold"
+                MACRO_CALL(OS_Func::_sprintf)(DAT_GameSynchronyState::instance.receivedChatMessage, "%d %s - ",


Some non-replaced string?

Literal strings should be replaced with the appropriate replacement in the src/OpenSHC/string-literals.hpp

TheRedDaemon · 2026-10-17T18:27:15Z

+            }
+
+            candidateCount = 0;
+            // Players 1..8, manually unrolled by 4 in the original.


So just having this:

if ((DAT_GameSynchronyState::instance.currentAIArray[playerID] != 0) && (DAT_GameState::instance.mapAndTime.playerTeams[playerID] != DAT_GameState::instance.mapAndTime .playerTeams[DAT_GameSynchronyState::instance.currentPlayerSlotID])) { candidates[candidateCount] = playerID; candidateCount++; }

in a loop from 1 to 8 does not unroll?

I should try

TheRedDaemon · 2026-10-17T18:34:43Z

+    {
+        for (int i = 0; i < 20; i++) {
+            MACRO_CALL_MEMBER(IO::LowLevelMemory_Func::fillMemory_ByteValue, DAT_LowLevelMemory::ptr)(
+                676, '\0', &this->aics[i]);


It this sizeof the aic type?

Yes! Should be replaced with a sizeof()

TheRedDaemon · 2026-10-17T18:35:51Z

@@ -34157,73 +34157,73 @@ SHC_3BB0A8C1_0x004C6CD0 | 0.0% | Pending



Does this still contains the percentages of the ones not part of the PR?

Good point, no change should be made to the txt file lines that don't be long to any of the functions in this PR.

…ents Two generated-header changes requested in the PR review, kept separate from the .cpp work as AGENTS.md asks: - AICState::sendUnitsToAttackBreachedCastle returns BOOLEnum instead of undefined4; the body returns TRUE/FALSE accordingly. - The 0x00EB0B18 global is named TIME_LastThanksBikPlaybackTime instead of DWORD_00eb0b18, which is what playThanksBikFromPlayerToPlayer stores into. quick_diff holds at its baseline for both functions (1.0000 and 0.9762). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Include hygiene (the reviewer's "wrong import", checked across all 137 files): every AICState .cpp now opens with the relative "../AICState.func.hpp" instead of the absolute "OpenSHC/AI/AICState.hpp", matching the files that already did. 63 files changed; the func header is a superset, so no other include moved. Magic values and style: - assignRequiredIdleEngineersToNewTribe: drop the stale (int) cast on maxUnitCount (already int in the header), and test `dying` for truthiness. - checksAndGenerateAITribesForPlayerIfNotExisting: `if (checkOnly)` instead of `if (checkOnly != FALSE)`. - playGoodsSentBikFromPlayerToPlayer: `if (!getAliveLordForPlayer(...))`. - getUnitTypeIndexForUnitID: the Caliph check now reads `aiType - 1 != AITA_CALIPH`, and the loop bound 0x14 is written as 20. - wipeAICMemory: sizeof(this->aics[i]) instead of the literal 676. - shareGoldAmongTeamMembers: drop the obsolete FIXME about the int return (the header already declares void), and use s__d__s___005a7674 from string-literals.hpp for the "%d %s - " format. Measured, not assumed: - aiRequiresExtraOxtethers: the (short) on quarryStockpileID is load-bearing (removing it drops quick_diff 1.0000 -> 0.9901), so it stays, with a comment. - someChatMessageSelection: rolling the unrolled player scan into a plain 1..8 loop drops quick_diff 0.9198 -> 0.6745, so the unroll is in the original source. Kept, with the measurement recorded in the comment. - computeNervousness: isNotNervousByEnemyTroopValue is an int field storing 0/1, which per AGENTS.md is an int and not a bool, so it is left as is. status/addresses: reverted the 56 changed lines that belong to functions outside this PR; the remaining 121 all map to a `// FUNCTION:` address in src/OpenSHC/AI/AICState. No regressions: quick_diff is unchanged from baseline on every touched function. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ested loop The 4 hand-written copies of the candidate test are replaced by an inner `for (offset = 0; offset < 4; offset++)` inside the existing step-by-4 outer loop. The generated instruction stream is identical to the unrolled source -- 132 instructions, same opcodes and operands, differing only in MSVC's internal label numbering -- so quick_diff stays at 0.9198 (118 vs the original's 119). This corrects the reasoning in the previous commit. The compiler does unroll here, contrary to the "MSVC1400 at /O2 never unrolls a loop" note in AGENTS.md: writing all 8 iterations out by hand compiles to byte-identical code to a plain 1..8 loop (136 instructions, 0.6745 either way). What the original's shape actually pins down is the unroll *factor* of 4, not the absence of a loop in its source, so the outer loop must step by 4 while the inner 4 can be left to the compiler. Also measured and rejected: a file-local inlined helper called 4 times in the outer loop (identical code, but emits a dead out-of-line copy of the helper), the same helper called 8 times (0.4000), unroll by 2 (0.7339), and a half/offset index split (0.8220). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ction Tried `for (int i = 1; i < 9; i++)` and three other spellings of the same loop (reusing the function's declared `i`, a fresh name declared in the `for`, and the pre-declared `playerID`). All four produce byte-identical assembly at 0.6745 and 154 instructions, against 132 for the nested form and 119 in the original, so the loop variable's name, declaration site and reuse are all irrelevant here. The reason is visible in the diff: fully unrolling 8 iterations with a known trip count lets MSVC constant-fold every index, so each body gets its own absolute addresses and an immediate player number. The original instead keeps the index in a register and computes the stored value with `lea`, which means its source had an induction variable that survived the unroll. Stepping the outer loop by 4 is what reproduces that. Comment updated with the mechanism; no code change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Verified against the original binary with Ghidra rather than inferred. The field at PlayerData + 0x3820 has 7 writes and 5 reads in the whole image, and every write stores 0 or 1: 004d3802 computeNervousness xor eax, eax -> 0 004d383f computeNervousness ebx -> 1 004ad1c5 ProcessAllyAcceptsRequest imm -> 0 004ad295 ProcessAllyDeniesRequest2 imm -> 0 004ad315 ProcessAllyRequestAttackDefense eax after xor -> 0 004ad38e ProcessAllyRequestAttackDefense imm -> 1 004ad433 ProcessAllyRequestAttackDefense imm -> 0 All five reads are == 0 / != 0 truthiness tests, so the field is boolean. BOOLEnum is already the convention for such fields in PlayerData.hpp. This also corrects the note in the earlier commit, which argued the field had to stay an int because the original stores it with `mov dword ptr [..], 0/1`. That AGENTS.md rule distinguishes int from C++ `bool` (a one-byte store); BOOLEnum is a typedef of BOOL, hence int, so the dword store is exactly what a BOOLEnum field looks like and was never evidence against it. Being the same underlying type, the change is purely semantic: quick_diff stays at 1.0000 for computeNervousness, determineAIPlayerHelp and determineAIPlayerAttackRequestResponse, the field's only users. The two readers also lose their `!= 0` per the review's preferred if() style. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

165 locals across the AICState sources gain `const` after the type, per the project's preferred `int const stackvariable` spelling. Applied only where the local is initialised once at its declaration and never afterwards assigned, incremented, decremented or address-taken; arrays, multi-declarators and uninitialised declarations are left alone. Wrong calls would not compile, and all 137 files were compiled. This also restores the `const` on getAIBuildInterval's `aiType`, which was not dropped deliberately: it was lost in a8f7dae, where the AICState sources were re-added from the field-refactor script's output. It was the only one of the four files predating that commit to lose it. const is NOT codegen-neutral here, so it is applied by measurement rather than by rule. Seven locals of the form `int tribeIndex = <literal>` had to keep their non-const spelling: with const, MSVC1400 folds the literal into the array index at its use site and drops 4 instructions that the original still has, costing 0.18 to 0.50 of match in aiCommandTribe11StandGroundToGate, aiCommandTribe13DefensiveWave, sendAITribe_0xf_toAttackPatrol, sendTribeToAttack, setTribe0x11ToAggressiveAndAttackIfConditionMet, setTribe0xCtoAggressiveAndAttack and setTribe0xbToAggressiveAndAttack. const on a local initialised from a memory read or a call is neutral, which is every other case here. Verified by a full before/after quick_diff sweep of all 137 functions: every one is identical to its baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ExtraOxtethers Both were declared `undefined4` in the generated headers, which is simply stale: Ghidra already declares each as returning BOOLEnum, with TRUE/FALSE in the decompiled bodies. Verified in the open program rather than inferred, so the next header import should agree. destroyBuildingIfNoWorker has exactly one caller in the whole image, UpdateWoodcuttersHut at 00410d73, which consumes the result with `test eax, eax; jnz` - a pure truthiness test. Header type change kept separate from the .cpp work, as AGENTS.md asks. The `return 0/1` in both bodies became `return FALSE/TRUE` in the preceding commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Restore status/addresses-SHC-3BB0A8C1.txt from origin/main and re-apply only the entries for addresses carrying a FUNCTION marker in the .cpp files added or changed on this branch (120 entries). Drops the unrelated 0x004CC000 percentage change for buyGoods.cpp, which this branch does not touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

gynt changed the base branch from main to reimpl/AICState-leftover October 14, 2026 07:49

gynt changed the base branch from reimpl/AICState-leftover to main October 14, 2026 07:50

reduce file list to AICState methods and status file

a8f7dae

gynt force-pushed the reimpl/AICState-100 branch from 959b9a9 to a8f7dae Compare October 14, 2026 08:15

style improvements

6b919a4

gynt commented Oct 17, 2026

View reviewed changes

Comment thread src/OpenSHC/AI/AICState/aiCommandTribe13DefensiveWave.cpp

gynt commented Oct 17, 2026

View reviewed changes

Comment thread src/OpenSHC/AI/AICState/aiFarmChoice.cpp

gynt commented Oct 17, 2026

View reviewed changes

TheRedDaemon requested changes Oct 17, 2026

View reviewed changes

gynt and others added 9 commits October 18, 2026 00:11

Merge branch 'main' into reimpl/AICState-100

7fa4c5b

clang-formatting

d816050

gynt requested a review from TheRedDaemon October 18, 2026 14:48

		@@ -34157,73 +34157,73 @@ SHC_3BB0A8C1_0x004C6CD0 \| 0.0% \| Pending

Conversation

gynt commented Oct 14, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRedDaemon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRedDaemon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers