-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add luckperms context support for worldgroup. #438
base: main
Are you sure you want to change the base?
Conversation
f202a31
to
1d52822
Compare
luckPerms = LuckPermsProvider.get(); | ||
} catch (IllegalArgumentException e) { | ||
Logging.warning("Unable to hook into LuckPerms. API not enabled!"); | ||
Logging.warning(e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the API something that has to be enabled by the user? Or is this an actual error?
Just wondering because it might be annoying if someone purposefully disables the API and gets a warning every time they start their server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If luckperms is installed (checking alr done above), it should always be enabled unless luckperms crashed on their server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looks good then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just took a look at ContextCalculator's Javadoc, do you know if the calculation is thread-safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to be thread-safe bcu calls may be async. Source code has 3 pointers to take note:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I read, I was asking if the code you wrote is thread-safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 54 to 66 in f704fa2
worldName = worldName.toLowerCase(); | |
List<WorldGroup> worldGroups = new ArrayList<>(); | |
for (WorldGroup worldGroup : getGroupNames().values()) { | |
if (worldGroup.containsWorld(worldName)) { | |
worldGroups.add(worldGroup); | |
} | |
} | |
// Only use the default group for worlds managed by MV-Core | |
if (worldGroups.isEmpty() && plugin.getMVIConfig().isDefaultingUngroupedWorlds() && | |
plugin.getCore().getMVWorldManager().isMVWorld(worldName)) { | |
Logging.finer("Returning default group for world: " + worldName); | |
worldGroups.add(getDefaultGroup()); | |
} |
hmm yea getGroupsForWorld
does have the chance of modifying... I think using getGroups
then filter is a safer option since makes a copy
LuckPerms has a context feature (https://luckperms.net/wiki/Context) which allow you to define permission per world/server etc. This PR add the ability to have permission per-world group which can allow more efficient context setup in some cases. E.g. instead of needing to set 3 context world
survival
survival_nether
andsurvival_the_end
, we can set context 1 for world groupsurvival
.