Skip to content

Add Profile API.#33

Closed
Techcable wants to merge 1 commit into
PaperMC:masterfrom
Techcable:feature/uuid-api
Closed

Add Profile API.#33
Techcable wants to merge 1 commit into
PaperMC:masterfrom
Techcable:feature/uuid-api

Conversation

@Techcable
Copy link
Copy Markdown
Contributor

Cache uuid/name requests.
Reroute requests in NMS through the api.
Make online player profile access thread safe (not player entities).
Pluggable lookup implementation, similar to @geNAZt 's in #13 .

@geNAZt
Copy link
Copy Markdown

geNAZt commented Feb 19, 2016

The implementatio of the CopyOnWriteHashmap is wrong. Its more like a fully synchronized memory wasting implementation 😁

The synchronized Blocks should only go around the copy and merge methods not on the full operation.

@Techcable
Copy link
Copy Markdown
Contributor Author

@geNAZt but then it doesn't guarantee a consistent snapshot, since someone else could be modifying it.
And how does it waste memory? Are there leaks?

@Techcable
Copy link
Copy Markdown
Contributor Author

Switched from a lookup interface to lookup events per @aikar 's suggestion.
Added ProfileLookupCallback.assumeFound() per @0x277F 's suggestion.
Switched from CopyOnWriteHashMap to synchronized map to fix issues with case insensitivity in the player map.

@aikar
Copy link
Copy Markdown
Member

aikar commented Feb 24, 2016

why the synchronize on the player map... ? that should not be accessed async ever. By synchronizing it, you're adding cost to player lookup operations across the entire server.

We should never ever ever ever encourage accessing main thread state (online players) async by throwing a synchronized on it.

@aikar
Copy link
Copy Markdown
Member

aikar commented Feb 24, 2016

On Server, public PlayerProfile getIfOnline(String name) {

These feel unnecessary.
getPlayerExact(name), check if null, and player.getProfile() is simple enough.

@aikar
Copy link
Copy Markdown
Member

aikar commented Feb 24, 2016

also this is a profile API, uuid is just a single detail of profiles.

suggest renaming package to .profile. and cleaning up the 'uuid api' comments.

@Techcable
Copy link
Copy Markdown
Contributor Author

The cache now maintains its own map of online players, and removed all the thread safety from the NMS player map (since its hard to maintain).
Now uses 'internal listeners' for this (since you need a plugin to register a regular listener).
Refactored all the 'uuid api' comments to 'profile api', and renamed the package.
The getIfOnline() methods and isOnline() methods have been removed.

@Techcable
Copy link
Copy Markdown
Contributor Author

Updated to 1.9

@zachbr
Copy link
Copy Markdown
Contributor

zachbr commented Mar 14, 2016

This still uses our old package

@Techcable Techcable force-pushed the feature/uuid-api branch 3 times, most recently from 2335e16 to fb1a9a0 Compare April 15, 2016 16:05
+
+ public EventProfileLookup(ProfileLookup delegate) {
+ Preconditions.checkNotNull(delegate, "Null delegate");
+ this.delegate = delegate;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this.delegate = Preconditions.checkNotNull(delegate, "delegate");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kashike That isn't a normal syntax used in the project nor necessary.

Best to leave as is. It reads cleaner as the way it currently is.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other parts of the project don't even use Preconditions. Use Validate, then, @Techcable.

@kashike
Copy link
Copy Markdown
Member

kashike commented Apr 15, 2016

PlayerProfile -> Profile

@Techcable
Copy link
Copy Markdown
Contributor Author

I've resolved most of the issues @kashike pointed out.
I've also renamed PlayerProfile to AccountProfile and 'getProfile()' to 'getAccount()' per @kashike 's demands.

@zachbr
Copy link
Copy Markdown
Contributor

zachbr commented Apr 22, 2016

@Techcable this still has a WIP tag on it. Are we at a point now where we can start doing final review?

@Techcable Techcable changed the title [WIP] Add UUID API. Add UUID API. Apr 22, 2016
@Techcable
Copy link
Copy Markdown
Contributor Author

@Zbob750 oh, i forgot to edit the tile.
You may commense final review!

@aikar
Copy link
Copy Markdown
Member

aikar commented Apr 22, 2016

<Aikar> Bukkit.getServiceManager().getMojangProfileLookupService() 
<Aikar> then getServiceManager.registerService(ProfileLookupService.class, myService);
<Aikar> + plugin*
<Aikar> dont remember those params off hand, but that general idea
<Aikar> and then if the persons service cannot complete the request, theycan call mojang
<Aikar> for example, i would check my DB
<Aikar> if my db fails ,call mojang, then store result in my DB
<Aikar> this is java, lets be verbose with our names for readability :P
<Aikar> the interface should be ProfileLookupService, then MojangLookup > MojangProfileLookupService
<DemonWav> obligatory link to FizzBuzzEnterpriseEdition
<Aikar> then your cacheing one doesnt need to be an interface, it can be an extension of Mojang
<Aikar> also are these API's for callbacks ACTUALLY async? I didnt call mojangs code being async...
<Aikar> its callback style, but not async
<Aikar> the looks can block, so they prolly shouldnt be called Async in the event name, and just dynamically set the Async flag based on main thread
<Aikar> or provide API's that ensure async

@aikar aikar changed the title Add UUID API. Add Profile API. Apr 28, 2016
@Techcable
Copy link
Copy Markdown
Contributor Author

I think having a services api and an event system is unnecessary complexity.

@Techcable Techcable force-pushed the feature/uuid-api branch 2 times, most recently from 52d3b16 to 2eef99a Compare May 27, 2016 16:51
@Techcable
Copy link
Copy Markdown
Contributor Author

@aikar @Zbob750 @kashike Besides the 1.10 update, renaming 'AccountProfile' to Profile, making 'Profile' and interface, and renaming 'ProfileLookup' to 'ProfileManager' is there anything else you want me to do?

@aikar
Copy link
Copy Markdown
Member

aikar commented Jun 9, 2016

@Techcable regardless of how much you disagree, EVERY ONE OF US agreed we want Profile a mutable API design. it was 100% agreement. So....

@Techcable
Copy link
Copy Markdown
Contributor Author

@aikar You mean the properties?
Yeah, mutable properties is fine with me 😉

Will that be all?

@Techcable Techcable force-pushed the feature/uuid-api branch 2 times, most recently from 90d4fee to ce68d02 Compare July 3, 2016 01:31
@AlfieC
Copy link
Copy Markdown
Contributor

AlfieC commented Jan 22, 2017

any update?

@aikar
Copy link
Copy Markdown
Member

aikar commented Jan 22, 2017

I'm going to revisit this once I get time, but I think we rather go with the official Mojang GameProfile class instead. I've actually got a lot of what this PR was aiming to provide in my fork, so i'm aiming to just pull it down to Paper.

@kamcio96
Copy link
Copy Markdown

Hmm, what about player's skin?

@kashike
Copy link
Copy Markdown
Member

kashike commented Jun 11, 2017

Any update to this?

@0x277F
Copy link
Copy Markdown

0x277F commented Jun 11, 2017

I can't remember, does it have a way to take a profile lookup, pass it a callback but block until the lookup completes, a la Netty's ChannelPromise#sync?

@aikar
Copy link
Copy Markdown
Member

aikar commented Jun 18, 2017

Closing this as I've finally pulled in my profile lookup events and other things to come. But these 2 events satisfy the root desire (ability to prefill from cached profile properties to avoid being rate limited)

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.

8 participants