akdukaan / GPFlags

GriefPreventionFlags is a Minecraft plugin to allow players to customize their GriefPrevention claims with claimflags.
GNU General Public License v3.0
17 stars 20 forks source link

New Flag: NoMapMaking #47

Closed ZepsiZola closed 1 year ago

ZepsiZola commented 1 year ago

Is your feature request related to a problem? Please describe. Players on my SMP server get annoyed when they work so hard on creating a map-art that they intend on selling, only for other players to stumble across it and make their own maps from it, causing their map-arts to become essentially worthless. The issue stems from people being able to create unauthorized map-arts in someone else's claim.

Describe the solution you'd like I just want a simple flag that stops people who aren't authorized in the claim from making maps in it. Something like "NoMapMaking" would make sense. On top of that it can't possibly be too hard to just cancel the "Make Map" event or whatever.

Describe alternatives you've considered I've told the players that they can also cover their maparts if they dont want them stolen, but apparently that's too much hassle.

akdukaan commented 1 year ago

On top of that it can't possibly be too hard to just cancel the "Make Map" event

I looked into this and there is no MakeMapEvent. I’d have to cancel the PlayerInteractEvent which complicates things quite a lot. I’d have to do a lot of filtering to see if this event was an attempt to create a map or something totally unrelated like open a chest.

And even after doing all that filtering, just canceling the event if the player is standing within the claim could easily be bypassed. I don’t know all that much about how map creation works, so correct me if I’m wrong here. But if someone stands just outside of the claim, they’d still be able to render the art within the claim, or at least part of it.

The only way I could see this being feasibly implemented would be after some sort of MapCreateEvent is made by some Bukkit implementation. This would also let me more easily look at the area contained by the map and appropriately cancel only the maps that should be canceled.

And with a MapCreateEvent, I imagine I would have much more control of the map that is created. However, I could see the flag causing issues if, for instance, someone wants to make a map of their own claim but there’s a nearby claim with the NoMapMaking flag. I considered simply cutting the claim out of the map and have the map rendered with a hole in it which makes more sense than canceling the map entirely. However, doing that, I’d still have to consider how players might try to update the map. I can lock the map to make it not further discoverable but that means that they also wouldn’t be able to continue to discover more of the map outside of the claim. Unfortunately, there doesn’t seem to be a good way to cut a hole in a map.

So in summary, I don’t think this flag is something I can really consider adding until a MapCreateEvent is made, and even then, there are a lot more details I’d need to consider.

Phoenix616 commented 1 year ago

You could use the MapInitializeEvent to check if the map's location is in/over a limited area. Then you could either change the location of the backing MapView to a default location (like the spawn) or remove the renderer/add your own renderer to draw an overlay over the disabled region.

ZepsiZola commented 1 year ago

Because my issue pertains specifically to map-arts, where typically an entire map-section will be filled up by one claim, I don't think other players would be able to bypass this, as they wouldn't be able to make the map in any part of the map-section since the entire map-section is taken up by one claim. To clarify, map-arts take up the entire map, meaning the claim would take up the entire map, meaning a player would not be able to make a map in that area.

ZepsiZola commented 1 year ago

I thought of the "cut a hole in the map" idea too, but that seemed too complicated, so I just thought simply canceling a map from being created in a claim would suffice.

akdukaan commented 1 year ago

You could use the MapInitializeEvent to check if the map's location is in/over a limited area. Then you could either change the location of the backing MapView to a default location (like the spawn) or remove the renderer/add your own renderer to draw an overlay over the disabled region.

I don’t think this considers that we still want maps by the claim owner to be unrestricted. MapInitializeEvent doesn’t have any sort of way to get the player who created the map as far as I know.

ZepsiZola commented 1 year ago

I don’t think this considers that we still want maps by the claim owner to be unrestricted. MapInitializeEvent doesn’t have any sort of way to get the player who created the map as far as I know.

Actually, I'd certainly take the ability to just set a flag that stops everyone from making maps in the claim. It would solve my problem, but it would be just a little bit less elegant than I wanted. My players would be able to make the map-art, make their maps, set the flag so that no one can make maps, and then unset the flag when the owner wants to make more maps.

ZepsiZola commented 1 year ago

If you can't find a way to create a flag that stops untrusted players from making maps, I'm fine with a flag that stops all players from making maps.

akdukaan commented 1 year ago

If you can't find a way to create a flag that stops untrusted players from making maps, I'm fine with a flag that stops all players from making maps.

The issue is that MapInitializeEvent is called not only when first creating the map but also after a restart when someone goes to view the map again. If the map was legitimately created by the claim owner but the claim owner later set the NoMapMaking flag on the claim, It'd break the map even though we'd want it to stay the same.

Seeing what you said about map arts taking up the full claim does make it a little easier to make such a flag. To prevent players from abusing the flag, I could make it so setting the flag requires a claim that fills up exactly the map boundry. And for detecting when a player tries to make a map, I'd still have to listen to the PlayerInteractEvent and filter out a bunch of exceptions to see if they're trying to make a map which I wouldn't really want to do but might be the only solution if a MapCreateEvent isn't made.

There might be a much easier way to solve this though. Have you considered NoEnter? Players need to be in the claim to create maps and NoEnter will prevent untrusted players from entering the claim.

Phoenix616 commented 1 year ago

The issue is that MapInitializeEvent is called not only when first creating the map but also after a restart when someone goes to view the map again

That should not be the case. According to the server code it should only call this when a new map is created, not when already stored map data is loaded.

ZepsiZola commented 1 year ago

There might be a much easier way to solve this though. Have you considered NoEnter? Players need to be in the claim to create maps and NoEnter will prevent untrusted players from entering the claim.

That would be a slippery slope for my server as I'd expect people would just start using that flag to stop people from entering their claims and not even for map-arts, ruining the openness and explorability of the world. So no, I wouldn't be able to do that.

Phoenix616 commented 1 year ago

Actually no, the implementation seems wrong from what I can tell (or at least not match the comment...)

image

It claims that it's only called when the map is invalid but in reality it's always called when the map is loaded from file. Maybe that changed at some point?

EDIT: Yeah, looks like the 1.17 update broke/changed it.

ZepsiZola commented 1 year ago

Seeing what you said about map arts taking up the full claim does make it a little easier to make such a flag. To prevent players from abusing the flag, I could make it so setting the flag requires a claim that fills up exactly the map boundry. And for detecting when a player tries to make a map, I'd still have to listen to the PlayerInteractEvent and filter out a bunch of exceptions to see if they're trying to make a map which I wouldn't really want to do but might be the only solution if a MapCreateEvent isn't made.

How willing are you to do this? I completely understand if you don't feel the need to, but I just wanna gauge whether you'd actually do this or not.

akdukaan commented 1 year ago

If I have to use PlayerInteractEvent, I’ll give it a shot but if the filtering seems to be too complicated and buggy, might not finish.

If there’s a MakeCreateEvent I can use, very likely I’ll make the flag. Want to do me a favor and make an issue on the Paper GitHub about the event?

Also, I think I changed my mind a little on the implementation. Going to make it not be strict about which claims the flag can be set to, but will only enforce the flag if it’s set to the entire map boundary.

ZepsiZola commented 1 year ago

If there’s a MakeCreateEvent I can use, very likely I’ll make the flag. Want to do me a favor and make an issue on the Paper GitHub about the event?

I'm not gonna lie, I don't think I have the technical knowledge to ask for them to implement an event such as that. Would it literally be as simple as saying "Can you guys create a MapCreateEvent?". And do you think they'd reject it? And also I feel like this would take a while even if they did feel like implementing this.

Phoenix616 commented 1 year ago

If there’s a MakeCreateEvent I can use, very likely I’ll make the flag. Want to do me a favor and make an issue on the Paper GitHub about the event?

I'm writing a PR to Spigot to fix the MapInitializeEvent right now.

ZepsiZola commented 1 year ago

If there’s a MakeCreateEvent I can use, very likely I’ll make the flag. Want to do me a favor and make an issue on the Paper GitHub about the event?

I'm not gonna lie, I don't think I have the technical knowledge to ask for them to implement an event such as that. Would it literally be as simple as saying "Can you guys create a MapCreateEvent?". And do you think they'd reject it? And also I feel like this would take a while even if they did feel like implementing this.

But yeah I'll write an issue to PaperMC github if that's what it takes.

akdukaan commented 1 year ago

But yeah I'll write an issue to PaperMC github if that's what it takes.

No need. Looks like Phoenix is PRing to Spigot which is better.

I can probably have the flag ready in a few weeks.

ZepsiZola commented 1 year ago

But yeah I'll write an issue to PaperMC github if that's what it takes.

No need. Looks like Phoenix is PRing to Spigot which is better.

I can probably have the flag ready in a few weeks.

Alright thanks. And this flag will make it so that no one (not even owners) can create a flag while inside the claim?

ZepsiZola commented 1 year ago

Thank you both guys.

Phoenix616 commented 1 year ago

No need. Looks like Phoenix is PRing to Spigot which is better.

Unfortunately it looks like this behaviour has at least been around since 1.13 so I'm not going to PR anything to change it :S I guess a separate event is really necessary and you would need to request that from Spigot or Paper.

ZepsiZola commented 1 year ago

Ok I've submitted an issue to Paper. https://github.com/PaperMC/Paper/issues/9073

ZepsiZola commented 1 year ago

No need. Looks like Phoenix is PRing to Spigot which is better.

Unfortunately it looks like this behaviour has at least been around since 1.13 so I'm not going to PR anything to change it :S I guess a separate event is really necessary and you would need to request that from Spigot or Paper.

Why would that stop you from making a PR

Phoenix616 commented 1 year ago

Why would that stop you from making a PR

Because changing API behaviour that has been around for a very long time is really bad. It's even questionable if this behaviour was actually broken/wrong or if just the one comment is wrong/confusing as the event in the current implementation of firing everytime a map is loaded from disk and not just newly created does have some value (e.g. if you want to add your own renderers to maps)

ZepsiZola commented 1 year ago

I don't think PaperMC will do anything about this one. Did you say that you were gonna try make the flag anyway?

akdukaan commented 1 year ago

I made it with the PlayerInteractEvent. You can download from https://jenkins.akiradev.xyz/job/GPFlags/. I kept it simple and made it cancel the map creation as long as you are standing in the claim -- didn't do any of the checking that the claim contains the entire map region. You can make it so the claim owner can bypass the flag using bypass trust permissions similar to the ones explained at https://modrinth.com/plugin/gpflags. Let me know how it works, preferrably on Discord. Might be removed before official release if it seems too buggy.

Also, just to be safe, take a backup of your flags.yml.

ZepsiZola commented 1 year ago

What is your discord?

akdukaan commented 1 year ago

https://discord.com/invite/MBdsxAR