Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benwoo1110
Copy link
Member

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 and survival_the_end, we can set context 1 for world group survival.

@benwoo1110 benwoo1110 added PR: Enhancement Pull requests to implement a feature or improvement in code. State: Needs Review By Dev Pull requests requires the approve of lead dev. labels Mar 9, 2021
luckPerms = LuckPermsProvider.get();
} catch (IllegalArgumentException e) {
Logging.warning("Unable to hook into LuckPerms. API not enabled!");
Logging.warning(e.getMessage());
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, looks good then.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Enhancement Pull requests to implement a feature or improvement in code. State: Needs Review By Dev Pull requests requires the approve of lead dev.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants