Apply advancements async

[Proximyst]

Asynchronous advancements, take 2.

Advancements are loaded in the same way as before, except asynchronously.
Unlike #3454, this uses a delegate class to make plugins like Citizens not spew
errors as if it's their fulltime job.

[tofipix]

Thanks!, I can confirm that it works , and it helped my server with login performance!

[Toffikk]

I hope that it will be merged soon

[Toffikk]

@Proximyst What do You think about licensing Your work/patches under MIT?

[Proximyst]

I am contributing these to an MIT licensed project; they are therefore also licensed under the same licence.

[Toffikk]

Isn’t Paper licensed under GNU 3 , because the last time i checked , Every author who want their patches be licensed under MIT , then they must write theirselves down in „license.md” file

[Proximyst]

Aikar just corrected me on that on Discord, yeah. It's still free software, so I don't particularly care.

[Toffikk]

It still would be nice if You license Your patches under MIT so anyone can use them

[Proximyst]

My contributions are GPLv3. This means anyone can use them, granted they give me the credit for my code. If you'd like to discuss my choice in what licence to keep it under, GitHub comment section is not the place.

[CDFN]

No need to ping & ask for review. ASAP Aikar will review it so no worries. @tofikarz

[aikar]

Sorry been busy but been meaning to come give feedback.

Overall, absolutely not ok with this implementation. This is a maintenance nightmare that puts it into category of "Drop this every update until proxi resubmits it" which I don't like.

Really needs to be back closer to original impl of start loading the data early, but if issues were found with version 1 then I have a new implementation proposal

On creating the EntityPlayer in PlayerList#attemptLogin, we can break the PlayerLoginEvent right below it into a split method, and have a marker for "advancementDataReady = true" when its done.

Then the network manager is ticking thats calling this, we can delay progressing to the next stages of the login operation until the data is ready.

But I have to ask, is this change even still needed?

We merged a major performance optimization to advancements. Is there NEW benchmarks showing a heavy hit? how much time are we talking about trying to save here.

If the cost of this work results in it being slower for majority of players, it's not worth doing.

We'd need to see what a "has been playing for a dew days" player might look like for performance for baseline, and "a veteran with most advancements completed" for the worst case scenario.

if a veteran was 1ms and moderate players is like 0.1ms, i'm not sure its worth the cost to do this.

Also what was the issues with previous attempt? It was closer to what id expect, just did more than I expected. I would of left the field in place, made it non final, and then anytime .getAdvancementData() is called and the field ISNT set yet, to join the future to block until it is finished, so its async long as it finishes before something accesses it.

there was more changes in that first PR than I expected, returning null instead of data in scenarios that were previously not possible, so thats why I expect bugs were introduced.

Id love to see attempt 1 cleaned up instead to touch less, but worse case, delay PlayerLoginEvent until its ready - though we may be able to say that advancement data is ok to not be used in PlayerLoginEvent ,and delay PlayerJoinEvent until both chunk AND advancement is ready.

But either way, I'm not ok with this current PR.

[Proximyst]

TL;DR: This PR will be closed and reopened in a new PR in the future if(/when?) I retry the original approach with Aikar's feedback taken into account.

---

This is a maintenance nightmare that puts it into category of "Drop this every update until proxi resubmits it" which I don't like.

I disagree. All methods but two are just generated by IntelliJ to delegate. If Mojang changes advancement loading, we're fucked with a patch to async load them anyways.

But I have to ask, is this change even still needed?

Honestly, probably not. My only real feedback on this is others reporting (on this PR, and from talking with @Puremin0rez who pulled this in their fork) it be beneficial.

Also what was the issues with previous attempt?

Mostly just the god damn plugins we sadly have to give a shit about. Citizens was the main one complaining every so often with its player NPCs, and I personally don't like console spam, so I decided to just redo it in a way that works and avoids Citizens spam.

Id love to see attempt 1 cleaned up instead to touch less, but worse case, delay PlayerLoginEvent until its ready - though we may be able to say that advancement data is ok to not be used in PlayerLoginEvent ,and delay PlayerJoinEvent until both chunk AND advancement is ready.

I'll take a stab at that in the future in a new PR. At the moment, I don't have a need to recreate it, and have others projects I have to focus on.