Info

• Server version: Paper version git-Paper-"591b2777" (MC: 1.12.2) (Implementing API version 1.12.2-R0.1-SNAPSHOT)
• OldCombatMechanics version: 1.6.6
• WorldGuard version 6.2.2-SNAPSHOT;63edfbf
• Server log file: N/A
• OldCombatMechanics config file: https://pastebin.com/ve8K3umT

Problem Description

Fishing rods can be used even when pvp is turned of in regions.

Sorry I had forgotten about this but I have now merged your commit @I-Al-Istannen. @iMaarten please check if the issues are still there.

Please try out the latest development build and let us know if this has solved your issue. Thanks :)

Is the PVP disabled in the area due to WorldGuard? If not, what plugin is disabling the PVP in that area?

It is the WorldGuard pvp flag

Just checking whether the event is cancelled (or setting ignoreCancelled = true) on the onRodLand listener might be enough.

Shouldn't this be as simple as firing our own EntityDamageEvent, checking if it's cancelled, and if it's not cancelled then only then doing damage with the fishing rod?

I misread the name, they use DamageEntityEvent not EntityDamageEvent, which is their own one. I will have to dig deeper into that one.

diff --git a/src/main/java/gvlfm78/plugin/OldCombatMechanics/module/ModuleFishingKnockback.java b/src/main/java/gvlfm78/plugin/OldCombatMechanics/module/ModuleFishingKnockback.java
index 3dde337..02745f2 100644
--- a/src/main/java/gvlfm78/plugin/OldCombatMechanics/module/ModuleFishingKnockback.java
+++ b/src/main/java/gvlfm78/plugin/OldCombatMechanics/module/ModuleFishingKnockback.java
@@ -73,6 +73,10 @@ public class ModuleFishingKnockback extends Module {
 
         EntityDamageEvent event = makeEvent(rodder, player, damage);
         Bukkit.getPluginManager().callEvent(event);
+        if(event.isCancelled()) {
+            System.out.println("NO!");
+            return;
+        }
 
         if(module().getBoolean("checkCancelled") && event.isCancelled()){

Seems to be enough.

You already fire an event, you just ignore the result currently.

He said, while the next line showed it was just behind a config option... Smart guy

I'll do that now 😄

@iMaarten Wait, do you really have checkCancelled enabled in the config (under fishing)?

If yes, can you enable the debug mode of OCM in the config and see if it sends you any message in the chat, if you rod somebody in a no-pvp area?

https://i.imgur.com/8b3nMis.png
https://i.imgur.com/1q9X5P9.png

Okay, I see the problem. You have useEntityDamageEvent enabled in the config and this causes OCM to (rightfully) use just that. The problem is that the pure damage event does not contain information about the attacker, so worldguard thinks it is just some damage — not that another player rodded you, but more like walking into a cactus. Logically the pvp check doesn't do anything then, as it wasn't player induced damage.

Try setting this value to false and see if the error persists. If this causes errors with NCP, we will have to look into that.

That fixes the WorldGuard issue, however, Only some of the hits will actually register and the whole knockback seems a bit off
https://i.imgur.com/bc4qfOT.png <- Debug of being hit
https://www.youtube.com/watch?v=9WJYiSio0-k <- Video of hitting

IIRC checkCancelled is an option in the config because EssentialsX seemed to cancel the event about 3 out of 4 times it fired, for some reason I was not able to figure out.

The problems seems to indeed to be EssentialsX, knockback still seems about the same.

Hm. I can't reproduce it with EssentialsX. Do you need some special config settings?

Knockback is interesting though. Does this build attached here is closer to it? It's mostly equal to the knockback from 1.8.8 based on the decompiled minecraft code.
OCM.zip

Knockbac seems to work great now!
Here is my Essentials
EssentialsX.zip

Made a small error in not removing enough of the old knockback code. This one fixes a too high Y Velocity (actually a teleport up) which allowed for far too great upwards momentum.

OCM.zip

But I can not reproduce the issue:

You said it was EssentialsX, is there anything else that needs to be present for you to reproduce the bug?

Then I guess we'll have to bisect his list of plugins… Or do you have a better idea to find out what plugin cancelled it?

The chat was a bit too fast to see what plugins were listening when it was cancelled sadly.

@iMaarten
Can you post the full output of that (i.e. all plugins that are listening), then I can try to bisect it. Or you do that, by removing all plugins (on a test server), and see if it works. If yes, then some plugin prevents it.
In that case just throw all plugins back on and remove half of them, see if it works and then continue to halve the part with the problem.

EDIT: The knockback direction is dependent on the relative location of the two players to each other. I guess the vanilla behaviour is using the location of the bobber instead. Let me change that and provide a build with that, so you can see if that feels more natural.
OCM.zip

When I was revising this module and came across this issue it had been EssentialsX that was cancelling the event, however I don't know if that was a bug that has now been fixed or a conflict of sorts.

Can not find anything else interfering. My apologies for the late reply.

I'd love to fix that, but I can not reproduce it. EssentialsX by itself only ever cancels the EntityDamageEvent under a few special circumstances and nothing you should have had happen in that video.

So it might be another one of your plugins or some settings are different? I used your config though, which is weird.

Can you, @rayzr522 or @gvlfm78 or anybody else reproduce it?

Seems to be fixed in the latest dev build however getting this error when hitting players/mobs getting this error https://pastebin.com/HjXYsfb0

1.12:

    public Fish getHook() {
        return this.hookEntity;
    }

1.13:

    public FishHook getHook() {
        return this.hookEntity;
    }

I will have a look at it tomorrow if nobody else has taken it up by then. I am way too tired right now sadly.

I don't believe it should be an issue, as they are both implementations of Entity. Perhaps we just need to cast e.getHook() to Entity?

Any chance it can be fixed that players can basically fly up https://i.imgur.com/PWKd0R0.gifv

@iMaarten I have added a configurable fishing rod knockback cooldown which appears to have fixed the issue.

To @rayzr522 @I-Al-Istannen: however, I am not sure whether it will be an issue if the HashMap I used fills up with an entry for each player that has been on the server and hit somebody else with a fishing rod since the server start. Could become a problem for larger servers due to memory usage. Currently, the list is cleared only if the module is reloaded. Maybe scheduling tasks to clear it periodically? (If this is even an issue at all)

You could have just used Player#setNoDamageTicks to prevent further damage within the configured delay (it is how it normally works with damaging entities --- they are granted a small period of nearly invincibility). For this to work correctly you likely need to delay applying the noDamageTick value by a tick, so do that it in a runnable that runs on the next tick.

If you really only want to prevent rod damage, then that sounds like a way to do it, but adding a listener to clear it out on PlayerQuitEvent should be easy and help somewhat.
Another way would be using a google guava cache and to set the expiration to the one specified in the config (though then you'd need to handle config reloads).

A cleaning task would work too, but might be a bit overkill, if you just remove them in on player quit. A Long is larger than a long (object header and such garbage), but still not incredibly so, so I am not sure if it is worth it.

The cleanest solution would be using the noDamageTicks, though that applies to all kinds of damage and not just rod usage.

@iMaarten there is already an issue for that here: #139. I might have a look at it now.

@gvlfm78 The concept of noDamageTicks is old and exists for all kinds of damage. You could spam the rod when using the plugin, as the plugin did not check the noDamageTicks and directly called Player#damage, which doesn't care about them.

If you add a check for noDamageTicks in the listener, it will mirror the vanilla behavior and not allow spamming it:

// ....
if(player.getGameMode() == GameMode.CREATIVE) return;

// Check if cooldown time has elapsed
if(player.getNoDamageTicks() > player.getMaximumNoDamageTicks() / 2f){
    return;
}

You can see the diff here.

@I-Al-Istannen This begs the question, in 1.8, did getting hit with a fishing rod grant you a short time of invincibility from all weapons or did it only apply to fishing rods?

@gvlfm78 noDamageTicks is used with everything, including fishing rods. Sorry if that was not clear enough.

Minecraft sets the noDamageTicks automatically to LivingEntity#maxNoDamageTicks when the entity is damaged, so you should not need to set it yourself, if you want the default vanilla behaviour of 20 ticks.

@I-Al-Istannen I was asking whether noDamageTicks was used specifically with fishing rods in 1.8. If that is the case, then using noDamageTicks would be the best solution.

Furthermore, previously you stated that we should use Player#setNoDamageTicks, however your code diff simply compares the damage ticks but never sets them. Would that then still be sufficient?

How did you configure the island? I find the plugin's protection system to be a bit confusing, as I can not find a PVP flag in the settings.