NamespacedKey based plugin chunk tickets

[Trigary]

A work-in-progress (at the time of creation) implementation of NamespacedKey based plugin chunk tickets. The reasoning behind the creation of this API can be found in #4147

State of draft PR:

• yet to be tested
• yet to receive JavaDocs

[Proximyst]

Looks pretty good so far!

[aikar]

good bit of changes needed. overall dont want to expose actual numeric levels, and want to improve the api signatures more than spigot used.
and while the system supports putting multiple tickets of the same key at different levels, dont want to support it over api as we cant guarantee that, and it also makes it more complicated.

if people want to have multiple tickets at different levels, they can use multiple keys to achieve that.

[Trigary]

Thanks for the review, I shall make these changes.

[Trigary]

I apologize for the delays, I finally made the requested changes.

There is one thing that's definitely not finished yet (they produce a compile error at the moment): there are two getter methods that list chunks that have these keyed plugin chunk tickets. What should they return?

• return Chunk instances, possibly forcing sync chunk loads
• return some sort int-int pair (the X, Z coordinates of the chunk) (I don't think this chunk coord pair class exists in the API yet)
• return Chunk instances for loaded chunks and specify in the JavaDocs that only already loaded chunks are returned, so not yet loaded chunks must be handled manually somehow

Another questionable implementation is CraftWorld#removeKeyedPluginChunkTicket(x, z, org.bukkit.NamespacedKey). It iterates over the TickingLevel values and tries to remove them one-by-one. The alternative is to iterate over the tickets that actually exist in the chunk and remove the correct iterator. The issue with this 2nd option is that removing a ticket is a bit harder to implement, I'm afraid I might make mistakes. And the issue with the 1st option is the performance overhead, heap allocations.

All of these WIP / not finished methods have TODOs in them, that's an easy way to find them.

[aikar]

I think you have a slight bit of confusion. TickingLevel.NONE is still going to be returned in Loaded Chunks.

These chunks will be loaded, just at BORDER status. TickingLevel.NONE = Border

[Trigary]

They will be loaded, but only asynchronously as far as I know, but I might be wrong. So the getter method might catch them when they are not yet loaded, just being loaded. In this case a Chunk does not yet exist AFAIK.

[aikar]

Hmm I see, in that this API is meant to return chunks that have a plugin ticket but since they are loaded async does present a window they havent finished yet.

Returning a collection of Long's does make more sense than chunks, and people can re-map it to Chunk objects and filter by loaded status on their own if needed.

Or even provide a .getKeyedPluginChunkTicketLoadedChunks that does the filtering for you.

[Trigary]

Thank you for your response, I shall make the necessary changes and finalize the PR.

[Trigary]

I am considering this done, apart from testing: I am yet to actually test these additions in action.

Ignore the failed GitHub Actions check. It worked fine locally on WSL and on Git Bash (on 2 separate PCs) and Z750 also believes that GitHub Actions is the culprit here.

[Trigary]

I have finished testing, everything seems to be in order, I am marking this PR as ready, non-draft. For possible future uses, this is what I used for testing: https://pastebin.com/S1B4NKAC

[Proximyst]

The CI does not seem to be in order, actually :p

[Trigary]

@Proximyst

The CI does not seem to be in order, actually :p

Have you read the 2nd paragraph of this comment of mine? #4173 (comment)

[Proximyst]

Tried forcepushing to have GH Actions retry? We can't merge without it passing 😅

[Trigary]

CI is having none of it by the looks of it

[Proximyst]

Well that's because it's trying to merge 1.16.3 code into 1.16.4; there will quite often be some kind of issue. I've rebased your PR for you.

[Trigary]

Makes sense, thank you.

[Trigary]

I have improved the JavaDocs. There was some outdated stuff in there, I removed that. I improved the wording here and there. I also documented what happens when chunk tickets are added to unloaded chunks (the chunk gets loaded async).

I have also moved the removeKeyedPluginChunkTickets(Plugin) call from removePluginChunkTickets(Plugin) to SimplePluginManager. Aikar once told me to put it inside removePluginChunkTickets(Plugin), but I'm confident keyed plugin chunk tickets shouldn't be removed just because non-keyed ones are removed through removePluginChunkTickets(Plugin). Feel free to tell me to revert that though if you believe this change is bad.

I have moved the TickingLevel class to the io.papermc.paper.world package from io.papermc.paper. I have also switched to using import statements. Again, feel free to tell me to revert.

Still awaiting review.

[Owen1212055]

Closing PR, as it seems that this has gone inactive.

If you're still interested in continuing this PR, feel free to leave a comment and we can reopen it for you. 😄

[Trigary]

I am still interested in continuing this PR. I am just waiting for a review.

[Owen1212055]

I am still interested in continuing this PR. I am just waiting for a review.

Would you be able to rebase it? 😄

[Owen1212055]

See above, if you are still interested in continuing, please feel free to rebase this pr. 😄
It should be noted that upstream has added some methods to get tick levels for chunks to the API.