Ignore cancelled events for compatibility with protection plugins

[NewbieOrange]

Make event listeners ignore events already cancelled by other plugins. (e.g. protection plugins like Residence or griefprevention)

[AtriusX]

This seems like a good idea, though I noticed you reverted the changes for this relating to PlayerInteractEvent. Did changing that event cause things to break?

[AtriusX]

@Sepshun is also testing this currently.

[luna-skye]

Tested with the two largest protection plugins I currently have, being WorldGuard v7.0.4 GriefPrevention v16.16.0

While this works well for DestroyEvent, and its BlockBreakEvent listener, it doesn't seem to be working for any of the PlayerInteractEvent listeners, being mostly the Link, Name, and Info Events. It seems to be due to neither WorldGuard nor GriefPrevention cancelling that particular event, regardless of flags set through WorldGuard. This could be an issue with my config, though it's all standard and basic prevention flags being set through WorldGuard, so I'm unsure.

I'll be able to test further and dig into it a bit later today, but as of right now only the destruction of links are being properly prevented.

[NewbieOrange]

This seems like a good idea, though I noticed you reverted the changes for this relating to PlayerInteractEvent. Did changing that event cause things to break?

The PlayerInteractEvent is fired as cancelled when the block is air.

[AtriusX]

Ah, so it was effectively redundant in this case then?

[NewbieOrange]

Well - if we ignore cancelled PlayerInteractEvent, then we will never get anything if the player interacts with air.

In this case, if we want other plugins to control the behaviour, we need to fire our own events and hope others listen to them (or hook into other plugins).

[AtriusX]

Ah ok. I've been debating on whether this approach is better or if having first/second-class integration support for protection plugins would be preferable. One of the big problems I'm aware of when it comes to just filtering out cancelled events is needing to ensure your plugin is the last one in line to catch the event (something we cant exactly guarantee).

[NewbieOrange]

The protection plugins tends to listen as HIGH (or higher) priority, which will be earlier called than our NORMAL priority listeners.

AFAIK that is mostly a standard (listener priority higher than NORMAL) for protection plugins.

[AtriusX]

I heard that too, though to my understanding I was under the impression that the higher priority levels would execute after the lower priority ones (so that they would get more information). But I could be wrong about that.

[NewbieOrange]

You are correct, they should be listening on LOW or even LOWEST priority.

Code snippet from GriefPrevention

    //when a player breaks a block...
    @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
    public void onBlockBreak(BlockBreakEvent breakEvent)
    {
        ... 
    }

So they will be called earlier than us.

[AtriusX]

This wouldn't work for worldguard though as I believe they have their priority listed as EventPriority.HIGH

[NewbieOrange]

Not sure why worldguard did that... EngineHub/WorldGuard#274 EngineHub/WorldGuard#804

[AtriusX]

From what it looks like the change was not implemented either, so that's a bit of a problem...

[AtriusX]

On second thought, went to check the code directly and it appears they're using EventPriority.LOW at least in the file mentioned in the PR.

[NewbieOrange]

From what i've seen, other plugins usually hook into wg to workaround this issue.

tbh it seems to be more of worldguard's issue...

[luna-skye]

According to this comment, it seems they did switch to using NORMAL a while back https://github.com/EngineHub/WorldGuard/issues/804#issuecomment-598205924

[AtriusX]

Digging around the project shows that it seems to be more of a mishmash of different priority levels depending on the event.

[NewbieOrange]

Both GP and Residence listen at LOWEST, and Bukkit doc1 suggested LOWEST for protection plugins.

Worldguard may need special treatment (hook into their api) for compatibility.

[AtriusX]

Figures. Might be better to make another PR specifically for focusing on creating an integration system and custom events.

[NewbieOrange]

Actually, for the events that we do not cancel - we might listen on MONITOR.

[AtriusX]

I guess, but will that really affect things that much?

[NewbieOrange]

MONITORs are always called last, so the blockbreakevent listener should be compatible with worldguard.

[AtriusX]

Alright, @Sepshun would you mind testing the changes?

[luna-skye]

The BlockBreakEvent is indeed compatible with WorldGuard now. So now the question is, does WorldGuard, under any flag, prevent PlayerInteractEvent in a generic enough manner to apply to Waystones without hooking into their API?

I'm failing to find the relevant code in the the WorldGuard source, but I've looked through the flag documentation and denied all that I imagined could impact PlayerInteractEvent, being use, interact, respawn-anchors, chest-access, and even lighter as a last ditch effort None of which prevent linking, renaming, querying info, or warping.

[AtriusX]

Glad to see BlockBreakEvent is functioning properly now at least.

[AtriusX]

Any ideas for what can be done to fix the problems with PlayerInteractEvent?

[NewbieOrange]

Most protection plugins do not care about lodestone interactions anyway so theres not much we can do for now, other than hooking into each plugin's api.

[AtriusX]

Figures. I don't think there is really a huge issue overall with PlayerInteractEvent not functioning as expected, though the possibility of someone griefing a waystone by changing its name is something we'd have to worry about.

[luna-skye]

My concern has mainly been preventing linking within specific worlds, such as the End or hubs/lobbies. As well as preventing non-trusted players from linking to Waystones within player claims through things like GriefPrevention.

[AtriusX]

I believe we already established permissions to loosely patch that problem though, did we not?

[luna-skye]

Yeah, but I feel that's a little more of a workaround. It's limiting to only specific contexts and only permission plugins that support contexts, like LuckPerms. Even with LuckPerms, it doesn't allow regions or subregions to be defined and blocked.

[AtriusX]

Then we're basically going to have to write a hook system for plugins then. I suppose this wouldn't be a bad idea to start discussing an API though, given the suggestions #19 and #20.

[AtriusX]

@Sepshun @NewbieOrange Whats the status on this PR? We obviously can't do anything about PlayerInteractEvent at the moment since it likely would involve a more integrated system which is better suited for another PR. Should we just merge this one as is?

[NewbieOrange]

I think this can be merged now. Not much can be done with PlayerInteractEvent without hooking other plugins.

[AtriusX]

Alright, I'll merge the PR now then. Any discussion related to the integration system should be directed to #22 from here onward.