Merge pull request OpenEnroth#1339 from captainurist/macos_fixes_399

Fixes for gold issues

src/Engine/Graphics/Indoor.cpp

@@ -2024,29 +2024,9 @@ void SpawnRandomTreasure(MapInfo *mapInfo, SpawnPoint *a2) {
             return;
         }
 
-        if (a2->uItemIndex == ITEM_TREASURE_LEVEL_1) {
-            a1a.containing_item.uItemID = ITEM_GOLD_SMALL;
-            v34 = grng->random(51) + 50;
-        } else if (a2->uItemIndex == ITEM_TREASURE_LEVEL_2) {
-            a1a.containing_item.uItemID = ITEM_GOLD_SMALL;
-            v34 = grng->random(101) + 100;
-        } else if (a2->uItemIndex == ITEM_TREASURE_LEVEL_3) {
-            a1a.containing_item.uItemID = ITEM_GOLD_MEDIUM;
-            v34 = grng->random(301) + 200;
-        } else if (a2->uItemIndex == ITEM_TREASURE_LEVEL_4) {
-            a1a.containing_item.uItemID = ITEM_GOLD_MEDIUM;
-            v34 = grng->random(501) + 500;
-        } else if (a2->uItemIndex == ITEM_TREASURE_LEVEL_5) {
-            a1a.containing_item.uItemID = ITEM_GOLD_LARGE;
-            v34 = grng->random(1001) + 1000;
-        } else if (a2->uItemIndex == ITEM_TREASURE_LEVEL_6) {
-            a1a.containing_item.uItemID = ITEM_GOLD_LARGE;
-            v34 = grng->random(3001) + 2000;
-        }
+        a1a.containing_item.generateGold(a2->uItemIndex);
         a1a.uType = pItemTable->pItems[a1a.containing_item.uItemID].uSpriteID;
-        a1a.containing_item.SetIdentified();
         a1a.uObjectDescID = pObjectList->ObjectIDByItemID(a1a.uType);
-        a1a.containing_item.special_enchantment = (ItemEnchantment)v34;
     } else {
         if (!a1a.containing_item.GenerateArtifact())
             return;

src/Engine/Objects/Actor.cpp

@@ -201,8 +201,7 @@ void Actor::SetRandomGoldIfTheresNoItem() {
             v2 = grng->randomDice(this->monsterInfo.uTreasureDiceRolls, this->monsterInfo.uTreasureDiceSides);
             if (v2) {
                 this->items[3].uItemID = ITEM_GOLD_SMALL;
-                this->items[3].special_enchantment =
-                    (ItemEnchantment)v2;  // actual gold amount
+                this->items[3].goldAmount = v2;
             }
         }
     }

src/Engine/Objects/Character.cpp

@@ -108,14 +108,16 @@ static constexpr IndexedArray<int, CHARACTER_SKILL_MASTERY_FIRST, CHARACTER_SKIL
            // helps avoid -1 indexing, originally 4 element array off by one
 static constexpr std::array<int, 5> StealingRandomBonuses = { -200, -100, 0, 100, 200 };  // dword_4EDEB4
 
-static constexpr IndexedArray<int, CHARACTER_SKILL_MASTERY_FIRST, CHARACTER_SKILL_MASTERY_LAST> StealingEnchantmentBonusForSkill = {
-    // {CHARACTER_SKILL_MASTERY_NONE, 0},
+/**
+ * The amount of gold that a character can steal in one go is determined as `[skill_level]d[mastery_die]`, where
+ * `skill_level` is the level of stealing skill, and `mastery_die` is picked from the table below.
+ */
+static constexpr IndexedArray<int, CHARACTER_SKILL_MASTERY_FIRST, CHARACTER_SKILL_MASTERY_LAST> goldStealingDieSidesByMastery = {
     {CHARACTER_SKILL_MASTERY_NOVICE, 2},
     {CHARACTER_SKILL_MASTERY_EXPERT, 4},
     {CHARACTER_SKILL_MASTERY_MASTER, 6},
     {CHARACTER_SKILL_MASTERY_GRANDMASTER, 10}
-};  // dword_4EDEC4      //the zeroth element isn't accessed, it just
-          // helps avoid -1 indexing, originally 4 element array off by one
+};
 
 static constexpr IndexedArray<ItemSlot, ITEM_TYPE_FIRST, ITEM_TYPE_LAST> pEquipTypeToBodyAnchor = {  // 4E8398
     {ITEM_TYPE_SINGLE_HANDED,  ITEM_SLOT_MAIN_HAND},
@@ -1537,21 +1539,21 @@ StealResult Character::StealFromActor(unsigned int uActorID, int _steal_perm, in
                 return STEAL_NOTHING;
             }
 
-            unsigned int enchBonusSum = grng->randomDice(stealingSkill.level(), StealingEnchantmentBonusForSkill[stealingSkill.mastery()]);
+            int stolenGold = grng->randomDice(stealingSkill.level(), goldStealingDieSidesByMastery[stealingSkill.mastery()]);
 
-            int *enchTypePtr = (int*)&actroPtr->items[3].special_enchantment;  // actor has this amount of gold
+            int *goldPtr = &actroPtr->items[3].goldAmount;  // actor has this amount of gold
 
-            if ((int)enchBonusSum >= *enchTypePtr) {  // steal all the gold
-                enchBonusSum = *enchTypePtr;
+            if (stolenGold >= *goldPtr) {  // steal all the gold
+                stolenGold = *goldPtr;
                 actroPtr->items[3].uItemID = ITEM_NULL;
-                *enchTypePtr = 0;
+                *goldPtr = 0;
             } else {
-                *enchTypePtr -= enchBonusSum;  // steal some of the gold
+                *goldPtr -= stolenGold;  // steal some of the gold
             }
 
-            if (enchBonusSum) {
-                pParty->partyFindsGold(enchBonusSum, GOLD_RECEIVE_NOSHARE_SILENT);
-                engine->_statusBar->setEvent(LSTR_FMT_S_STOLE_D_GOLD, this->name, enchBonusSum);
+            if (stolenGold) {
+                pParty->partyFindsGold(stolenGold, GOLD_RECEIVE_NOSHARE_SILENT);
+                engine->_statusBar->setEvent(LSTR_FMT_S_STOLE_D_GOLD, this->name, stolenGold);
             } else {
                 engine->_statusBar->setEvent(LSTR_FMT_S_FAILED_TO_STEAL, this->name);
             }

src/Engine/Objects/Chest.cpp

@@ -560,40 +560,7 @@ void GenerateItemsInChest() {
                         if (whatToGenerateProb < 20) {
                             currItem->Reset();
                         } else if (whatToGenerateProb < 60) {  // generate gold
-                            int goldAmount = 0;
-                            currItem->Reset();
-                            // TODO(captainurist): merge with the other implementation?
-                            switch (resultTreasureLevel) {
-                            case ITEM_TREASURE_LEVEL_1:
-                                goldAmount = grng->random(51) + 50;
-                                currItem->uItemID = ITEM_GOLD_SMALL;
-                                break;
-                            case ITEM_TREASURE_LEVEL_2:
-                                goldAmount = grng->random(101) + 100;
-                                currItem->uItemID = ITEM_GOLD_SMALL;
-                                break;
-                            case ITEM_TREASURE_LEVEL_3:
-                                goldAmount = grng->random(301) + 200;
-                                currItem->uItemID = ITEM_GOLD_MEDIUM;
-                                break;
-                            case ITEM_TREASURE_LEVEL_4:
-                                goldAmount = grng->random(501) + 500;
-                                currItem->uItemID = ITEM_GOLD_MEDIUM;
-                                break;
-                            case ITEM_TREASURE_LEVEL_5:
-                                goldAmount = grng->random(1001) + 1000;
-                                currItem->uItemID = ITEM_GOLD_LARGE;
-                                break;
-                            case ITEM_TREASURE_LEVEL_6:
-                                goldAmount = grng->random(3001) + 2000;
-                                currItem->uItemID = ITEM_GOLD_LARGE;
-                                break;
-                            default:
-                                assert(false);
-                                break;
-                            }
-                            currItem->SetIdentified();
-                            currItem->special_enchantment = (ItemEnchantment)goldAmount;
+                            currItem->generateGold(resultTreasureLevel);
                         } else {
                             pItemTable->generateItem(resultTreasureLevel, RANDOM_ITEM_ANY, currItem);
                         }

src/Engine/Objects/Items.cpp

@@ -261,6 +261,43 @@ bool ItemGen::GenerateArtifact() {
     }
 }
 
+void ItemGen::generateGold(ItemTreasureLevel treasureLevel) {
+    assert(isRandomTreasureLevel(treasureLevel));
+
+    Reset();
+    SetIdentified();
+
+    switch (treasureLevel) {
+    case ITEM_TREASURE_LEVEL_1:
+        goldAmount = grng->random(51) + 50;
+        uItemID = ITEM_GOLD_SMALL;
+        break;
+    case ITEM_TREASURE_LEVEL_2:
+        goldAmount = grng->random(101) + 100;
+        uItemID = ITEM_GOLD_SMALL;
+        break;
+    case ITEM_TREASURE_LEVEL_3:
+        goldAmount = grng->random(301) + 200;
+        uItemID = ITEM_GOLD_MEDIUM;
+        break;
+    case ITEM_TREASURE_LEVEL_4:
+        goldAmount = grng->random(501) + 500;
+        uItemID = ITEM_GOLD_MEDIUM;
+        break;
+    case ITEM_TREASURE_LEVEL_5:
+        goldAmount = grng->random(1001) + 1000;
+        uItemID = ITEM_GOLD_LARGE;
+        break;
+    case ITEM_TREASURE_LEVEL_6:
+        goldAmount = grng->random(3001) + 2000;
+        uItemID = ITEM_GOLD_LARGE;
+        break;
+    default:
+        assert(false);
+        break;
+    }
+}
+
 template<class Key, class ActualKey>
 static void AddToMap(std::map<Key, std::map<CharacterAttributeType, CEnchantment>> &map,
                      ActualKey key, CharacterAttributeType subkey, int bonusValue = 0, CharacterSkillType skill = CHARACTER_SKILL_INVALID) {

src/Engine/Objects/Items.h

@@ -51,6 +51,7 @@ struct ItemGen {  // 0x24
     inline void SetStolen() { uAttributes |= ITEM_STOLEN; }
 
     bool GenerateArtifact();
+    void generateGold(ItemTreasureLevel treasureLevel);
     unsigned int GetValue() const;
     std::string GetDisplayName();
     std::string GetIdentifiedName();

src/Engine/Objects/MonsterEnums.h

@@ -298,9 +298,6 @@ enum class MonsterId {
 
     MONSTER_FIRST = MONSTER_ANGEL_A,
     MONSTER_LAST = MONSTER_UNUSED_RAT_C,
-
-    MONSTER_FIRST_ARENA = MONSTER_ANGEL_A,
-    MONSTER_LAST_ARENA = MONSTER_GHOUL_C
 };
 using enum MonsterId;
 

src/Engine/Spells/CastSpellInfo.cpp

@@ -1502,14 +1502,14 @@ void CastSpellInfoHelpers::castSpell() {
 
                                     // step through until we hit that ench
                                     for (step = 0; step < ench_found; step++) {
-                                        current_item_apply_sum += pItemTable->pSpecialEnchantments[(ItemEnchantment)ench_array[step]].to_item_apply[this_equip_type];
+                                        current_item_apply_sum += pItemTable->pSpecialEnchantments[ench_array[step]].to_item_apply[this_equip_type];
                                         if (current_item_apply_sum >= target_item_apply_rand) {
                                             break;
                                         }
                                     }
 
                                     // set item ench
-                                    spell_item_to_enchant->special_enchantment = (ItemEnchantment)ench_array[step];
+                                    spell_item_to_enchant->special_enchantment = ench_array[step];
                                     spell_item_to_enchant->uAttributes |= ITEM_AURA_EFFECT_BLUE;
                                     ItemEnchantmentTimer = Timer::Second * 2;
                                     spell_failed = false;

test/Bin/GameTest/CMakeLists.txt

@@ -19,7 +19,7 @@ if(OE_BUILD_TESTS)
     ExternalProject_Add(OpenEnroth_TestData
             PREFIX ${CMAKE_CURRENT_BINARY_DIR}/test_data_tmp
             GIT_REPOSITORY https://github.com/OpenEnroth/OpenEnroth_TestData.git
-            GIT_TAG 94bdd06f36eabe78f51e7582fda7ea6d1a717c52
+            GIT_TAG 10ddfb20b6b1196158a3e8740c31e55d1830ee4c
             SOURCE_DIR ${CMAKE_CURRENT_BINARY_DIR}/test_data
             CONFIGURE_COMMAND ""
             BUILD_COMMAND ""

test/Bin/GameTest/GameTests.cpp

@@ -1468,10 +1468,12 @@ GAME_TEST(Issues, Issue1020) {
 }
 
 GAME_TEST(Issues, Issue1034) {
-    // Crash when casting telekinesis outdoors
+    // Crash when casting telekinesis outdoors.
+    auto houseTape = tapes.house();
+    auto statusTape = tapes.statusBar();
     test.playTraceFromTestData("issue_1034.mm7", "issue_1034.json");
-    // check we have entered into the shop
-    EXPECT_EQ(window_SpeakInHouse->houseId(), HOUSE_WEAPON_SHOP_EMERALD_ISLAND);
+    EXPECT_TRUE(statusTape.contains("Select Target")); // Telekinesis message.
+    EXPECT_EQ(houseTape, tape(HOUSE_INVALID, HOUSE_WEAPON_SHOP_EMERALD_ISLAND)); // We have entered into the shop.
 }
 
 GAME_TEST(Issues, Issue1036) {
@@ -1595,7 +1597,7 @@ GAME_TEST(Issues, Issue1191) {
     EXPECT_EQ(pParty->pCharacters[2].getActualSkillValue(CHARACTER_SKILL_DARK).level(), 0);
     EXPECT_EQ(pParty->pCharacters[2].getActualSkillValue(CHARACTER_SKILL_LIGHT).level(), 0);
 
-    // Uncomment when food issues (1226) resolved
+    // TODO(captainurist): Uncomment when food issues (1226) resolved
     // EXPECT_EQ(foodTape.delta(), -3);
     // EXPECT_EQ(pParty->GetFood(), 7);
 }
@@ -1705,3 +1707,63 @@ GAME_TEST(Issues, Issue1331) {
     auto damageRange = hpsTape.reversed().adjacentDeltas().flattened().filtered([] (int damage) { return damage > 0; }).minMax();
     EXPECT_EQ(damageRange, tape(1 * 2, (43 + 13) * 2));
 }
+
+GAME_TEST(Issues, Issue1338) {
+    // Casting telepathy on an actor and then killing it results in the actor not dropping any gold.
+    auto deadTape = actorTapes.indicesByState(AIState::Dead);
+    auto statusTape = tapes.statusBar();
+    auto goldTape = tapes.gold();
+    auto peasantGoldTape = tapes.custom([] { return pActors[18].items[3].goldAmount; });
+    test.playTraceFromTestData("issue_1338.mm7", "issue_1338.json");
+    EXPECT_EQ(deadTape, tape(std::initializer_list<int>{}, {18}, std::initializer_list<int>{})); // Alive -> Dead -> corpse picked up.
+    EXPECT_GT(peasantGoldTape.max(), 0); // Peasant should have had gold generated.
+    EXPECT_EQ(goldTape.delta(), peasantGoldTape.max());
+    EXPECT_TRUE(statusTape.contains(fmt::format("{} gold", peasantGoldTape.max()))); // Telepathy status message.
+    EXPECT_TRUE(statusTape.contains(fmt::format("You found {} gold!", peasantGoldTape.max()))); // Corpse pickup message.
+}
+
+GAME_TEST(Issues, Issue1340) {
+    // Gold piles in chests are generated with 0 gold.
+    auto goldTape = tapes.gold();
+    auto mapTape = tapes.map();
+    auto statusTape = tapes.statusBar();
+    auto screenTape = tapes.screen();
+    test.playTraceFromTestData("issue_1340.mm7", "issue_1340.json");
+    EXPECT_EQ(mapTape, tape("out01.odm", "d29.blv")); // Emerald Isle -> Castle Harmondale. Map change is important because
+                                                      // we want to trigger map respawn on first visit.
+    EXPECT_TRUE(screenTape.contains(SCREEN_CHEST));
+    EXPECT_GT(goldTape.delta(), 0); // Party should have picked some gold from the chest.
+    EXPECT_FALSE(statusTape.contains("You found 0 gold!")); // No piles of 0 size.
+    for (int gold : goldTape.adjacentDeltas())
+        EXPECT_TRUE(statusTape.contains(fmt::format("You found {} gold!", gold)));
+}
+
+GAME_TEST(Issues, Issue1341) {
+    // Can't steal gold from peasants.
+    auto goldTape = tapes.gold();
+    auto peasantGoldTape = tapes.custom([] { return pActors[6].items[3].goldAmount; });
+    auto statusTape = tapes.statusBar();
+    auto deadTape = actorTapes.countByState(AIState::Dead);
+    test.playTraceFromTestData("issue_1341.mm7", "issue_1341.json");
+    EXPECT_GT(goldTape.delta(), 0); // We did steal some gold.
+    EXPECT_EQ(peasantGoldTape.max(), goldTape.delta()); // And we did steal it from this peasant.
+    EXPECT_TRUE(statusTape.contains("Roderick failed to steal anything!")); // We have tried many times.
+    EXPECT_TRUE(statusTape.contains(fmt::format("Roderick stole {} gold!", peasantGoldTape.max()))); // And succeeded.
+    EXPECT_EQ(deadTape, tape(0)); // No one died in the process.
+}
+
+GAME_TEST(Issues, Issue1342) {
+    // Gold piles are generated with 0 gold.
+    auto goldTape = tapes.gold();
+    auto pilesTape = tapes.mapItemCount(ITEM_GOLD_SMALL);
+    auto statusTape = tapes.statusBar();
+    auto mapTape = tapes.map();
+    test.playTraceFromTestData("issue_1342.mm7", "issue_1342.json");
+    EXPECT_EQ(mapTape, tape("out01.odm", "d28.blv")); // Emerald Isle -> Dragon Cave. Map change is important here
+                                                      // because we need to trigger map respawn on first visit.
+    EXPECT_GT(goldTape.delta(), 0); // We picked up some gold.
+    EXPECT_EQ(pilesTape, tape(0, 6, 5, 4)); // Minus two small gold piles.
+    EXPECT_FALSE(statusTape.contains("You found 0 gold!")); // No piles of 0 size.
+    for (int gold : goldTape.adjacentDeltas())
+        EXPECT_TRUE(statusTape.contains(fmt::format("You found {} gold!", gold)));
+}

test/Testing/Game/CommonTapeRecorder.cpp

@@ -100,15 +100,21 @@ TestTape<bool> CommonTapeRecorder::turnBasedMode() {
 TestTape<int> CommonTapeRecorder::mapItemCount() {
     return custom([] {
         return static_cast<int>(std::ranges::count_if(pSpriteObjects, [] (const SpriteObject &object) {
-            return object.containing_item.uItemID != ITEM_NULL;
+            return object.uObjectDescID != 0 && object.containing_item.uItemID != ITEM_NULL;
         }));
     });
 }
 
 TestTape<int> CommonTapeRecorder::mapItemCount(ItemId itemId) {
     return custom([itemId] {
         return static_cast<int>(std::ranges::count_if(pSpriteObjects, [itemId] (const SpriteObject &object) {
-            return object.containing_item.uItemID == itemId;
+            return object.uObjectDescID != 0 && object.containing_item.uItemID == itemId;
         }));
     });
 }
+
+TestTape<HouseId> CommonTapeRecorder::house() {
+    return custom([] {
+        return window_SpeakInHouse ? window_SpeakInHouse->houseId() : HOUSE_INVALID;
+    });
+}

test/Testing/Game/CommonTapeRecorder.h

@@ -10,6 +10,7 @@
 #include "Engine/Time.h"
 #include "GUI/GUIEnums.h"
 #include "GUI/GUIDialogues.h"
+#include "GUI/UI/UIHouseEnums.h"
 
 #include "Library/Config/ConfigEntry.h"
 
@@ -73,6 +74,8 @@ class CommonTapeRecorder {
 
     TestTape<int> mapItemCount(ItemId itemId);
 
+    TestTape<HouseId> house();
+
  private:
     TestController *_controller = nullptr;
 };