AIV troop-spot loader skips 3 unit types (pikemen / swordsmen / Arabian swordsmen) — placed troops never walk to their AIV positions

[DDanielDragon]

Hi — sharing a finding from reverse-engineering the AIV troop-spot behaviour, in case it's useful for the reimplementation. All base-Stronghold Crusader.exe addresses below are against the SHA256 your SARIF targets (3BB0A8C1…AC5A); I verified the same bug exists in Crusader Extreme and shipped a UCP3 fix for it.

Symptom

Start troops placed at AIV unit positions (section 2012) never walk to their spots for three unit types, while ranged units / knights / assassins do. Most visibly the Arabic lords' swordsmen just idle at the keep. Firefly fixed this for the Definitive Edition; the original engine never was.

Mechanism

The loop that decodes AIV section 2012 into the per-AI spot arrays contains an explicit skip of three rows. In base SHC at 0x4EF4B0:

0x4EF4B0  cmp dword [esp+0x1C], 9      ; row index
0x4EF4B5  mov eax, [esp+0x38]
0x4EF4B9  mov dword [eax], 0           ; zero this row's count
0x4EF4BF  je  0x4EF7F7                 ; -> loop increment, row not loaded
0x4EF4C5  cmp dword [esp+0x1C], 0xB
0x4EF4CA  je  0x4EF7F7
0x4EF4D0  cmp dword [esp+0x1C], 0x12
0x4EF4D5  je  0x4EF7F7
0x4EF4DB  ...                          ; normal position decoding

The row index is the AIVUnitType (your enum). Skipped: 9 = Pikeman, 11 (0xB) = Swordsman, 18 (0x12) = Arabian Swordsman — three melee infantry types. Their positions are discarded, so the downstream assignment (AICState::sendWallPatrolUnitTribesToAIVLocations in your naming) finds no spot for them and they never get a move order.

Why it looks like a bug, not intent

• Stock AIVs place real troops in exactly these rows (e.g. every Arabic lord uses row 18; Wolf uses row 9; Phillip uses row 11).
• These three types otherwise go through the same "walk to spot" path as knights (row 12) and assassins (row 15), which do walk — only their spot data is missing because of the skip.
• Removing the three je (NOP) makes them march to their positions, matching DE behaviour. Verified live.

Crusader Extreme

Same code, shifted addresses (Extreme has extra code; base and Extreme addresses do not coincide). In Stronghold_Crusader_Extreme.exe the skip block is at 0x4EF840. I can provide the Extreme↔base mapping for the surrounding AI functions if that helps your cross-version work — happy to contribute.

Thanks for OpenSHC; the named symbols (sendWallPatrolUnitTribesToAIVLocations, getDefensiveTribeForUnit, aiGiveOuterPatrolCommand, the AIVUnitType enum) made confirming all of this much faster.

[gynt]

Why is it not intent? I mean I find it hard to cause their intent as well, but it could have bene intent. Do you have more information I am not aware of?

[TheRedDaemon]

I can not speak about intent here, since someone clearly created the original code, even by mistake, but at the current stage, the projects aim is recreating the game, including all "mistakes".
My personal vote would also always be on "improvements/bugfixes/changes of the original game will receive a fork based repo one day".
Now, the question is, what to do with such reports.

I personally do not think this is the right place for them, since it will be a huge while till something like this can be considered. As long as it is documented in the repo of the UCP module the same way, I also do not think this will be lost, since UCP modules are likely candidates of things people would want to integrate once the reimplementation is done. @gynt, what do you think?

[gynt]

In the specific case, we can remember this issue for the fixme comments in the cpp files we create. This works in this case because raising the question (why is this code here?) is also answering it (it shouldn't be here under these and those assumptions).

In the general case however, it may not be that simple to convert such an issue into a code comment.

I see several options:

1. Invite contributions like these ( I love these contributions ! ) and keep them here and open. This creates engagement. However, open issues that aren't being worked for the time being look desolated and clutter the issues list.
2. Transfer these issues to a repo that does aim to fix issues. This repo has to be in the same organization for GitHub's transfer to work, otherwise the poster has to reopen the issue there manually.
3. Add a fixme or a comment to the functions that are affected by the report (in the source code and/or in Ghidra) and close the issue. This seriously acknowledges the contribution (presuming we don't delete all fixme and code comments from Ghidra at some point before fixes are applied).
4. Open a GitHub discussion page to store these fixme situations and close the issue itself. We then build a database of source code quirks and issues.
5. Transfer the issue to the OpenSHC website, a dedicated page with detected code quirks. This approach acknowledges that things we consider quirks now might make sense later (or never).

Personally I am in favor of 4 or 5. 4 looks much more engaging on GitHub, because we can discuss quirks that are posted.

[TheRedDaemon]

I think 4 is good.
My mind also kinda likes 2, but it has the issue that we would just create an empty repo for them, or already start a fork no one even should work on for a long while.

Also checked if there is an API for the discussions (for the case that we really need to extract and move them) and there seems to be a GraphQL one LINK.

There should be hints in the README and on the website. Using the discussion feature on Github is more niche then just opening an issue, so we need to point people there, something like

You found something that could be a bug in the original game or you have ideas for features?
We are currently still in the stage of reimplementing the original game, but if you want to document your findings or maybe even engange in discussions about it, please put them into <Reference to Github Discussions>.

I do not know the capabilities of Github discussions, though.

Sorry @DDanielDragon for using you issue for this btw., but we have to figure out what to do with findings like yours.

[gynt]

I converted this issue to a discussion in the category "Code issues". Thanks again @DDanielDragon for raising this issue in a very clear way.