Store backend command tree per server connection to allow on-demand resending of command set

[WouterGritter]

Inspired by:

• GemstoneGG@137e6da
• GemstoneGG@6b63454

With this PR, plugins may dynamically decide to add commands to the client's command tree by triggering Player#sendAvailableCommands and listening for the PlayerAvailableCommandsEvent that gets fired as a result of it.

How much memory does this PR consume per player?

Let's assume 1.000 commands per player. A conservative lower bound, and a complete but educated-ish guess, would be about ~64 bytes per command, giving 64kb per player (for 500 players, this is 32MB)

Take this with a giant grain of salt, but I had Claude Opus 4.7 have a go at estimating the memory footprint this PR brings with it.
Prompt: Investigate exactly how much memory 'RootCommandNode<CommandSource> backendCommandsNode' in 'BackendPlaySessionHandler' consumes per player for 1000 commands with typical string lengths.
After 7 minutes of "brewing", it estimated 274-331 bytes per command, giving ~300kb per player. This would be a more substantial 150MB at 500 players.

If this does become a feature of Velocity, we may consider hiding the velocity:callback command from the command tree unless a plugin registers a click-callback through Adventure (vanilla Velocity never uses this feature; this command only really needs to be sent to the client if a plugin does actually make use of this feature). See: GemstoneGG#866

[WouterGritter]

I'm not too sure if we need to return a CompletableFuture here. This doesn't get returned anywhere else in the Player interface, but at the same time, when this is completed it does guarantee that all PlayerAvailableCommandsEvents have been fired and the packet is on its way to the player.

Could this be useful for plugins? Or should we just return void and rely on plugins listening for this even to figure out that the packet is (about to be) on it's way, as a plugin calling this method, is most likely also manipulating this event?

[R00tB33rMan]

Approving in its current state - changes may need to be made for other considerations by authors, of course

[Timongcraft]

My initial thought was to put this behind add a feature flag system, which would need to be enabled by plugins on init but...

[WouterGritter]

My initial thought was to put this behind add a feature flag system, which would need to be enabled by plugins on init but...

I mean we could do this intuitively.

• get rid of Player#sendAvailableCommands
• introduce ProxyServer#getAvailableCommandsService

interface AvailableCommandsService {
  void sendAvailableCommands(Player player);
}

Plugins would need to obtain the AvailableCommandsService which would trigger the command tree to now be saved.

The issue is however that plugins really need to do this on startup, or they'll end up missing the backend command trees from players that are already connected.

Though I don't really like this. Seems complex and unnecessary to avoid a couple megabytes of ram, realistically.
Maybe an opt-out system flag would be better. Having memory issues? Know that no plugins make use of this feature? Disable it.

[3add]

Wouldn't it need to be renamed from RootBeer?

Edit: lol I'm an idiot I read RootCommandNode as RootBeerCommandNode as I thought you were literally merging from CTD

[Timongcraft]

My initial thought was to put this behind add a feature flag system, which would need to be enabled by plugins on init but...

I mean we could do this intuitively.

• get rid of Player#sendAvailableCommands
• introduce ProxyServer#getAvailableCommandsService

interface AvailableCommandsService {
  void sendAvailableCommands(Player player);
}

Plugins would need to obtain the AvailableCommandsService which would trigger the command tree to now be saved.

The issue is however that plugins really need to do this on startup, or they'll end up missing the backend command trees from players that are already connected.

Though I don't really like this. Seems complex and unnecessary to avoid a couple megabytes of ram, realistically. Maybe an opt-out system flag would be better. Having memory issues? Know that no plugins make use of this feature? Disable it.

We could also just expose the AvailableCommandsService in the proxy init event but that would seriously obscure or hide this feature.
Alternatively we could keep the method as is and throw if it was not enabled by some feature system on init.

[WouterGritter]

Alternatively we could keep the method as is and throw if it was not enabled by some feature system on init.

I don't think we have any feature that works like this right? If we are going to set a precedent for "resource intensive feature that must be opt-in in an elegant way" we need to think of a proper solution, one that would also work for future features like this.

An alternative for this specific PR could be to store the raw packet instead of the decoded command tree. Decode the packet only when sendAvailableCommands is called. Trade a bit of CPU time (only when this feature is used) for a lower memory footprint. I'm kinda leaning towards this; I might do some testing to see how much this would reduce memory usage in comparison.

[Timongcraft]

Alternatively we could keep the method as is and throw if it was not enabled by some feature system on init.

I don't think we have any feature that works like this right? If we are going to set a precedent for "resource intensive feature that must be opt-in in an elegant way" we need to think of a proper solution, one that would also work for future features like this.

Sure, though we also should think about the users of this API and if it is hidden behind some 'obscure' mechanism they might not find it

[WouterGritter]

Sure, though we also should think about the users of this API and if it is hidden behind some 'obscure' mechanism they might not find it

I mean, if we don't want to hide this API, we could keep Player#sendAvailableCommands and throw with an exception saying You must call ProxyInitializeEvent#enableFeature(FeatureFlags.SEND_AVAILABLE_COMMANDS) to make use of this feature!

This should never really produce an exception at runtime, because plugin developers will (should) catch this during development.

Alternatively we could log an error or warning and have it behave like a NOP instead of throwing.

[WouterGritter]

An alternative for this specific PR could be to store the raw packet instead of the decoded command tree. Decode the packet only when sendAvailableCommands is called. Trade a bit of CPU time (only when this feature is used) for a lower memory footprint. I'm kinda leaning towards this; I might do some testing to see how much this would reduce memory usage in comparison.

This is really cursed. AvailableCommandsPacket needs to store a byte[] encoded and perform encoding during the constructor, decoding during the getRootNode getter. Reading/writing the bytes into this encoded field during encode() and decode(). IMO this isn't worth it, it deviates from how other packets do things so much, but it will reduce the memory footprint by a lot. Curious what other maintainers think of this approach.

[Timongcraft]

Sure, though we also should think about the users of this API and if it is hidden behind some 'obscure' mechanism they might not find it

I mean, if we don't want to hide this API, we could keep Player#sendAvailableCommands and throw with an exception saying You must call ProxyInitializeEvent#enableFeature(FeatureFlags.SEND_AVAILABLE_COMMANDS) to make use of this feature!

This should never really produce an exception at runtime, because plugin developers will (should) catch this during development.

Well this is what I meant.

Alternatively we could log an error or warning and have it behave like a NOP instead of throwing.

Well I think this could be annoying, and idk but I see no benefit with this option