MobArena Clickable signs no longer work

Hello.

We're using signs in our lobby room with some commands inside it that runs as player if he perform a right-click on the sign. But now these commands work only if the arena is in edit mode.

I think it's related to https://github.com/garbagemule/MobArena/commit/af513b03b079ad1a8e611fa6b81348b52384bb3b

I could solve this issue myself by "cancelling the cancellation" of interact event, but you cancel it in MONITOR priority (a priority that should only be used ONLY TO LISTEN TO EVENT RESULTS :/ otherwise it can at least break some logging plugins)...

Example of sign (for easy reproduce):

setblock ~ ~ ~ minecraft:oak_sign{front_text:{messages:['{"clickEvent":{"action":"run_command","value":"ma j"},"extra":[{"color":"dark_red","text":"Click"}],"text":""}',"''","''","''"]}}

Feb 17 '24 12:02 molor

Use https://jd.papermc.io/paper/1.20/io/papermc/paper/event/player/PlayerOpenSignEvent.html instead of PlayerInteractEvent in the mentioned commit, it exists for a months now...

Feb 17 '24 12:02 molor

Also note that WorldGuard already uses mentioned PlayerOpenSignEvent and protects arenas from sign editing, so in that (my) case if you just add a per-arena setting like allow-sign-interaction then I can turn it on and everything will work fine like before.

Feb 17 '24 12:02 molor

TL;DR: If bumping to Spigot API version 1.20.2+ and registering the event handler does not break the plugin on 1.19 servers, we can perhaps use the new event. The MONITOR issue should be fixed if we continue to use interact events. Perhaps it's worth exploring workarounds that don't involve code changes.

Thanks for the bug report!

I could solve this issue myself by "cancelling the cancellation" of interact event, but you cancel it in MONITOR priority (a priority that should only be used ONLY TO LISTEN TO EVENT RESULTS :/ otherwise it can at least break some logging plugins)...

I'm well aware of the role of the MONITOR priority, and all of MobArena's uses of it have, in fact, been perfectly fine up until this recent change. These handlers were never meant to cancel anything, but I guess I lost track of the bigger picture trying to squash the sign editing issue, so thanks for bringing it to my attention 🙂

Use https://jd.papermc.io/paper/1.20/io/papermc/paper/event/player/PlayerOpenSignEvent.html instead of PlayerInteractEvent in the mentioned commit, it exists for a months now...

I'm aware of the event, but thanks for calling attention to it. Unfortunately, this event is not available in the version of the API that MobArena currently targets (note that MobArena targets Spigot, not Paper), or I would have just used it from the get-go. It looks like we'd have to bump to API version 1.20.2 for this event to be available. Over 50% of the current server base runs Minecraft versions lower than that according to bStats. Unless you know for a fact that implementing the suggested change does not break the plugin for those servers, I honestly don't see a justification for the snide remark about how long the event has existed for. It would be nice if people would just update their servers, but there could be lots of reasons as to why they haven't gotten around to it yet, and I'm not about to alienate them for it unless I absolutely have to.

I'm not sure what happens if a plugin tries to register an event handler for a non-existent event. If it doesn't throw up over it, I guess we could check and see if anything else breaks. If you have spare time on your hands, it would be awesome if you could check it out.

Also note that WorldGuard already uses mentioned PlayerOpenSignEvent and protects arenas from sign editing, so in that (my) case if you just add a per-arena setting like allow-sign-interaction then I can turn it on and everything will work fine like before.

This is a knee-jerk reaction and a pretty awkward solution to a problem that should be solved differently. You're right that the best approach is to use the PlayerSignOpenEvent (not sure why the name is different in Spigot), but we can't do that right now (unless we can show that bumping versions and registering a handler for the event is a non-breaking change for 1.19 servers). But adding an entire per-arena setting to allow you to temporarily work around a problem you're having with the plugin in a possibly niche setup is a bit much.

First of all, I think knocking the event handler priority down a notch is a change that should be in the plugin right now, because as you mentioned yourself, MONITOR handlers should not mutate events. If you want to test that out and submit a PR, that would be awesome 🙏 If it solves your problem until we can bump Spigot versions, I think we can hold off on other changes for now.

Secondly, could you perhaps use left-click for your signs instead? That's the recommended way to interact with MobArena signs anyway, so it would be consistent with how the rest of the signs should be used.

Feb 17 '24 20:02 garbagemule

(I would be glad if you can just add per-arena setting like deny-sign-interaction to https://github.com/garbagemule/MobArena/commit/af513b03b079ad1a8e611fa6b81348b52384bb3b and make it true by default... Off-topic, but 90+ issues and 2+ months have passed since last commit... It's sad to see that such a legendary plugin is slowly... dying...)

Mar 25 '24 11:03 molor

The plugin is definitely not dying. Take into account that people have jobs and lives outside of this! :)

The 90+ issues is actually, 12 bugs and 74 ! requests for stuff. That is not bad at all, you don't have to add every singe suggestion the minute they arrive. ^^

Mar 25 '24 11:03 Nesseley

(Please don't take my comment as a complaint or negative feedback. I have been managing the server for almost 10 years now and I understand that people have jobs and lives :D I wrote just to make it clear that I care about the fate of MA and the issue is still relevant for me)

Mar 25 '24 11:03 molor

I'm not going to add a per-arena setting to disable a specific part of the protection functionality. This kind of "sub-configuration" is too messy with how the config-file is currently structured. It's already a huge mess as it is, and if I had added every little setting someone had asked for over the years, it would probably be twice as big as it is now and ten times harder to reason about. It does make sense to break the protect flag open and allow for toggling the various protections on or off to better fit a given server/arena setup, but there is no good way to do that in the current code base and config-file structure. Wonderful input for a rework of the config-file, though.

I'll admit it's a little frustrating to see a comment about "lots of issues and no commits", because if you actually look through the issues, most of them will have a long message from me inviting people to get involved in one way or another. There are issues that "end" with me suggesting a deferral to a rework of sorts, which then, quite understandably, stunts the conversation, but a pretty big chunk is just me talking into the void, hoping for some sort of engagement that never comes.

This issue is a perfect example of that, I think. In my response, I very specifically invited you to try out one of your suggested solutions (MONITOR to HIGHEST priority for a listener) and submit a PR if it was successful, because it's a bug that needs fixing. No answer. I assume you know your way around the code base because of the detective work you put in, but even if you don't, a concrete "I can't do that" is miles better than complete silence. Much less hands-on (in regards to MobArena) is the final question about using left-clicks on your signs. Nothing.

The best way to help the project along is to do the leg work you can do, because it reduces the amount of work I need to do to get changes out. This isn't me being lazy. I don't have as much spare time available to me as I once did, so if I have to do everything, it quite simply takes longer than when I don't have to do everything. I don't think it's unfair of me to invite people to get involved, and I don't think it's unfair of people to decline that invitation. But I do think it's unfair to post a GitHub issue, leave the conversation completely open for a month, and then return with a "what's taking so long?" as if the answers to the questions were supposed to just magically spawn into the conversation in the meantime.

In hopes of steering the issue back on track, here are the options as I see them:

Use left-click instead of right-click to interact with signs. If this works, it is the preferred solution as it makes the custom setup in question consistent with how you're supposed to interact with MobArena signs. This can only be verified by the person with the custom setup.
Change the event handler priority from MONITOR to HIGHEST and write a custom, external event handler to "uncancel" events. This is a much less preferable solution to the problem at hand, because it's a hackaround that may or may not break in the future, and we can't guarantee backwards compatibility with hackarounds. However, the change would be a bug fix in and of itself. This can be done by anyone who can build the plugin from source and knows where to look in the code.
Rework the protection functionality and the config-file in a way that allows for toggling various protection features on or off. This is a lot of refactoring work that requires some knowledge about the code base and design philosophy of the plugin. This can be done by any seasoned Java developer (in collaboration with yours truly).

And to be clear: I can't do the first one. I could do the second one, but I still have no idea if someone else is going to do it (no reply to my last post), and I'd prefer to give the community a chance to get involved, especially with such a "good first issue" kind of change. I will eventually get to to the third one, but I can't give an ETA.

Mar 25 '24 23:03 garbagemule

@molor Can you check option 1? And did you want to PR the changes for option 2? It's completely fine if it will take a while - no rush - but I just want to know if the issue is waiting for you or not.

Mar 27 '24 14:03 garbagemule

Currently I have to register my PlayerInteractEvent handler with MONITOR priority 1 second after MA was enabled, in order to cancel the event in case it was canceled by MA. Only in this case did I manage to make everything work. I agree, this isn't the correct method, but 1) my players are used to having to press the RMB, and to retrain them is, frankly, difficult, and 2) when the issue first happened, I don't have time to do something more correct and thoughtful. Yes, I have time now, but I still don't really want to make changes due to reason # 1.

About making a PR, to be honest, I have absolutely no experience working with Git in that way, and probably the most I can do is building plugins according to the instructions in ReadMe...

So I guess I can only help by running test builds of the plugin and reporting the results.

Mar 27 '24 15:03 molor

About making a PR, to be honest, I have absolutely no experience working with Git in that way, and probably the most I can do is building plugins according to the instructions in ReadMe...

That's okay. If you can build MobArena from source (just run ./gradlew build in a terminal with a valid JDK) and you know which event handler to knock the priority down for, then you should be able to make the change and test it out without me having to do anything. That alone would be a great amount of help, because you'd be able to do most of the leg work instead of waiting for me. Making a PR with the change(s) would just be the icing on top.

Let me know if you'd like to give that a try. Otherwise hit me up on Discord and I'll try to make a build with the change. I've got some time tomorrow (Thursday).

Mar 27 '24 23:03 garbagemule