search

BG-Software-LLC / WildStacker

Stacked drops, stacked entities, stacked blocks and stacked spawners in one plugin!
https://bg-software.com/wildstacker/
GNU General Public License v3.0
92 stars 38 forks source link

WildStacker Conflict with Towny #371

Closed skw359 closed 2 years ago

skw359 commented 2 years ago

Minecraft's Version

git-Purpur-1426

Plugin's Version

3.6.0-DEVb222

Describe the bug

WildStacker override's Towny's land protection system and allows anyone to break spawners, regardless of what Towny allows or not. Towny is a survival plugin that allows for land claiming (towns) in order to protect players' builds and creations. In normal behavior, when a player is not a part of a town, they cannot destroy anything (blocks, interact with doors, build, etc.) However, people are able to break spawners because of WildStacker overriding Towny's cancelled block break events. I tested my server without WildStacker and players were no longer able to break spawners.

To Reproduce

1) Install Towny and WildStacker 2) Create a town and have an outsider (another account) try to break inside the claimed land. They should not be able to. 3) Place a spawner and make sure spawners are handled by WildStacker. Have the outsider account break the spawner. It should work.

Additional Information

Here's my current configuration: https://pastebin.com/nJZMcF27

I talked with the Towny developer and he said that the plugin (Wildstacker) will need to ignore cancelled block break events, as well as an ignorecancelled=true in the EventHandler and they should be on Highest or Monitor priority.

OmerBenGera commented 2 years ago

WildStacker doesnt override anything. Towny is probably not cancelling properly their events in the correct priority, causing WildStacker to run before Towny. They listen in HIGH priority instead of normal, causing it to break, as you can see here: https://github.com/TownyAdvanced/Towny/blob/16c1b9804923c48c2f768e3da4a0d4ec9c3b6cba/src/com/palmergames/bukkit/towny/listeners/TownyBlockListener.java#L53

Open an issue on their github repository so they can change it. I will keep this issue opened for now.

skw359 commented 2 years ago

So if @EventHandler(priority = EventPriority.HIGH, ignoreCancelled = true) was changed to @EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true), the problem would (or should be at least) be fixed? 😅

OmerBenGera commented 2 years ago

Exactly.

skw359 commented 2 years ago

Okay I will try thank you! I'll update here with whatever happens 😄

OmerBenGera commented 2 years ago

Perfect. For now, I am keeping this opened. You are more than welcome to mention this issue on the other issue

LlmDl commented 2 years ago

Towny operates on High to maintain compatibility with WorldGuard. Your plugin should be listening for the block break on highest or even monitor, to see whether anything has cancelled the break.

OmerBenGera commented 2 years ago

Towny operates on High to maintain compatibility with WorldGuard. Your plugin should be listening for the block break on highest or even monitor, to see whether anything has cancelled the break.

I see. I do the same thing, just for SilkSpawners. Why do you need to care about WorldGuard, if you cancel the event anyways?

OmerBenGera commented 2 years ago

@LlmDl ?

LlmDl commented 2 years ago

It goes back 10+ years so I'm a bit fuzzy on all the particulars, but if we aren't operating higher than WG, then the WG global regions would override anyone inside of their own town/plot. There's probably other reasons I don't remember.

If you weren't cancelling the events yourself I'd say you should be listening to block breaks on Monitor.

OmerBenGera commented 2 years ago

It goes back 10+ years so I'm a bit fuzzy on all the particulars, but if we aren't operating higher than WG, then the WG global regions would override anyone inside of their own town/plot. There's probably other reasons I don't remember.

If you weren't cancelling the events yourself I'd say you should be listening to block breaks on Monitor.

Are you sure it still an issue? We're talking about breaking of blocks, and that you cancel the BlockBreakEvent on HIGH, similar to WS's listening priority. I don't see how WG can somehow interface with it, if both plugins just cancel the events... I might be wrong, not sure

LlmDl commented 2 years ago

Either way, you're listening for block breaks earlier than other plugins can/will listen to it. You should be waiting until Monitor to see if the event was cancelled by anything else.

OmerBenGera commented 2 years ago

Either way, you're listening for block breaks earlier than other plugins can/will listen to it. You should be waiting until Monitor to see if the event was cancelled by anything else.

Not really. Plugins should cancel block-break if they do it for claiming purposes on NORMAL. HIGH and HIGHEST are used for the purpose that I use - to also cancel the event if needed, but do it after other plugins. Also, I can't use MONITOR or HIGHEST, due to support for SilkSpawners.

Anyways, I am not gonna argue on this matter; I think you are able to use NORMAL for your cancellation, I can't think about a reason how it would conflict with WorldGuard nowadays, and it will also probably fix potential future issues with other plugins doing the same as WildStacker does.

LlmDl commented 2 years ago

It is still an issue: https://github.com/EngineHub/WorldGuard/blob/2541483ebc189a6749f7f0a77c48a834392e91d3/worldguard-bukkit/src/main/java/com/sk89q/worldguard/bukkit/listener/WorldGuardBlockListener.java#L81

The idea that a protection plugin has to operate on Normal isn't a rule I've seen anywhere, Towny and WorldGuard have never followed it anyways.

The issue here is not a failing of Towny, it is that you're doing something before every plugin has had their kick at the can, and assuming that the event will not be cancelled later on. I'm not sure what your silkspawner issue is, but you could also consider spreading your listener over the current priority, and then doing the actual drop on Monitor to avoid this scenario.

Also not looking to argue about this, just pointing out what is happening for the users here.

OmerBenGera commented 2 years ago

It is still an issue: https://github.com/EngineHub/WorldGuard/blob/2541483ebc189a6749f7f0a77c48a834392e91d3/worldguard-bukkit/src/main/java/com/sk89q/worldguard/bukkit/listener/WorldGuardBlockListener.java#L81

The idea that a protection plugin has to operate on Normal isn't a rule I've seen anywhere, Towny and WorldGuard have never followed it anyways.

The issue here is not a failing of Towny, it is that you're doing something before every plugin has had their kick at the can, and assuming that the event will not be cancelled later on. I'm not sure what your silkspawner issue is, but you could also consider spreading your listener over the current priority, and then doing the actual drop on Monitor to avoid this scenario.

Also not looking to argue about this, just pointing out what is happening for the users here.

The issue is that SilkSpawners handle breaking of spawners as well, and WildStacker must be fired before them. I'll look into another solution, maybe just removing their listener for BlockBreakEvent or something.

Thanks for taking your time into answering, appreciated.

OmerBenGera commented 2 years ago

Check out latest dev build, let me know if it works.

skw359 commented 2 years ago

Sure! I am currently groovin' up a test server, but holidays of course slow everything down :)

I'll report back here to see if it works or not - thank you so much!

skw359 commented 2 years ago

It appears to work 100% - thank you very much once again and have a wonderful holiday season :)