Plugin API: NamespacedKey based Chunk plugin tickets

[Trigary]

Preamble: I am more than happy to implement this, I am just asking whether this is something that would get merged. I am not looking to waste my time as I did with #3139. I totally understand if stuff isn't deemed "worth to maintain".

The Chunk plugin ticket system currently uses Plugin instances, meaning a single plugin can only have a single ticket on a specific chunk. This means that if multiple separate components of a bigger plugin need to place tickets, then they can't, or the plugin author must resolve conflicts manually.

For plugin authors, a single counter is enough: increment the count when you need the chunk, decrement it when you no longer need it. If you incremented it from 0 to 1, then call the addPluginChunkTicket ticket and if you decremented it from 0 to 1 then remove the ticket. My proposal is to remove this boilerplate code and handle this internally using NamespacedKey based keys instead of Plugin instances.

(Why use NamespacedKey instead of a counter you ask? A counter is a fast, simple, crude method to solve this issue, but is hard to debug and not very extensible.)

This addition must keep Bukkit/Spigot compatibility, so the current system must be left untouched. The addition must also remove all tickets added by the plugin when the plugin gets disabled. Since plugins will either use the newly added or the old API, removePluginChunkTicket can just remove all tickets relating to the Plugin.

[aikar]

I'm not sure what you mean by #3139, I don't see any comment from us rejecting it? Not being merged isn't a rejection, just haven't gotten to it.

This idea overall seems ok and does align better with how the ticket system works, can be a PluginNamespacedTicket that has a string identifier with the keys name, and the ticking status controllable.

public enum TickingLevel { ENTITY, BLOCK, NONE; }

addPluginChunkTicket(x, z, NamespacedKey);
addPluginChunkTicket(x, z, NamespacedKey, TickingLevel tickingLevel);

This would solve some others request too.

[pop4959]

I think this is a wonderful idea, as it does make things much easier for plugin developers. I also strongly agree with Aikar's suggestion of adding the ticket level into the API. I have a use case myself where I would like to ensure that the ticket level is set to 33 (no tick) for loaded chunks, however the current plugin tickets set them to level 31 (in line with regular force loading).

The only thing I'm not sure about is how this would be affected by ticket level propagation. For example, if you wanted to ensure that a chunk is loaded at level 33, a neighboring chunk could propagate into it and reduce the ticket level.

[Trigary]

Sounds good Aikar, I will start working on it soon! I am yet to deep dive into how these tickets work, I have one design question: by PluginNamespacedTicket do you mean that we shouldn't piggyback on TicketType#PLUGIN_TICKET and should rather add a new TicketType, or did I misunderstand?

[Trigary]

The only thing I'm not sure about is how this would be affected by ticket level propagation. For example, if you wanted to ensure that a chunk is loaded at level 33, a neighboring chunk could propagate into it and reduce the ticket level.

@pop4959 Now I have zero clue as to how this system you just described works at the moment, but I believe the logical operation for it would be to allow the change you just described. So even if you placed a TickingLevel#NONE, a player entering the chunk should change the TickingLevel to ENTITY.

And I would like to make it clear that I do not understand the system at all yet, so I have no intention of modifying ticking level propagation or anything. All I want is NamespacedKey based plugin chunk tickets with configurable TickingLevels.

[pop4959]

So even if you placed a TickingLevel#NONE, a player entering the chunk should change the TickingLevel to ENTITY.

Yeah of course, I wasn't saying that. I'm just saying that in the case that there aren't players nearby. Although, I guess then probably all loaded chunks would be not ticking. I'm also not an expert at this system, I just looked into it last week! 😝

And I would like to make it clear that I do not understand the system at all yet, so I have no intention of modifying ticking level propagation or anything. All I want is NamespacedKey based plugin chunk tickets with configurable TickingLevels.

Yeah of course, I was just hopping on board Aikar's train of thought. You'll still need to be aware of how the chunk ticket system works though, as plugin tickets build directly on this system. I recommend checking out this post which I found extremely helpful in understanding it: https://gist.github.com/Drovolon/24bfaae00d57e7a8ca64b792e14fa7c6. Might be slightly outdated but I think the bulk of that information still holds correct.

[aikar]

You don't have to worry about "others" needs. If you add a ticket for non ticking, and yours is the only ticket, then it is non ticking.

If a player moves near it, a player ticket is added at 1 of the 3 levels based on view distances, and the "lowest level" aka "highest ticking level" takes effect.

so if a player teleports to it where you have a NONE ticket at 33, the player adds a ENTITY 31 ticket, the chunk will be at 31.

Once player leaves, their ticket is removed, and now 33 is the only ticket remaining, and it'll demote itself.

I don't want to expose raw ticket levels because that's an implementation detail that could change.

[pop4959]

Oh OK, so they stack rather than replace each other. That makes way more sense. Thanks for explaining, Aikar!

Also I'm for the most part against having raw ticket values exposed in the API, although I think one valid application where someone might want to do that is if they wanted to do some sort of per-player view distance system (so you set lower tickets that propagate out further). For most plugins though, the 3-4 standard ticket levels should be fine.

[Trigary]

Thank you for that link @pop4959, it helps a lot.

This is the planned API at the moment:

boolean addKeyedPluginChunkTicket(int x, int z, NamespacedKey key);
boolean addKeyedPluginChunkTicket(int x, int z, NamespacedKey key, com.destroystokyo.paper.TickingLevel level);

Map<NamespacedKey, Collection<Chunk>> getKeyedPluginChunkTickets();
Collection<NamespacedKey> getKeyedPluginChunkTickets(int x, int z);
Collection<TickingLevel> getKeyedPluginChunkTicketLevels(int x, int z, NamespacedKey key);

boolean removeKeyedPluginChunkTicket(int x, int z, NamespacedKey key);
boolean removeKeyedPluginChunkTicket(int x, int z, NamespacedKey key, com.destroystokyo.paper.TickingLevel level);
void removeKeyedPluginChunkTickets(NamespacedKey key);
void removeKeyedPluginChunkTickets(Plugin plugin);

When adding a ticket, the default level will be the same as the CraftBukkit methods', so ENTITY. This is to make the API less confusing, to make the migration from the old API easier.

When removing tickets without specifying a ticking level, then all ticking levels will be removed. So if for some reason you have multiple ticking level tickets on the same chunk under the same NamespacedKey, then all of them will be removed. This method will probably just iterate over the TickingLevels and try to remove each of them, but that's just an implementation detail.

The methods that remove tickets from all chunks will ignore ticking levels and use an iterator removal approach, but this is just an implementation detail.

The getter methods, with the exception of the ticket level related one, mimic the old API methods.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. If the issue still applies to the most recent supported version, please open a new issue referencing this original issue.