potatolain / TravelPortals

A portal system for Bukkit for Minecraft
BSD 2-Clause "Simplified" License
3 stars 2 forks source link

Adds ability to limit the creation of portals #26

Closed Flying--Dutchman closed 4 years ago

Flying--Dutchman commented 4 years ago

I wanted to limit the creation of portals by my players. Changes in this PR:

  1. Adds a new permission: travelportals.portal.create.250 Which allows to set a limit on a permission basis. (Simply change the number)
  2. Adds a new config entry: maxportalsperworld: false When set to true, the limit set by the permission applies on a per world basis.
Phoenix616 commented 4 years ago

Can you please change this so there is no portal limit by default? This should only ever be active if the server admins decide that they want this functionality and set the permission explicitly. It shouldn't be set for everyone by default as that would break existing setups.

Also I'm not really a fan of using streams to count the amount but I guess its alright as it's only when a portal creation is detected. Please change it so that is only counted when the player actually has a maximum amount set. There's no need to search through thousands of portals if there is no limit after all.

Another thing: With what permission plugin did you test? Doas the permission attachment searching work properly with common plugins (especially LuckPerms) and also how efficient this is with a large amount of permissions defined.

Flying--Dutchman commented 4 years ago

Can you please change this so there is no portal limit by default? This should only ever be active if the server admins decide that they want this functionality and set the permission explicitly. It shouldn't be set for everyone by default as that would break existing setups.

Of course, I'm working on it.

Also I'm not really a fan of using streams to count the amount but I guess its alright as it's only when a portal creation is detected. Please change it so that is only counted when the player actually has a maximum amount set. There's no need to search through thousands of portals if there is no limit after all.

Great catch, will do. Also, why no streams? I'm relatively new to Java and completly new to Minecraft plugin development...

Another thing: With what permission plugin did you test? Doas the permission attachment searching work properly with common plugins (especially LuckPerms) and also how efficient this is with a large amount of permissions defined.

Tested it on my server with LuckPerms, had no problems so far. Couldn't find another way to check the permissions unfortunatly, except a stream ofcourse =/

Edit:

Would like to move the search for portals as a function to PortalStorage named getPortals(Player player), any objections?

Flying--Dutchman commented 4 years ago

I changed the code, as suggested.

I've noticed a bug with the ownership of portals and the UUIDs, will open a ticket for that one.

Basicly sometimes the "Owner UUID" is null, don't know why. (noticed it sometimes when I changed something in the config) Also, as soon as that happens, my search query for portals of a specific player returns 0, very strange.

Maybe wait with merging, will look further into this.

Edit: Would like to move the search for portals as a function to PortalStorage named getPortals(Player player), any objections?

Phoenix616 commented 4 years ago

Also, why no streams? I'm relatively new to Java and completly new to Minecraft plugin development...

Streams tend to be slower and allocate more memory for objects than traditional for loops. Of course if they aren't used all the time and only when absolutely necessary like it would be in this case then it should be fine until they are actually pinpointed to really cause performance issues.

Tested it on my server with LuckPerms, had no problems so far. Couldn't find another way to check the permissions unfortunatly, except a stream ofcourse =/

If that works then that's good enough. Just wasn't sure if it supported that seeing as they basically completely replace the internal server's permission handling logic (iirc). And if it doesn't work for someone then they can just create an issue anyways and we can take a look at how their setup differs from a "normal" one :)

Would like to move the search for portals as a function to PortalStorage named getPortals(Player player), any objections?

Maybe that could just take the player's UUID and name so that player's that aren't online can be searched too but sounds good.

I've noticed a bug with the ownership of portals and the UUIDs, will open a ticket for that one. Basicly sometimes the "Owner UUID" is null, don't know why. (noticed it sometimes when I changed something in the config) Also, as soon as that happens, my search query for portals of a specific player returns 0, very strange. Maybe wait with merging, will look further into this.

That really sounds odd, I can't really think of anything that would cause that besides maybe some config corruption? Because the getOwnerId method returns null when it wasn't able to read the UUID from the owner string (which should be in the format <name>,<uuid> for legacy/backwards compatibility reasons. (As in the past owner information was just stored by name)

Let me know if you find anything regarding that, otherwise the PR looks good (but of course feel free to add the method that you wanted before we merge this :))

Flying--Dutchman commented 4 years ago

Created 2 new methods in PortalStorage to find the player portals, and switched from the stream to a simple for-loop ;) ATM only with the "Player" class.

Flying--Dutchman commented 4 years ago

Wasn't able to recreate my problem with the uuids. Probably an error an my side.

You can merge this PR.