fix Mobs can drop items without ownership on big and long fights

[MysteriousDev 9]

There is a very old bug, most known on servers where players can deal absurd amount of damage or there is a world boss and many players can join the fight.
So the issue is that mobs can drop items without ownership, and if there is a lot of players the drop just instantly disappears cause everyone is spamming the pick up key.

How is it possible?
The main issue is in CHARACTER::Reward (check comments):

			std::priority_queue<std::pair<int, LPCHARACTER> > pq;
			
			int total_dam = 0;

			for (TDamageMap::iterator it = m_map_kDamage.begin(); it != m_map_kDamage.end(); ++it)
			{
				int iDamage = it->second.iTotalDamage;
				if (iDamage > 0)
				{
					LPCHARACTER ch = CHARACTER_MANAGER::instance().Find(it->first);

					if (ch)
					{
						pq.push(std::make_pair(iDamage, ch));
						// first issue here
						total_dam += iDamage;
					}
				}
			}

			std::vector<LPCHARACTER> v;
			// second one here
			while (!pq.empty() && pq.top().first * 10 >= total_dam)
			{
				v.emplace_back(pq.top().second);
				pq.pop();
			}

so when the total damage (int total_dam) of all players exceeds INT_MAX, guess what happens? It overflows to values below 0.

Another risk is that pq.top().first * 10 will exceed the INT_MAX if the fight is long enough and the player has really good DPS.

How do we fix it? It's trivial easy:

@@ -812,7 +812,7 @@ void CHARACTER::Reward(bool bItemDrop)
 
 			std::priority_queue<std::pair<int, LPCHARACTER> > pq;
 
-			int total_dam = 0;
+			long long total_dam = 0;
 
 			for (TDamageMap::iterator it = m_map_kDamage.begin(); it != m_map_kDamage.end(); ++it)
 			{
@@ -830,8 +830,7 @@ void CHARACTER::Reward(bool bItemDrop)
 			}
 
 			std::vector<LPCHARACTER> v;
-
-			while (!pq.empty() && pq.top().first * 10 >= total_dam)
+			while (!pq.empty() && static_cast<long long>(pq.top().first) * 10 >= total_dam)
 			{
 				v.emplace_back(pq.top().second);
 				pq.pop();
-- 

 

The topic covers the case which can occur on base Metin2. There is nearly 0% chance to reach INT_MAX in total damage per Mob as a normal player.
However, 30 players fighting with world boss are able to exceed the INT limit.

If you assume that a player himself is able to exceed INT_MAX in total damage per mob you should consider changing the type of iTotalDamage in TBattleInfo struct.
This of course comes with more editions, wherever the damage map is changed or read.

Edited October 3, 2024 by MysteriousDev

[Endymion 366]

According to your worldbosses example, if a large number of players fight it out there is a chance that no player will reach the required 10% with total_dam and the items will also be ownerless.

[MysteriousDev 9]

22 minutes ago, Endymion said:

According to your worldbosses example, if a large number of players fight it out there is a chance that no player will reach the required 10% with total_dam and the items will also be ownerless.

I guess the original thought behind this was to not reward a player that has dealt only 1 hit to the boss/metin.

The best way to fix that is to rewrite the item drop to make it prioritize the damage dealt with no artificial thresholds. This would reward the best player with the best DPS. But it makes a gameplay impact.

Or just make an exception for a world boss to not account the 10% range.

Edited October 4, 2024 by MysteriousDev

[Kidro 160]

i fixed it like this:

 

void CHARACTER::Reward(bool bItemDrop)

 

under BYTE bMulPct = 10;

 

            if (IsBoss())
                bMulPct = 100;

 

and works perfectly

[betsch96 9]

 

That also fixes the problem for everyone using mobs with like crazy amounts of HP

Edited October 19, 2024 by Metin2 Dev International
Core X - External 2 Internal

[Owsap 10675]

I've been curious about this and I've spent a little time debunking if player damages really get overflowed and guess what I've discovered, it's a yes and no.

Overflow?

Yes, int overflow is technically possible, but only under extremely unrealistic conditions, and these are:

• A monster with 2B+ HP and/or with health regeneration.
• A player dealing 10M+ damage per hit.

In theory, int is enough. In practice, overflow will never occur under normal server settings.
Once the monster dies, the damage map is instantly cleared, so no lingering accumulation.

On official servers:

• The highest health monsters are the "World Boss" event mobs, with 50M health points.
• They have no health regeneration.
• Player damages aren't unrealistic.

Therefore, the total sum of all player damage will always be close to the monster's health points (no matter if 1 or 50 players are attacking it).
As long as the monster's health points stay under 2B, you are safe using int.

That said, for safety and future-proofing, I recommend changing only this:

int total_dam -> int64_t total_dam

If you also want to make the changes that you mentioned they're valid and adapted for unusually high single-player damage values.

Item Ownership Logic

The 10% threshold doesn't scale when many players are involved or when the monster regenerates a lot. It's not a bug, just a flawed design assumption.

while (!pq.empty() && pq.top().first * 10 >= total_dam)

Suggested Fix for Ownership

Instead of individual 10% checks, you can make all players whose combined damage >= X% of total damage eligible, like so:

accumulated_damage * 100 < total_damage * X

Example: use X = 30 to allow the top damage dealers up to 30% of total damage but I would leave it at 10.

What I Tested

I ran simulations with:

• 1 player, up-to 50 players.
• 2B+ monster health points.
• 10M+ player damage per hit.
• Monster regenerating every few cycles.
• 10K+ Simulations.

The tests confirm that overflow on player or total damage is possible, but only under unrealistic conditions...

Spoiler

// Simulate Metin2 damage priority logic and item drop ownership.
// Author: Owsap

// In response to:
// https://metin2.dev/topic/33063-mobs-can-drop-items-without-ownership-on-big-and-long-fights/#comment-167585

#include <iostream>
#include <map>
#include <random>
#include <numeric>
#include <queue> // includes <vector>
#include <cstdarg>

// Simulation of a monster fight with multiple players.
// If the monster regenerates HP, the total combined damage
// can exceed INT_MAX. So, int64_t is used only for total_dam to future-proof.
#define SIMULATION 1 // 10000

// Enables accumulated damage-based ownership eligibility logic.
#define __ACCUMULATED_OWNERSHIP_LOGIC__

// Enables boss regeneration simulation.
#define __REGEN_MONSTER__

constexpr int MONSTER_HP = 50'000'000; // Boss max health points (e.g. Bagjanamu)
constexpr int PLAYER_COUNT = 30; // Number of players
constexpr int MIN_DAMAGE = 30'000; // Minimum damage per hit
constexpr int MAX_DAMAGE = 80'000; // Maximum damage per hit

#ifdef __REGEN_MONSTER__
// Not 100% accurate to real regen behavior, but simulates the core idea.
constexpr int REGEN_INTERVAL = 6;
constexpr int REGEN_PERCENT = 10;
#endif

void log(const char* fmt, ...)
{
	va_list args;
	va_start(args, fmt);
	vprintf(fmt, args);
	printf("\n");
	va_end(args);
}

int random_between(int min, int max)
{
	static std::random_device rd;
	static std::mt19937 gen(rd());
	std::uniform_int_distribution<> dist(min, max);
	return dist(gen);
}

struct BattleInfo
{
	int total_damage = 0; // used to calculate who is eligible for item ownership.
	int aggro = 0; // used to calculate who the monster should target based on damage and special attacks.
};

std::map<int, BattleInfo> g_damage_map;
std::map<int, int> g_pids;

void generate_players()
{
	std::vector<int> ids(PLAYER_COUNT);
	std::iota(ids.begin(), ids.end(), 0);
	std::shuffle(ids.begin(), ids.end(), std::mt19937{ std::random_device{}() });

	for (int i = 0; i < PLAYER_COUNT; ++i)
		g_pids[i] = ids[i];
}

int main()
{
	int total_damage_overflow_detected = 0;
	int total_player_damage_overflow_detected = 0;

	for (int run = 1; run <= SIMULATION; ++run)
	{
		log("\n=== Simulation Run #%d ===", run);

		generate_players();

		std::vector<int> hits_per_player(PLAYER_COUNT, 0);
		int boss_hp = MONSTER_HP;
		int total_hits = 0;

#ifdef __REGEN_MONSTER__
		int loops = 0;
		int regens = 0;
#endif

		while (boss_hp > 0)
		{
#ifdef __REGEN_MONSTER__
			++loops;
#endif
			++total_hits;

			int players = random_between(1, PLAYER_COUNT);
			for (int i = 0; i < players; ++i)
			{
				int pid = g_pids[i];

				int dam = random_between(MIN_DAMAGE, MAX_DAMAGE);
				if (dam > 0)
				{
					if (g_damage_map[pid].total_damage > INT_MAX - dam)
					{
						log(" ~ player %d total damage would overflow (current = %d, next hit = %d)", pid, g_damage_map[pid].total_damage, dam);
						++total_player_damage_overflow_detected;
						break;
					}

					g_damage_map[pid].total_damage += dam;

					++hits_per_player[pid];

					boss_hp -= dam;
					if (boss_hp <= 0)
						break;
				}
			}

#ifdef __REGEN_MONSTER__
			if (REGEN_INTERVAL > 0 && loops % REGEN_INTERVAL == 0 && boss_hp > 0)
			{
				++regens;
				int regen_amount = MONSTER_HP * REGEN_PERCENT / 100;
				boss_hp += regen_amount;
				if (boss_hp > MONSTER_HP)
					boss_hp = MONSTER_HP;
			}
#endif
		}

		std::priority_queue<std::pair<int, int>> pq;
		int64_t total_dam = 0; // int -> int64_t

		for (auto it = g_damage_map.begin(); it != g_damage_map.end(); ++it)
		{
			int dam = it->second.total_damage;
			if (dam > 0)
			{
				auto pid_it = g_pids.find(it->first);
				if (pid_it != g_pids.end())
				{
					pq.emplace(dam, pid_it->first);
					total_dam += dam;
				}
			}
		}

		log("fight ended in %d hits.", total_hits);
#ifdef __REGEN_MONSTER__
		log("boss regened %d times", regens);
#endif
		log("total damage dealt: %lld", total_dam);
		if (total_dam > INT_MAX)
		{
			++total_damage_overflow_detected;
			log(" ~ total_dam exceeded INT_MAX!");
		}

		log("damage per player:");

		for (const auto& it : g_damage_map)
		{
			log(" - player %d: %d damage in %d hits",
				it.first, it.second.total_damage, hits_per_player[it.first]);
		}

		std::vector<int> v;
#ifdef __ACCUMULATED_OWNERSHIP_LOGIC__
		int64_t accumulated_dam = 0;
#endif

#ifdef __ACCUMULATED_OWNERSHIP_LOGIC__
		while (!pq.empty() && accumulated_dam * 100 < total_dam * 10)
		{
			accumulated_dam += pq.top().first;
			v.push_back(pq.top().second);
			pq.pop();
		}
#else
		// Classic logic: any player dealing >= 10% of total damage qualifies
		while (!pq.empty() && pq.top().first * 10 >= total_dam)
		{
			v.push_back(pq.top().second);
			pq.pop();
		}
#endif

		if (v.empty())
		{
			log("\nitem drop, no ownership!!!");
		}
		else
		{
			log("\nitem drop, ownership (dice rolling...)");
			for (int pid : v)
				log(" - player %d eligble for ownership", pid);
		}

		if (total_player_damage_overflow_detected)
			break;

		g_damage_map.clear();
	}

	if (total_damage_overflow_detected)
		log("!!! total damage exceeded INT_MAX %d times !!!",
			total_damage_overflow_detected);

	if (total_player_damage_overflow_detected)
		log("!!! player total damage exceeded INT_MAX % times !!!",
			total_player_damage_overflow_detected);

	return 0;
}

 

 

Edited Saturday at 12:34 AM by Owsap

[Endymion 366]

All that is really needed for overflow to occur is enabled regen in the monster and some time.
On servers where people fight over bosses even for several hours at a time, achieving INT_MAX damage is not that unrealistic at all.

Just changing “int total_dam -> int64_t total_dam” is not enough in such a case, since one player can reach INT_MAX damage for several hours.
It would also be good to take a look at char.h:TBattleInfo.

[Owsap 10675]

1 hour ago, Endymion said:

All that is really needed for overflow to occur is enabled regen in the monster and some time.
On servers where people fight over bosses even for several hours at a time, achieving INT_MAX damage is not that unrealistic at all.

Just changing “int total_dam -> int64_t total_dam” is not enough in such a case, since one player can reach INT_MAX damage for several hours.
It would also be good to take a look at char.h:TBattleInfo.

For a player to accumulate enough damage to overflow a int32 without killing a boss with 50M health points, the boss must regenerate more than 32.4% of its health per minute (that is already too much), that is, over 16.2M health points per minute.

Under these conditions, the player can keep dealing damage indefinitely without ever defeating the boss, eventually reaching the 2,147,483,647 damage cap. At 90k damage per hit and 3 hits per second, this would take around 2 hours of nonstop combat.

Note that the lower the damage output, the longer it will take to reach the overflow point. While this is technically possible, it’s practically unrealistic, any sane player would have quit long before, seeing no progress against the boss.

Additionally, if multiple players are involved, each one tracks their own damage separately. This means each player can independently reach their own overflow, as long as the boss regenerates enough to prevent death. For example, with two players doing identical damage, the boss would need to regenerate more than 64.8% of its health points per minute to remain unkillable, but each player would still take the same ~2h to hit the int32 limit individually.

I'm not saying it's impossible, I'm just giving my own observation, but of course it's better to change all int32 to int64_t for such conditions. 

Edited Saturday at 11:34 PM by Owsap

[Endymion 366]

I understand how it works and have experienced this error myself. Keeping PvP in mind, you have to count sometimes bigger breaks for beating a boss than the constant “3 hits per sec” by X players. You have to add in running with the boss by the top aggr player in order to disturb others.

In my case, after 5 hours of fighting for the boss core just went down. If I remember correctly, the source of the crash was the CHARACTER::DistributeExp().

I understand very well that you need to complete several requirements for this bug to occur, but in a natural environment it is not as unrealistic as it seems (or I should just take part in some lottery).