potatolain / TravelPortals

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

UUID Lookup - Question #29

Closed Flying--Dutchman closed 4 years ago

Flying--Dutchman commented 4 years ago

Is there a reason not to use Player.getUniqueId() before calling the Mojang API? https://github.com/cppchriscpp/TravelPortals/blob/b65afcd630735ecbbbe2eda03300494d6c7a476d/src/main/java/net/cpprograms/minecraft/General/uuidconverter/UuidConverter.java#L72

Phoenix616 commented 4 years ago

Probably because the converter is executed on plugin load and its highly unlikely that Players will already be online then. I guess a simple source which checks online players wouldn't hurt too much but I doubt it would actually improve anything.

Flying--Dutchman commented 4 years ago

Ah ok, so Bukkit doesn't hold a cache version?

Ok thank you 👍

In that case I will be creating a new PR in the next couple of days. A new UUIDSource for Geyser, so that cross platform users also will have the "correct" UUID stored for their portals

Phoenix616 commented 4 years ago

The server has a cache of that but it's not accessible via the API last time I checked. And the Bukkit#getOfflinePlayer method might cause a lookup to Mojang if the information isn't cached anyways (but via a method which doesn't batch multiple names so it could more easily cause issues like rate limiting)

Regarding Geyser: I would be surprised if that isn't supported already. All that TravelPortals does is get the UUID directly from the Player object when creating the portal, I would assume with Geyser the appropriate UUID gets returned there already? And the UuidConverter tool is only used when converting old, pre-UUID configs to UUID based one and Geyser didn't exist back then.

Flying--Dutchman commented 4 years ago

Regarding Geyser: I would be surprised if that isn't supported already. All that TravelPortals does is get the UUID directly from the Player object when creating the portal, I would assume with Geyser the appropriate UUID gets returned there already? And the UuidConverter tool is only used when converting old, pre-UUID configs to UUID based one and Geyser didn't exist back then.

Thats what I thought would happen, but it doesn't seem to work that way. (Tested on 1.16.2 and 1.16.3).

Here the Portal gets created: https://github.com/cppchriscpp/TravelPortals/blob/6adf5407a79b1045ccee6c71d2867ebfe8ae88ec/src/main/java/net/cpprograms/minecraft/TravelPortals/TravelPortalsBlockListener.java#L135

Here is the UUID check: https://github.com/cppchriscpp/TravelPortals/blob/6adf5407a79b1045ccee6c71d2867ebfe8ae88ec/src/main/java/net/cpprograms/minecraft/TravelPortals/storage/PortalStorage.java#L112-L113 But as we can see, at this point it only gets stored in the "namesToConvert"-Set.

The set is only checked during the PortalStorage.update() method: image

Which is only called during "onEnable": image

Am I missing something?

I created on both of my server multiple portals, they only get the uuid when I restart my server. Of course, on startup it won't be able to get any Geyser UUIDs without a new UUIDSource.

Phoenix616 commented 4 years ago

Yeah, apparently the owner is only set to the name and not the UUID too. The UUID should be passed there too if UUID storage is enabled. Apparently I missed that when I did the UUID update :S

Phoenix616 commented 4 years ago

Set the UUID directly on the portal on creation now. Feel free to improve the converter/unknown player handling though so that it gets compatible with Geyser's XUIDs or tries to use the local cache :)

Flying--Dutchman commented 4 years ago

Thank you!

Won't need it ATM, but will work on it in the near future.