Fix: Critical hits by monsters

[InVoidEcho]

Description

Simple fix, adding missing return to prevent monster logic from executing

based on fix suggestion:
#3545 (comment)

Behaviour

Actual

"When a player casts a spell/attack, the damage calculation incorrectly continues execution into the monster logic section, causing monster critical damage modifiers to be applied on top of player damage calculations. This results in significantly higher damage than intended."

Expected

Proper monster critical damage calculation

Fixes #issuenumber

#3600

Type of change

• Bug fix (non-breaking change which fixes an issue)

Test Configuration:

• Server Version: Canary - Version 3.2.1
• Client: 14.12
• Operating System: windows

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I checked the PR checks reports
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gabrielew]

Couldn't reproduce here with 2 players and attack giant spider with SD
Server Log.txt

[dudantas]

The change just converts

} else if (monster) { ... }

into

return;  // early-return for players
...
if (monster) { ... }

Because a Creature can’t be both player and monster at the same time, the old else if already guaranteed that the monster block ran only when the caster was not a player.

I tested the scenario described in #3600 but couldn’t reproduce any double-critical or inflated damage; numbers are identical before and after the patch.

So the change has no practical effect, it’s only a stylistic refactor (early-return vs. else if). If you’re seeing different damage, please share exact repro steps or a minimal test case.

[omagonheiro]

Couldn't reproduce here with 2 players and attack giant spider with SD Server Log.txt

---

As mentioned in #3545

How to Reproduce
Summon a Giant Spider near Player A
With Player B, attack Player A using a Sudden Death rune
Expected Result: Giant Spider should deal maximum 300 damage
Actual Result: Giant Spider deals 500+ damage (monster critical applied to player attack)

GS dealing 700+

[gabrielew]

Reproduced here too!

My bad, I was hitting the giant spider not the player

19:43 You lose 113 hitpoints due to an attack by a giant spider.
19:43 You lose 555 hitpoints due to an attack by Player Two.
19:43 You lose 19 hitpoints due to an attack by a giant spider.
19:43 You lose 1111 hitpoints due to an attack by a giant spider.
19:44 You lose 139 hitpoints due to an attack by a giant spider.
19:44 You lose 546 hitpoints due to an attack by Player Two.
19:44 You lose 141 hitpoints due to an attack by a giant spider.
19:44 You lose 1093 hitpoints due to an attack by a giant spider.
19:44 You lose 88 hitpoints due to an attack by a giant spider.

[gabrielew]

Couldn't reproduce here with 2 players and attack giant spider with SD Server Log.txt

As mentioned in #3545

How to Reproduce Summon a Giant Spider near Player A With Player B, attack Player A using a Sudden Death rune Expected Result: Giant Spider should deal maximum 300 damage Actual Result: Giant Spider deals 500+ damage (monster critical applied to player attack)

I think that the problems is not the critical damage, cuz the giant spider hits the player with death damage and giant spider doesn't have death damage as you can see below:

monster.attacks = {
	{ name = "melee", interval = 2000, chance = 100, minDamage = 0, maxDamage = -300, condition = { type = CONDITION_POISON, totalDamage = 160, interval = 4000 } },
	{ name = "poisonfield", interval = 2000, chance = 10, range = 7, radius = 1, shootEffect = CONST_ANI_POISON, target = true },
	{ name = "combat", interval = 2000, chance = 10, type = COMBAT_EARTHDAMAGE, minDamage = -40, maxDamage = -70, range = 7, radius = 1, shootEffect = CONST_ANI_POISON, target = true },
}

Maybe it's getting some "garbage" or "ref" from last player damage, as we can see the giant spider hit the player with death damage

[InVoidEcho]

Most important was missing "return;" previously in player logic.

Without it, the player-specific critical and fatal hit bonuses are applied first, but then the monster logic also executes afterward.
This causes the monster logic to re-apply multipliers and potentially overwrite or stack damage values unintentionally.

As a result, you may encounter:
damage.critical and damage.fatal flags being wrongly overwritten or misapplied.

[github-actions]

This PR is stale because it has been open 45 days with no activity.

[Ormezitol]

Still the same, I don't think it's the creature's critical hit, I disabled mine before running the test again and a giant spider attacks the same damage as the player, only bigger, in this case the sd

03:29 Test Dois loses 22688 hitpoints due to an attack by a giant spider.

2025-08-27.03-29-28.mp4

[gabrielew]

It is also happening on NO-PVP servers.

18:05:57 You lose 4130 mana due to an critical attack by an energuardian of tales.

[Mortera-world]

The change just converts

} else if (monster) { ... }

into

return;  // early-return for players
...
if (monster) { ... }

Because a Creature can’t be both player and monster at the same time, the old else if already guaranteed that the monster block ran only when the caster was not a player.

I tested the scenario described in #3600 but couldn’t reproduce any double-critical or inflated damage; numbers are identical before and after the patch.

So the change has no practical effect, it’s only a stylistic refactor (early-return vs. else if). If you’re seeing different damage, please share exact repro steps or a minimal test case.

I have the same problem, it reproduces, with a knight or any vocation you stay blocking a creature and with another character you start hitting the one who blocked the creature with sd, or spells and you will notice that the creature replicates and increases the damage caused by the other character

[Mortera-world]

Most important was missing "return;" previously in player logic.

Without it, the player-specific critical and fatal hit bonuses are applied first, but then the monster logic also executes afterward. This causes the monster logic to re-apply multipliers and potentially overwrite or stack damage values unintentionally.

As a result, you may encounter: damage.critical and damage.fatal flags being wrongly overwritten or misapplied.

According to ChatGPT

Creatures should use their own Combat Damage when attacking.
When they do not have an assigned Combat Damage, the code uses the last TmpDamage generated by a player and applies it to the creature.
This causes the creature's damage to be much higher than expected, or equal to the player's damage.

Key Technical Issue
Monsters attacking without a defined Combat Damage enter a code flow where the last player damage is applied to them.
There is no base or default damage assignment for these creatures, so they rely on the floating value in TmpDamage.

Consequence
Players and monsters do not deal damage independently.
Player attacks can cause creatures to hit harder than intended, resulting in uncontrolled damage and inconsistent combat outcomes.

In the CombatFunc function, when applying damage (func), tmpDamage is used if creature->getCombatDamage() is empty.
Monsters don't have their own CombatDamage; that's why they inherited tmpDamage from the player's last attack.
This causes creatures to deal absurd damage, copying player attacks.

[github-actions]

This PR is stale because it has been open 45 days with no activity.

[lBaah]

https://github.com/opentibiabr/canary/blob/main/src/creatures/combat/combat.cpp#L2410
this damage will be stored and never reseted, it's applied immediately as intended, then stored with setCombatDamage and will be used next time an area spell is casted, causing the damage values to stack and get multiplied as you see in your serverlog

https://github.com/opentibiabr/canary/blob/main/src/creatures/combat/combat.cpp#L1325
try this instead

			if (isSingleCombat) {
				damage = targetDamage;
			} else {
			    targetCreature->setCombatDamage(targetDamage);
			}

[dudantas]

Resolved here: #3800