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:25:15Z

+    // FUNCTION: STRONGHOLDCRUSADER 0x004CF270
+    void AICState::aiCommandTribe13DefensiveWave(int playerID)
+    {
+        int tribeIndex = 13;


To be documented in the future

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

+        int aiType = DAT_GameState::instance.playerDataArray[playerID].aiType;
+        if (aiType == AITA_NULL)
+            return 0;
+        return (&this->aics[aiType - 1].farm1)[DAT_GameState::instance.playerDataArray[playerID].aiFarmIndex];


The farms may be an array rather than farm1, farm2, etc. However, because the setAICParameters are not 100% (and that took long) I am sceptical I should change them to an array. I could make a union so we can treat it as an array or as fields, but I don't care that much at the moment, there are more important things. This code is pretty clear to me.

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:17:47Z

+        int aicIndex = DAT_GameState::instance.playerDataArray[playerID].aiType - 1;
+        if (DAT_GameState::instance.mapAndTime.playerTeams[playerID]
+            == DAT_GameState::instance.mapAndTime.playerTeams[DAT_GameSynchronyState::instance.currentPlayerSlotID]) {
+            bik = 23;


The BIKs should likely also receive an enum. Athough, I assume you want to postpone this?

Yea I really divide my time between reimplementing and doing research in Ghidra to develop enums, and currently I am not researching. It is also easy to solve in the future, we change the field type and compiler will complain in all included cpp files in which we still have a stray constant.

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.

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

TheRedDaemon requested changes Oct 17, 2026

View reviewed changes

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

Conversation

gynt commented Oct 14, 2026

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!

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!

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!

Choose a reason for hiding this comment