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

Added support for getOfflinePlayer(String) with optional cache #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TheBusyBiscuit
Copy link

PaperMC/Paper#4687 has introduced the ability to get an OfflinePlayer by their name without making a web request.

To properly utilise this feature in a Spigot-compatible environment, we currently need to rely on an isPaper and a version check to even see whether one is able to optimize for that.
I think this is a perfect opportunity for a PaperLib implementation.

This is just a basic implementation which was done in a similar manner to how the BlockStateSnapshot is handled.
The boolean makeWebRequest determines whether a web request is allowed. In environments that do not support Bukkit.getOfflinePlayerIfCached(), this boolean is just ignored. Similar to how BlockStateSnapshot will ignore the useSnapshot boolean if unsupported.

Of course, the Bukkit.getOfflinePlayerIfCached() method will not be available in every single 1.16.4-compatible build of Paper I presume, but I am gonna assume the typical "old versions are not supported anyway" situation here for now.
Lemme know if there is anything that should be changed or documented.

Copy link
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

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

Couple of things plus you're missing the helper method in the PaperLib class (and its javadocs)

src/main/java/io/papermc/lib/environments/Environment.java Outdated Show resolved Hide resolved
@Override
@Nullable
public OfflinePlayer getOfflinePlayer(@Nonnull String name, boolean makeWebRequest) {
return Bukkit.getOfflinePlayer(name);
Copy link
Member

Choose a reason for hiding this comment

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

reflection can be used here to check if the profile cache returns null for the given username (UserCache#c#get mapping has been consistent since 1.8.8). That way the makeWebRequest parameter can be respected even if we're on spigot.

(core team may have input on this)

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, that would certainly be a neat idea.
I will look into it but also wait and see what others think about this.

Copy link
Author

@TheBusyBiscuit TheBusyBiscuit Jan 6, 2021

Choose a reason for hiding this comment

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

So I had a look into it and UserCache#c seems to refer to a static boolean on Spigot 1.16.4 (tested using git-Spigot-05da6fa-b0c6dfe).
So unless, that was a typo the mapping does not seem to be as consistent as expected.
The actual Map<String, UserCacheEntry> is found under UserCache#d on the build I investigated.

There is a UserCache#getProfile(String) method though, which returns a nullable GameProfile for a given username from the cache (unless expired).
I would need to look up how long this method existed though.

Update: UserCache#getProfile(String) has been consistent since CB 1.8 (said class did not exist prior to 1.8). Invoking that method using reflection is definitely our safest option to implement this in a non-Paper environment.
What do you think?

* Added static helper method to PaperLib
* Renamed implementation classes
@TheBusyBiscuit
Copy link
Author

Thanks for the input!

you're missing the helper method in the PaperLib class (and its javadocs)

Whoops, I somehow completely forgot about that, thanks for spotting it.
I agree with the name changes for the classes as they seem more reflective of the internal logic here, also implemented the reflection-check you suggested for the method. Probably much better than simply relying on people to update their software.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants