Make invalid items unusable (#7506)

Source/CMakeLists.txt

@@ -120,6 +120,8 @@ set(libdevilutionx_SRCS
   engine/render/scrollrt.cpp
   engine/render/text_render.cpp
 
+  items/validation.cpp
+
   levels/crypt.cpp
   levels/drlg_l1.cpp
   levels/drlg_l2.cpp

Source/items.cpp

@@ -512,13 +512,19 @@ void CalcSelfItems(Player &player)
 		const int currdex = std::max(0, da + player._pBaseDex);
 
 		changeflag = false;
+		// Iterate over equipped items and remove stat bonuses if they are not valid
 		for (Item &equipment : EquippedPlayerItemsRange(player)) {
 			if (!equipment._iStatFlag)
 				continue;
 
-			if (currstr >= equipment._iMinStr
+			bool isValid = IsItemValid(player, equipment);
-			    && currmag >= equipment._iMinMag
+
-			    && currdex >= equipment._iMinDex)
+			if (currstr < equipment._iMinStr
+			    || currmag < equipment._iMinMag
+			    || currdex < equipment._iMinDex)
+				isValid = false;
+
+			if (isValid)
 				continue;
 
 			changeflag = true;
@@ -1954,7 +1960,7 @@ void SpawnOnePremium(Item &premiumItem, int plvl, const Player &player)
 		GetItemBonus(player, premiumItem, plvl / 2, plvl, true, !gbIsHellfire);
 
 		if (!gbIsHellfire) {
-			if (premiumItem._iIvalue <= 140000) {
+			if (premiumItem._iIvalue <= MaxVendorValue) {
 				break;
 			}
 		} else {
@@ -1991,7 +1997,7 @@ void SpawnOnePremium(Item &premiumItem, int plvl, const Player &player)
 				break;
 			}
 			itemValue = itemValue * 4 / 5; // avoids forced int > float > int conversion
-			if (premiumItem._iIvalue <= 200000
+			if (premiumItem._iIvalue <= MaxVendorValueHf
 			    && premiumItem._iMinStr <= strength
 			    && premiumItem._iMinMag <= magic
 			    && premiumItem._iMinDex <= dexterity
@@ -4371,10 +4377,10 @@ void SpawnSmith(int lvl)
 {
 	constexpr int PinnedItemCount = 0;
 
-	int maxValue = 140000;
+	int maxValue = MaxVendorValue;
 	int maxItems = 19;
 	if (gbIsHellfire) {
-		maxValue = 200000;
+		maxValue = MaxVendorValueHf;
 		maxItems = 24;
 	}
 
@@ -4442,7 +4448,7 @@ void SpawnWitch(int lvl)
 	int bookCount = 0;
 	const int pinnedBookCount = gbIsHellfire ? RandomIntLessThan(MaxPinnedBookCount) : 0;
 	const int itemCount = RandomIntBetween(10, gbIsHellfire ? 24 : 17);
-	const int maxValue = gbIsHellfire ? 200000 : 140000;
+	const int maxValue = gbIsHellfire ? MaxVendorValueHf : MaxVendorValue;
 
 	for (int i = 0; i < WITCH_ITEMS; i++) {
 		Item &item = WitchItems[i];
@@ -4527,7 +4533,7 @@ void SpawnBoy(int lvl)
 		GetItemBonus(*MyPlayer, BoyItem, lvl, 2 * lvl, true, true);
 
 		if (!gbIsHellfire) {
-			if (BoyItem._iIvalue > 90000) {
+			if (BoyItem._iIvalue > MaxBoyValue) {
 				keepgoing = true; // prevent breaking the do/while loop too early by failing hellfire's condition in while
 				continue;
 			}
@@ -4602,7 +4608,7 @@ void SpawnBoy(int lvl)
 		}
 	} while (keepgoing
 	    || ((
-	            BoyItem._iIvalue > 200000
+	            BoyItem._iIvalue > MaxBoyValueHf
 	            || BoyItem._iMinStr > strength
 	            || BoyItem._iMinMag > magic
 	            || BoyItem._iMinDex > dexterity

Source/items.h

@@ -29,6 +29,11 @@ namespace devilution {
 // Item indestructible durability
 #define DUR_INDESTRUCTIBLE 255
 
+constexpr int MaxVendorValue = 140000;
+constexpr int MaxVendorValueHf = 200000;
+constexpr int MaxBoyValue = 90000;
+constexpr int MaxBoyValueHf = 200000;
+
 enum item_quality : uint8_t {
 	ITEM_QUALITY_NORMAL,
 	ITEM_QUALITY_MAGIC,

Source/items/validation.cpp

@@ -0,0 +1,155 @@
+/**
+ * @file items/validation.cpp
+ *
+ * Implementation of functions for validation of player and item data.
+ */
+
+#include "items/validation.h"
+
+#include <cstdint>
+
+#include "items.h"
+#include "monstdat.h"
+#include "player.h"
+
+namespace devilution {
+
+namespace {
+
+bool hasMultipleFlags(uint16_t flags)
+{
+	return (flags & (flags - 1)) > 0;
+}
+
+} // namespace
+
+bool IsCreationFlagComboValid(uint16_t iCreateInfo)
+{
+	iCreateInfo = iCreateInfo & ~CF_LEVEL;
+	const bool isTownItem = (iCreateInfo & CF_TOWN) != 0;
+	const bool isPregenItem = (iCreateInfo & CF_PREGEN) != 0;
+	const bool isUsefulItem = (iCreateInfo & CF_USEFUL) == CF_USEFUL;
+
+	if (isPregenItem) {
+		// Pregen flags are discarded when an item is picked up, therefore impossible to have in the inventory
+		return false;
+	}
+	if (isUsefulItem && (iCreateInfo & ~CF_USEFUL) != 0)
+		return false;
+	if (isTownItem && hasMultipleFlags(iCreateInfo)) {
+		// Items from town can only have 1 towner flag
+		return false;
+	}
+	return true;
+}
+
+bool IsTownItemValid(uint16_t iCreateInfo, const Player &player)
+{
+	const uint8_t level = iCreateInfo & CF_LEVEL;
+	const bool isBoyItem = (iCreateInfo & CF_BOY) != 0;
+	const uint8_t maxTownItemLevel = 30;
+
+	// Wirt items in multiplayer are equal to the level of the player, therefore they cannot exceed the max character level
+	if (isBoyItem && level <= player.getMaxCharacterLevel())
+		return true;
+
+	return level <= maxTownItemLevel;
+}
+
+bool IsShopPriceValid(const Item &item)
+{
+	const int boyPriceLimit = gbIsHellfire ? MaxBoyValueHf : MaxBoyValue;
+	if ((item._iCreateInfo & CF_BOY) != 0 && item._iIvalue > boyPriceLimit)
+		return false;
+
+	const uint16_t smithOrWitch = CF_SMITH | CF_SMITHPREMIUM | CF_WITCH;
+	const int smithAndWitchPriceLimit = gbIsHellfire ? MaxVendorValueHf : MaxVendorValue;
+	if ((item._iCreateInfo & smithOrWitch) != 0 && item._iIvalue > smithAndWitchPriceLimit)
+		return false;
+
+	return true;
+}
+
+bool IsUniqueMonsterItemValid(uint16_t iCreateInfo, uint32_t dwBuff)
+{
+	const uint8_t level = iCreateInfo & CF_LEVEL;
+
+	// Check all unique monster levels to see if they match the item level
+	for (const UniqueMonsterData &uniqueMonsterData : UniqueMonstersData) {
+		const auto &uniqueMonsterLevel = static_cast<uint8_t>(MonstersData[uniqueMonsterData.mtype].level);
+
+		if (IsAnyOf(uniqueMonsterData.mtype, MT_DEFILER, MT_NAKRUL, MT_HORKDMN)) {
+			// These monsters don't use their mlvl for item generation
+			continue;
+		}
+
+		if (level == uniqueMonsterLevel) {
+			// If the ilvl matches the mlvl, we confirm the item is legitimate
+			return true;
+		}
+	}
+
+	return false;
+}
+
+bool IsDungeonItemValid(uint16_t iCreateInfo, uint32_t dwBuff)
+{
+	const uint8_t level = iCreateInfo & CF_LEVEL;
+	const bool isHellfireItem = (dwBuff & CF_HELLFIRE) != 0;
+
+	// Check all monster levels to see if they match the item level
+	for (int16_t i = 0; i < static_cast<int16_t>(NUM_MTYPES); i++) {
+		const auto &monsterData = MonstersData[i];
+		auto monsterLevel = static_cast<uint8_t>(monsterData.level);
+
+		if (i != MT_DIABLO && monsterData.availability == MonsterAvailability::Never) {
+			// Skip monsters that are unable to appear in the game
+			continue;
+		}
+
+		if (i == MT_DIABLO && !isHellfireItem) {
+			// Adjust The Dark Lord's mlvl if the item isn't a Hellfire item to match the Diablo mlvl
+			monsterLevel -= 15;
+		}
+
+		if (level == monsterLevel) {
+			// If the ilvl matches the mlvl, we confirm the item is legitimate
+			return true;
+		}
+	}
+
+	if (isHellfireItem) {
+		uint8_t hellfireMaxDungeonLevel = 24;
+
+		// Hellfire adjusts the currlevel minus 7 in dungeon levels 20-24 for generating items
+		hellfireMaxDungeonLevel -= 7;
+		return level <= (hellfireMaxDungeonLevel * 2);
+	}
+
+	uint8_t diabloMaxDungeonLevel = 16;
+
+	// Diablo doesn't have containers that drop items in dungeon level 16, therefore we decrement by 1
+	diabloMaxDungeonLevel--;
+	return level <= (diabloMaxDungeonLevel * 2);
+}
+
+bool IsItemValid(const Player &player, const Item &item)
+{
+	if (item.IDidx != IDI_GOLD && !IsCreationFlagComboValid(item._iCreateInfo))
+		return false;
+
+	if ((item._iCreateInfo & CF_TOWN) != 0) {
+		if (!IsTownItemValid(item._iCreateInfo, player) || !IsShopPriceValid(item))
+			return false;
+	} else if ((item._iCreateInfo & CF_USEFUL) == CF_UPER15) {
+		if (!IsUniqueMonsterItemValid(item._iCreateInfo, item.dwBuff))
+			return false;
+	}
+
+	if (!IsDungeonItemValid(item._iCreateInfo, item.dwBuff))
+		return false;
+
+	return true;
+}
+
+} // namespace devilution

Source/items/validation.h

@@ -0,0 +1,23 @@
+/**
+ * @file items/validation.h
+ *
+ * Interface of functions for validation of player and item data.
+ */
+#pragma once
+
+#include <cstdint>
+
+// Forward declared structs to avoid circular dependencies
+struct Item;
+struct Player;
+
+namespace devilution {
+
+bool IsCreationFlagComboValid(uint16_t iCreateInfo);
+bool IsTownItemValid(uint16_t iCreateInfo, const Player &player);
+bool IsShopPriceValid(const Item &item);
+bool IsUniqueMonsterItemValid(uint16_t iCreateInfo, uint32_t dwBuff);
+bool IsDungeonItemValid(uint16_t iCreateInfo, uint32_t dwBuff);
+bool IsItemValid(const Player &player, const Item &item);
+
+} // namespace devilution

Source/loadsave.cpp

@@ -582,7 +582,7 @@ void LoadPlayer(LoadHelper &file, Player &player)
 	sgGameInitInfo.nDifficulty = static_cast<_difficulty>(file.NextLE<uint32_t>());
 	player.pDamAcFlags = static_cast<ItemSpecialEffectHf>(file.NextLE<uint32_t>());
 	file.Skip(20); // Available bytes
-	CalcPlrItemVals(player, false);
+	CalcPlrInv(player, false);
 
 	player.executedSpell = player.queuedSpell; // Ensures backwards compatibility
 
@@ -972,24 +972,6 @@ bool LevelFileExists(SaveWriter &archive)
 	return archive.HasFile(szName);
 }
 
-bool IsShopPriceValid(const Item &item)
-{
-	const int boyPriceLimit = 90000;
-	if (!gbIsHellfire && (item._iCreateInfo & CF_BOY) != 0 && item._iIvalue > boyPriceLimit)
-		return false;
-
-	const int premiumPriceLimit = 140000;
-	if (!gbIsHellfire && (item._iCreateInfo & CF_SMITHPREMIUM) != 0 && item._iIvalue > premiumPriceLimit)
-		return false;
-
-	const uint16_t smithOrWitch = CF_SMITH | CF_WITCH;
-	const int smithAndWitchPriceLimit = gbIsHellfire ? 200000 : 140000;
-	if ((item._iCreateInfo & smithOrWitch) != 0 && item._iIvalue > smithAndWitchPriceLimit)
-		return false;
-
-	return true;
-}
-
 void LoadMatchingItems(LoadHelper &file, const Player &player, const int n, Item *pItem)
 {
 	Item heroItem;
@@ -1015,10 +997,6 @@ void LoadMatchingItems(LoadHelper &file, const Player &player, const int n, Item
 				unpackedItem._iMaxCharges = std::clamp<int>(heroItem._iMaxCharges, 0, unpackedItem._iMaxCharges);
 				unpackedItem._iCharges = std::clamp<int>(heroItem._iCharges, 0, unpackedItem._iMaxCharges);
 			}
-			if (!IsShopPriceValid(unpackedItem)) {
-				unpackedItem.clear();
-				continue;
-			}
 			if (gbIsHellfire) {
 				unpackedItem._iPLToHit = ClampToHit(unpackedItem, heroItem._iPLToHit); // Oil of Accuracy
 				unpackedItem._iMaxDam = ClampMaxDam(unpackedItem, heroItem._iMaxDam);  // Oil of Sharpness

Source/pack.cpp

@@ -9,6 +9,7 @@
 
 #include "engine/random.hpp"
 #include "init.h"
+#include "items/validation.h"
 #include "loadsave.h"
 #include "playerdat.hpp"
 #include "plrmsg.h"
@@ -75,109 +76,8 @@ void VerifyGoldSeeds(Player &player)
 	}
 }
 
-bool hasMultipleFlags(uint16_t flags)
-{
-	return (flags & (flags - 1)) > 0;
-}
-
 } // namespace
 
-bool IsCreationFlagComboValid(uint16_t iCreateInfo)
-{
-	iCreateInfo = iCreateInfo & ~CF_LEVEL;
-	const bool isTownItem = (iCreateInfo & CF_TOWN) != 0;
-	const bool isPregenItem = (iCreateInfo & CF_PREGEN) != 0;
-	const bool isUsefulItem = (iCreateInfo & CF_USEFUL) == CF_USEFUL;
-
-	if (isPregenItem) {
-		// Pregen flags are discarded when an item is picked up, therefore impossible to have in the inventory
-		return false;
-	}
-	if (isUsefulItem && (iCreateInfo & ~CF_USEFUL) != 0)
-		return false;
-	if (isTownItem && hasMultipleFlags(iCreateInfo)) {
-		// Items from town can only have 1 towner flag
-		return false;
-	}
-	return true;
-}
-
-bool IsTownItemValid(uint16_t iCreateInfo, const Player &player)
-{
-	const uint8_t level = iCreateInfo & CF_LEVEL;
-	const bool isBoyItem = (iCreateInfo & CF_BOY) != 0;
-	const uint8_t maxTownItemLevel = 30;
-
-	// Wirt items in multiplayer are equal to the level of the player, therefore they cannot exceed the max character level
-	if (isBoyItem && level <= player.getMaxCharacterLevel())
-		return true;
-
-	return level <= maxTownItemLevel;
-}
-
-bool IsUniqueMonsterItemValid(uint16_t iCreateInfo, uint32_t dwBuff)
-{
-	const uint8_t level = iCreateInfo & CF_LEVEL;
-
-	// Check all unique monster levels to see if they match the item level
-	for (const UniqueMonsterData &uniqueMonsterData : UniqueMonstersData) {
-		const auto &uniqueMonsterLevel = static_cast<uint8_t>(MonstersData[uniqueMonsterData.mtype].level);
-
-		if (IsAnyOf(uniqueMonsterData.mtype, MT_DEFILER, MT_NAKRUL, MT_HORKDMN)) {
-			// These monsters don't use their mlvl for item generation
-			continue;
-		}
-
-		if (level == uniqueMonsterLevel) {
-			// If the ilvl matches the mlvl, we confirm the item is legitimate
-			return true;
-		}
-	}
-
-	return false;
-}
-
-bool IsDungeonItemValid(uint16_t iCreateInfo, uint32_t dwBuff)
-{
-	const uint8_t level = iCreateInfo & CF_LEVEL;
-	const bool isHellfireItem = (dwBuff & CF_HELLFIRE) != 0;
-
-	// Check all monster levels to see if they match the item level
-	for (int16_t i = 0; i < static_cast<int16_t>(NUM_MTYPES); i++) {
-		const auto &monsterData = MonstersData[i];
-		auto monsterLevel = static_cast<uint8_t>(monsterData.level);
-
-		if (i != MT_DIABLO && monsterData.availability == MonsterAvailability::Never) {
-			// Skip monsters that are unable to appear in the game
-			continue;
-		}
-
-		if (i == MT_DIABLO && !isHellfireItem) {
-			// Adjust The Dark Lord's mlvl if the item isn't a Hellfire item to match the Diablo mlvl
-			monsterLevel -= 15;
-		}
-
-		if (level == monsterLevel) {
-			// If the ilvl matches the mlvl, we confirm the item is legitimate
-			return true;
-		}
-	}
-
-	if (isHellfireItem) {
-		uint8_t hellfireMaxDungeonLevel = 24;
-
-		// Hellfire adjusts the currlevel minus 7 in dungeon levels 20-24 for generating items
-		hellfireMaxDungeonLevel -= 7;
-		return level <= (hellfireMaxDungeonLevel * 2);
-	}
-
-	uint8_t diabloMaxDungeonLevel = 16;
-
-	// Diablo doesn't have containers that drop items in dungeon level 16, therefore we decrement by 1
-	diabloMaxDungeonLevel--;
-	return level <= (diabloMaxDungeonLevel * 2);
-}
-
 bool RecreateHellfireSpellBook(const Player &player, const TItem &packedItem, Item *item)
 {
 	Item spellBook {};

Source/pack.h

@@ -142,10 +142,6 @@ struct PlayerNetPack {
 };
 #pragma pack(pop)
 
-bool IsCreationFlagComboValid(uint16_t iCreateInfo);
-bool IsTownItemValid(uint16_t iCreateInfo, const Player &player);
-bool IsUniqueMonsterItemValid(uint16_t iCreateInfo, uint32_t dwBuff);
-bool IsDungeonItemValid(uint16_t iCreateInfo, uint32_t dwBuff);
 bool RecreateHellfireSpellBook(const Player &player, const TItem &packedItem, Item *item = nullptr);
 void PackPlayer(PlayerPack &pPack, const Player &player);
 void UnPackPlayer(const PlayerPack &pPack, Player &player);

Source/player.cpp

@@ -1527,6 +1527,16 @@ void Player::CalcScrolls()
 	EnsureValidReadiedSpell(*this);
 }
 
+bool Player::CanUseItem(const Item &item) const
+{
+	if (!IsItemValid(*this, item))
+		return false;
+
+	return _pStrength >= item._iMinStr
+	    && _pMagic >= item._iMinMag
+	    && _pDexterity >= item._iMinDex;
+}
+
 void Player::RemoveInvItem(int iv, bool calcScrolls)
 {
 	if (this == MyPlayer) {

Source/player.h

@@ -20,6 +20,7 @@
 #include "engine/point.hpp"
 #include "interfac.h"
 #include "items.h"
+#include "items/validation.h"
 #include "levels/gendung.h"
 #include "multi.h"
 #include "playerdat.hpp"
@@ -398,12 +399,7 @@ public:
 
 	void CalcScrolls();
 
-	bool CanUseItem(const Item &item) const
+	bool CanUseItem(const Item &item) const;
-	{
-		return _pStrength >= item._iMinStr
-		    && _pMagic >= item._iMinMag
-		    && _pDexterity >= item._iMinDex;
-	}
 
 	bool CanCleave()
 	{

test/missiles_test.cpp

@@ -45,7 +45,7 @@ TEST(Missiles, RotateBlockedMissileArrow)
 	*MyPlayer = {};
 	LoadMissileData();
 
-	Player &player = Players[0];
+	devilution::Player &player = Players[0];
 	// missile can be a copy or a reference, there's no nullptr check and the functions that use it don't expect the instance to be part of a global structure so it doesn't really matter for this use.
 	Missile missile = *AddMissile({ 0, 0 }, { 0, 0 }, Direction::South, MissileID::Arrow, TARGET_MONSTERS, player, 0, 0);
 

test/player_test.cpp

@@ -14,7 +14,7 @@ extern bool TestPlayerDoGotHit(Player &player);
 
 int RunBlockTest(int frames, ItemSpecialEffect flags)
 {
-	Player &player = Players[0];
+	devilution::Player &player = Players[0];
 
 	player._pHFrames = frames;
 	player._pIFlags = flags;
@@ -90,7 +90,7 @@ TEST(Player, PM_DoGotHit)
 	}
 }
 
-static void AssertPlayer(Player &player)
+static void AssertPlayer(devilution::Player &player)
 {
 	ASSERT_EQ(CountU8(player._pSplLvl, 64), 0);
 	ASSERT_EQ(Count8(player.InvGrid, InventoryGridCells), 1);
@@ -189,6 +189,7 @@ TEST(Player, CreatePlayer)
 	ASSERT_TRUE(HaveSpawn() || HaveDiabdat());
 
 	LoadPlayerDataFiles();
+	LoadMonsterData();
 	LoadItemData();
 	Players.resize(1);
 	CreatePlayer(Players[0], HeroClass::Rogue);

test/player_test.h

@@ -10,9 +10,9 @@
 
 using namespace devilution;
 
-static size_t CountItems(Item *items, int n)
+static size_t CountItems(devilution::Item *items, int n)
 {
-	return std::count_if(items, items + n, [](Item x) { return !x.isEmpty(); });
+	return std::count_if(items, items + n, [](devilution::Item x) { return !x.isEmpty(); });
 }
 
 static size_t Count8(int8_t *ints, int n)

test/stores_test.cpp

@@ -8,7 +8,7 @@ namespace {
 
 TEST(Stores, AddStoreHoldRepair_magic)
 {
-	Item *item;
+	devilution::Item *item;
 
 	item = &PlayerItems[0];
 
@@ -41,7 +41,7 @@ TEST(Stores, AddStoreHoldRepair_magic)
 
 TEST(Stores, AddStoreHoldRepair_normal)
 {
-	Item *item;
+	devilution::Item *item;
 
 	item = &PlayerItems[0];
 

test/writehero_test.cpp

@@ -386,6 +386,7 @@ TEST(Writehero, pfile_write_hero)
 
 	LoadSpellData();
 	LoadPlayerDataFiles();
+	LoadMonsterData();
 	LoadItemData();
 	_uiheroinfo info {};
 	info.heroclass = HeroClass::Rogue;