improve: rework of Item Attributes for better stability/maintability (o…

…pentibiabr#827)

This brings in a major overhaul of the Item Attributes component, addressing long-standing issues in the TFS and OpenTibiaServer project. Adopting class inheritance and harnessing the latest capabilities of C++, this update resolves specific crashes and streamlines future changes to the code. The Attributes class has been separated into its own file, making the code more readable and easier to understand, while also removing any confusing and redundant functions from the Item class. This results in a more stable, maintainable, and user-friendly solution for Item Attributes.

Simplified code by moving to shared_ptr for memory management, removing redundant functions, and revamping key classes such as Attributes, ItemAttribute, and CustomAttribute. The deprecated "set/getSpecialAttribute" functions have been replaced with more efficient "set/getCustomAttribute." This simplified approach leads to better code maintenance and increased stability, for example, Hirelings now utilize the CustomAttribute.

data-otservbr-global/scripts/actions/other/hirelinglamp.lua

@@ -2,7 +2,7 @@ local hirelingLamp = Action()
 
 function hirelingLamp.onUse(player, item, fromPosition, target, toPosition, isHotkey)
 	local spawnPosition = player:getPosition()
-	local hireling_id = item:getSpecialAttribute(HIRELING_ATTRIBUTE)
+	local hireling_id = item:getCustomAttribute("Hireling")
 	local house = spawnPosition and spawnPosition:getTile() and spawnPosition:getTile():getHouse() or nil
 	if not house then
 		player:getPosition():sendMagicEffect(CONST_ME_POFF)
@@ -23,6 +23,13 @@ function hirelingLamp.onUse(player, item, fromPosition, target, toPosition, isHo
 	end
 
 	local hireling = getHirelingById(hireling_id)
+	if not hireling then
+		player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "There was an error creating the hireling and it has been deleted, please, contact server admin.")
+		Spdlog.error(string.format("[hirelingLamp.onUse] Player '%s' is using hireling not exist in the database", player:getName(), hireling_id))
+		Spdlog.error("Deleted the lamp")
+		item:remove(1)
+		return true
+	end
 
 	hireling:setPosition(spawnPosition)
 	item:remove(1)
@@ -31,5 +38,5 @@ function hirelingLamp.onUse(player, item, fromPosition, target, toPosition, isHo
 	return true
 end
 
-hirelingLamp:id(HIRELING_LAMP_ID)
+hirelingLamp:id(HIRELING_LAMP)
 hirelingLamp:register()

data-otservbr-global/scripts/actions/quests/dangerous_depth/lever_barrel.lua

@@ -25,8 +25,8 @@ local function checarPos(item)
 	for _, info1 in pairs(config.first) do
 		local fromPos, toPos, stgRoom = info1.fromPosition, info1.toPosition, info1.stgRoom
 		if item:getPosition():isInRange(fromPos, toPos) then
-			local stgBarril = item:getSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
-			local player = Player(stgBarril)
+			local stgbarrel = item:getCustomAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
+			local player = Player(stgbarrel)
 			if player then
 				if player:getStorageValue(stgRoom) < 1 then
 				player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelCount, player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelCount) + 1)
@@ -38,8 +38,8 @@ local function checarPos(item)
 	for _, info2 in pairs(config.second) do
 		local fromPos, toPos, stgRoom = info2.fromPosition, info2.toPosition, info2.stgRoom
 		if item:getPosition():isInRange(fromPos, toPos) then
-			local stgBarril = item:getSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
-			local player = Player(stgBarril)
+			local stgbarrel = item:getCustomAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
+			local player = Player(stgbarrel)
 			if player then
 				if player:getStorageValue(stgRoom) < 1 then
 				player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelCount, player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelCount) + 1)
@@ -51,8 +51,8 @@ local function checarPos(item)
 	for _, info3 in pairs(config.third) do
 		local fromPos, toPos, stgRoom = info3.fromPosition, info3.toPosition, info3.stgRoom
 		if item:getPosition():isInRange(fromPos, toPos) then
-			local stgBarril = item:getSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
-			local player = Player(stgBarril)
+			local stgbarrel = item:getCustomAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
+			local player = Player(stgbarrel)
 			if player then
 				if player:getStorageValue(stgRoom) < 1 then
 				player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelCount, player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelCount) + 1)
@@ -64,8 +64,8 @@ local function checarPos(item)
 	for _, info4 in pairs(config.fourth) do
 		local fromPos, toPos, stgRoom = info4.fromPosition, info4.toPosition, info4.stgRoom
 		if item:getPosition():isInRange(fromPos, toPos) then
-			local stgBarril = item:getSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
-			local player = Player(stgBarril)
+			local stgbarrel = item:getCustomAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
+			local player = Player(stgbarrel)
 			if player then
 				if player:getStorageValue(stgRoom) < 1 then
 				player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelCount, player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelCount) + 1)
@@ -77,8 +77,8 @@ local function checarPos(item)
 	for _, info5 in pairs(config.fifth) do
 		local fromPos, toPos, stgRoom = info5.fromPosition, info5.toPosition, info5.stgRoom
 		if item:getPosition():isInRange(fromPos, toPos) then
-			local stgBarril = item:getSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
-			local player = Player(stgBarril)
+			local stgbarrel = item:getCustomAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
+			local player = Player(stgbarrel)
 			if player then
 				if player:getStorageValue(stgRoom) < 1 then
 				player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelCount, player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelCount) + 1)
@@ -125,23 +125,25 @@ function dangerousDepthLever.onUse(player, item)
 		return true
 	end
 
-	local posBarril = Position(33838, 32077, 14)
+	local posBarrel = Position(33838, 32077, 14)
 	local stgCount = player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelCount)
 	local BarrelTimer = player:getStorageValue(Storage.DangerousDepths.Scouts.BarrelTimer)
 
 	if item:getId() == 2772 then
 		if player:getStorageValue(Storage.DangerousDepths.Scouts.Growth) == 1 and stgCount < 5 and BarrelTimer <= 0 then
-			local barril = Game.createItem(31992, 1, posBarril)
-			local stgBarril = barril:getSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel) or -1
-			barril:setSpecialAttribute(Storage.DangerousDepths.Scouts.Barrel, player:getId())
+			local barrel = Game.createItem(27492, 1, posBarrel)
+			if not barrel then
+				return false
+			end
+			barrel:setCustomAttribute(Storage.DangerousDepths.Scouts.Barrel, player:getId())
 			addEvent(function()
-				if barril then
-					explode(barril)
+				if barrel then
+					explode(barrel)
 				end
 			player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelTimer, 0)
 			end, 2 * 60 * 1000)
-			player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelTimer, os.time() + 2*60) -- Só para barrar.
-			--O tempo é setado em 0 ao barril explodir.
+			--Time is set to 0 when barrel explodes
+			player:setStorageValue(Storage.DangerousDepths.Scouts.BarrelTimer, os.time() + 2 * 60)
 		end
 	end
 	item:transform(transformid[item:getId()])

data-otservbr-global/scripts/movements/quests/cults_of_tibia/begin_task.lua

@@ -26,11 +26,11 @@ function beginTask.onEquip(player, item, slot, isCheck)
 
 	if player:getStorageValue(Storage.CultsOfTibia.Misguided.Mission) >= 2 and
 	player:getStorageValue(Storage.CultsOfTibia.Misguided.Mission) <= 3 then
-		local equippedBefore = item:getSpecialAttribute("task") or 0
+		local equippedBefore = item:getCustomAttribute("task") or 0
 		if equippedBefore ~= player:getGuid() and
 		player:getStorageValue(Storage.CultsOfTibia.Misguided.Monsters) < 10 then
 			player:setStorageValue(Storage.CultsOfTibia.Misguided.Monsters, 0)
-			item:setSpecialAttribute("task", player:getGuid())
+			item:setCustomAttribute("task", player:getGuid())
 		end
 		if player:getStorageValue(Storage.CultsOfTibia.Misguided.Mission) == 2 then
 			player:setStorageValue(Storage.CultsOfTibia.Misguided.Mission, 3)

data-otservbr-global/scripts/movements/quests/cults_of_tibia/task_teleport.lua

@@ -32,14 +32,14 @@ function taskTeleport.onStepIn(creature, item, position, fromPosition)
 		local teleport = Tile(position):getItemById(index)
 		if teleport then
 			local storage = (player:getStorageValue(value.storage) < 0 and 0 or player:getStorageValue(value.storage))
-			local attribute = teleport:getSpecialAttribute("task") or ''
+			local attribute = teleport:getCustomAttribute("task") or ''
 			if attribute:find(player:getName()) or storage >= value.max then
 				player:sendTextMessage(MESSAGE_EVENT_ADVANCE,
 				"The power of these souls is now within you. You cannot absorb any more souls.")
 				return false
 			end
 			attribute = string.format("%s, %s", attribute, player:getName())
-			teleport:setSpecialAttribute("task", attribute)
+			teleport:setCustomAttribute("task", attribute)
 			player:setStorageValue(value.storage, storage + 1)
 			player:getPosition():sendMagicEffect(value.effect)
 			teleport:remove()

data/libs/functions/functions.lua

@@ -764,19 +764,6 @@ function unserializeTable(str, out)
 	return table.copy(tmp, out)
 end
 
-local function setTableIndexes(t, i, v, ...)
-	if i and v then
-		t[i] = v
-		return setTableIndexes(t, ...)
-	end
-end
-
-local function getTableIndexes(t, i, ...)
-	if i then
-		return t[i], getTableIndexes(t, ...)
-	end
-end
-
 function unpack2(tab, i)
 	local i, v = next(tab, i)
 	if next(tab, i) then
@@ -794,37 +781,6 @@ function pack(t, ...)
 	return t
 end
 
-function Item:setSpecialAttribute(...)
-	local tmp
-	if self:hasAttribute(ITEM_ATTRIBUTE_SPECIAL) then
-		tmp = self:getAttribute(ITEM_ATTRIBUTE_SPECIAL)
-	else
-		tmp = "{}"
-	end
-
-	local tab = unserializeTable(tmp)
-	if tab then
-		setTableIndexes(tab, ...)
-		tmp = serializeTable(tab)
-		self:setAttribute(ITEM_ATTRIBUTE_SPECIAL, tmp)
-		return true
-	end
-end
-
-function Item:getSpecialAttribute(...)
-	local tmp
-	if self:hasAttribute(ITEM_ATTRIBUTE_SPECIAL) then
-		tmp = self:getAttribute(ITEM_ATTRIBUTE_SPECIAL)
-	else
-		tmp = "{}"
-	end
-
-	local tab = unserializeTable(tmp)
-	if tab then
-		return getTableIndexes(tab, ...)
-	end
-end
-
 if not PLAYER_STORAGE then
 	PLAYER_STORAGE = {}
 end

data/libs/hireling_lib.lua

@@ -72,9 +72,6 @@ HIRELING_STORAGE = {
 	OUTFIT = 28900
 }
 
-HIRELING_LAMP_ID = 29432
-HIRELING_ATTRIBUTE = "HIRELING_ID"
-
 HIRELING_FOODS_BOOST = {
 	MAGIC = 29410,
 	MELEE = 29411,
@@ -113,9 +110,9 @@ local function checkHouseAccess(hireling)
 	Spdlog.info("Returning Hireling:" .. hireling:getName() .. " to owner Inbox")
 	local inbox = player:getSlotItem(CONST_SLOT_STORE_INBOX)
 	-- Using FLAG_NOLIMIT to avoid losing the hireling after being kicked out of the house and having no slots available in the store inbox
-	local lamp = inbox:addItem(HIRELING_LAMP_ID, 1, INDEX_WHEREEVER, FLAG_NOLIMIT)
+	local lamp = inbox:addItem(HIRELING_LAMP, 1, INDEX_WHEREEVER, FLAG_NOLIMIT)
 	lamp:setAttribute(ITEM_ATTRIBUTE_DESCRIPTION, "This mysterious lamp summons your very own personal hireling.\nThis item cannot be traded.\nThis magic lamp is the home of " .. hireling:getName() .. ".")
-	lamp:setSpecialAttribute(HIRELING_ATTRIBUTE, hireling:getId()) --save hirelingId on item
+	lamp:setCustomAttribute("Hireling", hireling:getId()) --save hirelingId on item
 	player:save()
 	hireling.active = 0
 	hireling.cid = -1
@@ -381,7 +378,7 @@ function Hireling:returnToLamp(player_id)
 			return
 		end
 
-		local lampType = ItemType(HIRELING_LAMP_ID)
+		local lampType = ItemType(HIRELING_LAMP)
 		if owner:getFreeCapacity() < lampType:getWeight(1) then
 			owner:getPosition():sendMagicEffect(CONST_ME_POFF)
 			return owner:sendTextMessage(MESSAGE_FAILURE, "You do not have enough capacity.")
@@ -399,11 +396,11 @@ function Hireling:returnToLamp(player_id)
 		end
 
 		npc:say("As you wish!",	TALKTYPE_PRIVATE_NP, false, owner, npc:getPosition())
-		local lamp = inbox:addItem(HIRELING_LAMP_ID, 1, INDEX_WHEREEVER, FLAG_NOLIMIT)
+		local lamp = inbox:addItem(HIRELING_LAMP, 1, INDEX_WHEREEVER, FLAG_NOLIMIT)
 		npc:getPosition():sendMagicEffect(CONST_ME_PURPLESMOKE)
 		npc:remove() --remove hireling
 		lamp:setAttribute(ITEM_ATTRIBUTE_DESCRIPTION, "This mysterious lamp summons your very own personal hireling.\nThis item cannot be traded.\nThis magic lamp is the home of " .. self:getName() .. ".")
-		lamp:setSpecialAttribute(HIRELING_ATTRIBUTE, hirelingId) --save hirelingId on item
+		lamp:setCustomAttribute("Hireling", hirelingId) --save hirelingId on item
 		hireling:setPosition({x=0,y=0,z=0})
 	end, 1000, self.cid, player:getGuid(), self.id)
 end
@@ -420,7 +417,7 @@ function getHirelingById(id)
 	local hireling
 	for i = 1, #HIRELINGS do
 		hireling = HIRELINGS[i]
-		if hireling:getId() == id then
+		if hireling:getId() == tonumber(id) then
 			return hireling
 		end
 	end
@@ -525,7 +522,7 @@ function Player:addNewHireling(name, sex)
 		hireling.sex = HIRELING_SEX.MALE
 	end
 
-	local lampType = ItemType(HIRELING_LAMP_ID)
+	local lampType = ItemType(HIRELING_LAMP)
 	if self:getFreeCapacity() < lampType:getWeight(1) then
 		self:getPosition():sendMagicEffect(CONST_ME_POFF)
 		self:sendTextMessage(MESSAGE_FAILURE, "You do not have enough capacity.")
@@ -549,9 +546,9 @@ function Player:addNewHireling(name, sex)
 		end
 		table.insert(PLAYER_HIRELINGS[self:getGuid()], hireling)
 		table.insert(HIRELINGS, hireling)
-		local lamp = inbox:addItem(HIRELING_LAMP_ID, 1, INDEX_WHEREEVER, FLAG_NOLIMIT)
+		local lamp = inbox:addItem(HIRELING_LAMP, 1, INDEX_WHEREEVER, FLAG_NOLIMIT)
 		lamp:setAttribute(ITEM_ATTRIBUTE_DESCRIPTION, "This mysterious lamp summons your very own personal hireling.\nThis item cannot be traded.\nThis magic lamp is the home of " .. hireling:getName() .. ".")
-		lamp:setSpecialAttribute(HIRELING_ATTRIBUTE, hireling:getId()) --save hirelingId on item
+		lamp:setCustomAttribute("Hireling", hireling:getId()) --save hirelingId on item
 		hireling.active = 0
 		return hireling
 	end
@@ -631,7 +628,7 @@ function Player:findHirelingLamp(hirelingId)
 	local lastIndex = inbox:getSize() - 1
 	for i=0,lastIndex do
 		local item = inbox:getItem(i)
-		if item and item:getId() == HIRELING_LAMP_ID and item:getSpecialAttribute(HIRELING_ATTRIBUTE) == hirelingId then
+		if item and item:getId() == HIRELING_LAMP and item:getCustomAttribute("Hireling") == hirelingId then
 			return item
 		end
 	end

src/CMakeLists.txt

@@ -212,7 +212,9 @@ PRIVATE
 	items/decay/decay.cpp
 	items/item.cpp
 	items/items.cpp
-	items/functions/item_parse.cpp
+	items/functions/item/attribute.cpp
+	items/functions/item/custom_attribute.cpp
+	items/functions/item/item_parse.cpp
 	items/thing.cpp
 	items/tile.cpp
 	items/trashholder.cpp

src/creatures/combat/combat.cpp

@@ -53,7 +53,7 @@ CombatDamage Combat::getCombatDamage(Creature* creature, Creature* target) const
 					damage.secondary.type = weapon->getElementType();
 					damage.secondary.value = weapon->getElementDamage(player, target, tool);
 					if (params.useCharges) {
-						uint16_t charges = tool->getCharges();
+						auto charges = tool->getAttribute<uint16_t>(ItemAttribute_t::CHARGES);
 						if (charges != 0) {
 							g_game().transformItem(tool, tool->getID(), charges - 1);
 						}
@@ -727,7 +727,7 @@ void Combat::combatTileEffects(const SpectatorHashSet& spectators, Creature* cas
 
 		Item* item = Item::CreateItem(itemId);
 		if (caster) {
-			item->setOwner(caster->getID());
+			item->setAttribute(ItemAttribute_t::OWNER, caster->getID());
 		}
 
 		ReturnValue ret = g_game().internalAddItem(tile, item);
@@ -1207,7 +1207,7 @@ void ValueCallback::getMinMaxValues(Player* player, CombatDamage& damage, bool u
 				}
 
 				if (useCharges) {
-					uint16_t charges = tool->getCharges();
+					auto charges = tool->getAttribute<uint16_t>(ItemAttribute_t::CHARGES);
 					if (charges != 0) {
 						g_game().transformItem(tool, tool->getID(), charges - 1);
 					}
@@ -1649,7 +1649,7 @@ void MagicField::onStepInField(Creature& creature)
 	const ItemType& it = items[getID()];
 	if (it.conditionDamage) {
 		Condition* conditionCopy = it.conditionDamage->clone();
-		uint32_t ownerId = getOwner();
+		auto ownerId = getAttribute<uint32_t>(ItemAttribute_t::OWNER);
 		if (ownerId) {
 			bool harmfulField = true;
 

src/creatures/monsters/monster.cpp

@@ -1150,6 +1150,9 @@ bool Monster::pushItem(Item *item, const Direction& nextDirection)
 
 void Monster::pushItems(Tile *tile, const Direction& nextDirection)
 {
+	// We can not use iterators here since we can push the item to another tile
+	// which will invalidate the iterator.
+	// start from the end to minimize the amount of traffic
 	TileItemVector* items;
 	if (!(items = tile->getItemList())) {
 		return;
@@ -1160,7 +1163,7 @@ void Monster::pushItems(Tile *tile, const Direction& nextDirection)
 	while (it != items->end()) {
 		Item* item = *it;
 		if (item && item->hasProperty(CONST_PROP_MOVEABLE) && (item->hasProperty(CONST_PROP_BLOCKPATH)
-				|| item->hasProperty(CONST_PROP_BLOCKSOLID)) && item->getActionId() != 100 /* non-moveable action*/) {
+				|| item->hasProperty(CONST_PROP_BLOCKSOLID)) && item->getAttribute<uint16_t>(ItemAttribute_t::ACTIONID) != 100 /* non-moveable action*/) {
 			if (moveCount < 20 && pushItem(item, nextDirection)) {
 				++moveCount;
 			} else if (!item->isCorpse() && g_game().internalRemoveItem(item) == RETURNVALUE_NOERROR) {
@@ -1927,11 +1930,11 @@ Item* Monster::getCorpse(Creature* lastHitCreature, Creature* mostDamageCreature
 	if (corpse) {
 		if (mostDamageCreature) {
 			if (mostDamageCreature->getPlayer()) {
-				corpse->setCorpseOwner(mostDamageCreature->getID());
+				corpse->setAttribute(ItemAttribute_t::CORPSEOWNER, mostDamageCreature->getID());
 			} else {
 				const Creature* mostDamageCreatureMaster = mostDamageCreature->getMaster();
 				if (mostDamageCreatureMaster && mostDamageCreatureMaster->getPlayer()) {
-					corpse->setCorpseOwner(mostDamageCreatureMaster->getID());
+					corpse->setAttribute(ItemAttribute_t::CORPSEOWNER, mostDamageCreatureMaster->getID());
 				}
 			}
 		}