Slimefun

Slimefun

3M Downloads

Cancelling Bukkit's PlayerInteractEvent does not stop Slimefun's interaction

bloodmc opened this issue ยท 3 comments

commented

๐Ÿ“ Description (REQUIRED)

Hi, I'm the developer of GriefDefender and found a bug within Slimefun ignoring interaction events when GD cancels. During PlayerInteractEvent, Slimefun absorbs the Bukkit event to fire its own event. This would be fine if the Bukkit event result was copied to the new event but it is not. This violates Bukkit's event contract and should be fixed.
Relevant code here

PlayerRightClickEvent event = new PlayerRightClickEvent(e);
Bukkit.getPluginManager().callEvent(event);
boolean itemUsed = e.getHand() == EquipmentSlot.OFF_HAND;
if (event.useItem() != Result.DENY) {
rightClickItem(e, event, itemUsed);
}

The fix is to either add ignoreCancelled = true to the annotation or copy the bukkit event result to the new event.

๐Ÿ“‘ Steps to reproduce the Issue (REQUIRED)

  1. Force cancel Bukkit's PlayerInteractEvent
  2. Right-click on a block such as Electronic Furnace.
  3. GUI opens

๐Ÿ’ก Expected behavior (REQUIRED)

After cancelling PlayerInteractEvent, the right-click action should not occur.

๐Ÿ“œ Server Log

Not relevant.

๐Ÿ“‚ /error-reports/ Folder

N/A

๐Ÿงญ Environment (REQUIRED)

  • Server Software (Spigot/Paper): Paper build 251
  • Minecraft Version: 1.16.3
  • Slimefun Version: Latest dev build 734
  • CS-CoreLib Version: CS-CoreLib - DEV 90 (git 9dd4a)
commented

I don't think I have ever heard of GriefDefender, but cool. Nice to meet you.
Slimefun probably doesn't even support GD as of now, so if you could add a module to our parent library CS-CoreLib for hooking into GD that would be very nice, it would allow us to make automatic building or block breaking machines adhere to your protective boundaries.
You can find more info about that here:
https://github.com/Slimefun/Slimefun4/wiki/Protection-plugins

Other than that, I will look into this inconsistency at some point.
I just don't really have too much time to work on things right now as I just got back from the hospital yesterday and I'm still recovering from an operation.
Unless another project member decides to make a pull request, it would be pretty nice if you could maybe propose these changes in form of a PR to discuss what the best approach would be. Perhaps someone from the team could also conduct some ingame testing already.
Anyway, thanks for bringing this up!

commented

Hi! Likewise!

So about your protection modules...
GD has its own built-in support for Slimefun as seen here

https://github.com/bloodmc/GriefDefender/blob/master/bukkit/src/main/java/com/griefdefender/provider/SlimefunProvider.java

GD's protection is heavily reliant on events and contexts which are linked to flags. So when a specific bukkit event is triggered, GD will associate the event with a flag and provide the event details such as block id, item id, player, location, etc.. This is one of the main reasons why I decided to write a custom provider for Slimefun as the protection would be limited if support was added to your side.

Here is an example of GD's debug with slimefun usage
https://griefdefender.github.io/debug/?TqlQJRvRaP

Each action can be protected for a specific user/group/global.

As for the issue, I can definitely PR the fix I have done on my side for my users. I just wasn't sure if Slimefun was ignoring cancelled events for specific functionality. I'll let users test a fix for about a week and if I hear no issues then I'll PR it.

Again, thanks for the response. Hope you get better soon!

commented

@bloodmc The issue should have been patched now.
The corresponding Result is now copied into the PlayerRightClickEvent.

Sorry that it took so long.
I hope that this change doesn't have any unintended side effects but only time will tell...
Given Bukkit's history with lots of plugins picking their event priorities almost arbitrarily (with this project certainly being no exception in the past...), I I could see a few issues popping up in that regard here and there. But we will see, the underyling issue you reported should have been fixed now.