search

pmmp / PocketMine-MP

A server software for Minecraft: Bedrock Edition in PHP
https://pmmp.io
GNU Lesser General Public License v3.0
3.28k stars 1.56k forks source link

EntityDamageEvent Attack cooldown bug #3997

Open xLordShadow opened 3 years ago

xLordShadow commented 3 years ago

Issue description

(This is my first time submitting an bug report, let me know if i messed it up somehow)

Steps to reproduce the issue

  1. Create a plugin with a listener for the EntityDamageEvent
  2. Modify the base damage in anyway (Shown in the gist at the bottom)

OS and versions

Plugins

Plugins (2): AttackTest v0.0.1, DevTools v1.13.0

Crashdump, backtrace or other files

This gist shows the code i used to recreate the issue: https://gist.github.com/xLordShadow/a3a29c1192340522a5fd1a5533f2fc77

dktapps commented 3 years ago

This is most likely happening because the total damage adds up to less than zero because of the base damage change.

xLordShadow commented 3 years ago

This is most likely happening because the total damage adds up to less than zero because of the base damage change.

Does it make it not a bug then? I do notice it happens only when reducing the amount of damage they take

dktapps commented 3 years ago

I don't remember the specifics of this code so it'll have to wait until someone has time to investigate it.

TheNewHEROBRINEX commented 3 years ago

I think this is caused by this if https://github.com/pmmp/PocketMine-MP/blob/da71540fcedec6cdfdb3931802c3612e60ad3589/src/pocketmine/entity/Living.php#L446-L448 Cancelling the event will result in the cooldown not being updated. This happens because on the left side of the comparison we have the base damage of the previous event ~BEFORE~ AFTER the plugin reduced it while on the right side we have the base damage of the current event ~AFTER~ BEFORE it has been reduced, therefore this check always yields ~true~ false when reducing the base damage. Anyway, I don't know what game mechanic this code pretends to implement so I can't say what this should be changed to.

dktapps commented 3 years ago

I don't think this is a bug. The purpose of the cooldown is to prevent excessive damage being done by multiple attacks within a short space of time. Since reducing the base damage causes the damage to be ignored within that short time frame, the cooldown will not be reset and no damage will be done anyway.

TheNewHEROBRINEX commented 3 years ago

But animation, sounds, knockback will still apply and this way you can knock the player very farther away than usual

dktapps commented 3 years ago

if that's the case, it's not that code that's causing this problem, because otherwise none of that would happen.

dktapps commented 3 years ago

in addition, this code is executed before any event handler has a chance to interfere with the result.

TheNewHEROBRINEX commented 3 years ago

This happens because on the left side of the comparison we have the base damage of the previous event BEFORE the plugin reduced it while on the right side we have the base damage of the current event AFTER it has been reduced, therefore this check always yields true when reducing the base damage.

Sorry, I messed up what I wanted to say here. I meant the following: lastDamageCause holds the previous event after the plugin has reduced the base damage since lastDamageCause gets saved after the event had been called: https://github.com/pmmp/PocketMine-MP/blob/da71540fcedec6cdfdb3931802c3612e60ad3589/src/pocketmine/entity/Entity.php#L932-L937 On the other side of the comparison, getBaseDamage() is called on the event object before the event had been called as it can be seen here (applyDamageModifiers is the method were the indicted code is, parent::attack is the method in which the event gets called): https://github.com/pmmp/PocketMine-MP/blob/da71540fcedec6cdfdb3931802c3612e60ad3589/src/pocketmine/entity/Living.php#L540-L552 so the base damaged is queried before the plugin could reduce it and therefore the check is always ~true~ false in these circumstances, leading to the cooldown not being reset: https://github.com/pmmp/PocketMine-MP/blob/da71540fcedec6cdfdb3931802c3612e60ad3589/src/pocketmine/entity/Living.php#L554-L558

I myself experienced this issue in the past and I can recall that the player attacked was subject to a higher knockback, probably derived from the sum of normal knockbacks being applied in a shorter frame of time. Now I can't tell why exactly this would happen since event cancellation due to that check should also prevent knockback from being applied: https://github.com/pmmp/PocketMine-MP/blob/da71540fcedec6cdfdb3931802c3612e60ad3589/src/pocketmine/entity/Living.php#L554-L573

dktapps commented 3 years ago

That still doesn't make any sense. If the cooldown is not reset, nor will knockback apply (they are both gated by the same check).

TheNewHEROBRINEX commented 3 years ago

Ok, I think I figured it out. I was wrong, the check is not always true but always false because it checks damage-after-reduction >= damage-before-reduction. Due to that check, the event is cancelled only if the newer damage is smaller than the previous one. Damage is nullified by the modifier in any case if attackTime > 0. So when reducing the base damage via a plugin, the cooldown still does damage prevention but does not prevent knockback, animation and sound as well.

TheNewHEROBRINEX commented 3 years ago

The check is not aware yet that a plugin is going to reduce the final damage so maybe this check should be moved after the event call? Edit: nvm, plugins wouldn't be able to uncancel the event this way.

TheNewHEROBRINEX commented 3 years ago

I found the game mechanic which is being implemented by that code, quoting from https://minecraft.gamepedia.com/Damage#Immunity:

If an entity is in the immunity period and then receives higher damage (before accounting for armor, enchantments, or status effects), the difference between the original and new damage amounts is dealt. For example, if a mob is attacked with a weapon dealing 7 damage and then attacked with another weapon dealing 12 damage during the immunity period, the second hit deals 5 damage (resulting in 12 total). This does not reset the immunity period.

~However, it does not explicitly say whether or not knockback should still apply for damages dealt during attack cooldown.~

TheNewHEROBRINEX commented 3 years ago

Perhaps this could be solved by adding a boolean property to EntityDamageEvent called "ignoreCooldown" so that we can move the check after the event call and account for the plugin modified base damage while still allowing plugins to prevent event cancellation due to the cooldown.

TheNewHEROBRINEX commented 3 years ago

Another approach to solving this issue would be to use lastDamageCause->getOriginalBaseDamage() in the check instead of getBaseDamage() so that plugin modifications are not taken into account. Either way you should compare the base damages taking both their values before plugins have modified them or both after plugins have had the chance to modify them.

dktapps commented 1 year ago

To summarize this issue:

Halving the base damage of event n.1 (e.g. 4 -> 2) causes event n.2 to not be cancelled with the same damage amount (4), since the base damage before plugin modification is higher than the previous event due to not having been modified by any event handlers yet.

By the time event n.2 is actually processed, it will also have a base damage of 2, and an additional MODIFIER_ATTACK_COOLDOWN of -2, which would cause the total to add up to 0. However, the knockback and post-damage effects would still take place in this case.

Possible solutions: