[Suggestion/Discussion] Brigadier support for command declaration

[marcbal]

I am thinking about doing a pull request for plugin developers to use the Brigadier API for the command declaration, and I am facing some question I would like to share with you before starting to implement something.

1. Is it a good idea to expose to plugin developers the Brigadier API itself, or do we have to implement a BungeeCord wrapper for it ?

I don’t see a problem for this, this is not like exposing NMS or OBC methods in the Bukkit API, and Brigadier is open source with MIT license.

2. How the API (and the implementation behind) could be modified to allow custom command declaration ?

(this part is edited to take suggestions into account)

2.1. Add a field in Command object and set it optionally via constructor

A field of type LiteralCommandNode could be added in the Command class, that is null by default and can be set via constructor. It could also be set via a setter, but the command structure is not initially intended to be modified on the fly, since the data is only sent to the client once.

When the method below iterate over the commands to add it to the Commands packet, if the new field is set to null, a dummy LiteralCommandNote is used instead.

https://github.com/SpigotMC/BungeeCord/blob/f1c32f84f46589632d7721d8de87d5589ef8e6a6/proxy/src/main/java/net/md_5/bungee/connection/DownstreamBridge.java#L584-L608

2.2. Create a CommandsDeclareEvent

This event is expected to be called by DownstreamBridge#handle(Commands). The event should contains at least the player (to check permissions) targetted by the packet and the server which the packet comes from. The event will also contains a reference to the root command node. Bungee will then take care of all the remaining command that was not included during the event execution, by adding the dummy LiteralCommandNode.

2.-. Maybe a better idea ?

Which method is the best, or is there a better way ?

(This suggestion is related to #2669 )

[Janmm14]

Couldn't it just be one extra field in the Command class you can optionally set via constructor? Also I never used or looked at brigardier, but maybe bungee could add some utility methods to shortcut/ease the creation if that's not straightforward with brigardier. Maybe some Builder class with method chaining might come in handy if its going to be a constructor parameter.

I thought to add it as a constructor parameter as its making it more immutable, as changing stuff after it got sent requires bungee to resend the whole list.

An event might still be a good option to add. But it should be player-specific and contain a list of commands to send, maybe wrapped in some class, so plugins can see the origin of the command (bungee or spigot).

[marcbal]

Adding a field in the Command class was one of my idea (but including it in a constructor as optional is a good idea). Brigadier can’t be use in a standalone way, we have to deal with Bungeecord to communicate with the client to send the data (this is why we have to modify DownstreamBridge#handle(Commands))

~The idea is just to send the data to the client by including the data to the packet sent through the proxy. So I don’t plan to send any extra packet.~ Ok I didn’t understand your answer in the first place. Yes It’s finally a better idea to include it as an optional parameter for constructor, as it is not intended to be updated on the fly.

The event is expected to be fired by DownstreamBridge#handle(Commands), that is executed when the proxy receive the Commands packet. This packet comes from a server and goes to a specific player so we can include the two informations in the event :) For the command data, we could put the reference to the RootCommandNode and let the plugins access it with a getRoot() method (like the Commands#getRoot()).

[Janmm14]

I was only talking about the API part as well and not about possible implementations.

[marcbal]

Also, Brigadier already provides Builder classes, so there is no need for utility methods if the Brigadier API is accessible for the plugin developer.

I‘m gonna make a pull request with the optional parameter in the Bungee API and expose the Brigadier API ;)

[MrIvanPlays]

An event would be best suitable to define brigadier completions

[marcbal]

I think we should discuss the pros and cons of both options we have :

Option 1: optionally set via Command’s contructor

Pros

• follows the principle of immutability of command structure
• the command structure is only constructed once
• it makes more sense to declare the Brigadier structure of your command in the source file where you also implement your command execution and tab-completion.

Cons

• No easy control of player specific command structure (it can still be controlled using Brigadier’s .requires(Predicate) method)

Option 2: Event fired by DownstreamBridge#handle(Commands)

Pros

• Fine control of which player receive which command structure
• Ability to alter the command structure sent by the downstream server.

Cons

• Rebuild the command structure each time a player join a server
• Need, for the plugin developer, to implement an event listener that takes care of all the command structure of the plugin, or to implement an event listener for each command.

[Janmm14]

Maybe a combination? Default option is to overload Command constructor and older plugins can be improved by modifications done by new specialized plugins using the even. Also would suggest to have a method causing to resend the command list to the client to manually trigger the events again.

Docs should state clearly however that the constructor should be prioritized due to it being more performant.

[MrIvanPlays]

I don't think we should fill the constructor with junk. It already has lots of junk: name, permission, aliases

[Janmm14]

"junk"?

[MrIvanPlays]

"junk"?

yes. there are lot more suitable options to put these in. I'm not going to comment the design choices made 7 years ago.

[Janmm14]

"junk"?

yes. there are lot more suitable options to put these in. I'm not going to comment the design choices made 7 years ago.

uff

[marcbal]

A combination is not a bad idea, as you said, it gives more flexibility and control to the plugin développer the command sent to the client.

A method to resend the command structure would need to cache the data sent by the server. It could be useful to resend the data structure after a reload of a plugin or when some permissions change.

I don't see the bad thing about adding a new constructor. It's not like there are already a lot of them. We will just add a new one with the new parameter, and keep the old one for compatibility. Also, the pull request is more likely to be accepted if we follow the design choice made for Bungee, even if they may be bad from some point of view.

[MrIvanPlays]

If we're doing events, we would need 2:

• 1 for when bungee sends commands, that would be a TargetedEvent
• 1 for when bungee sends suggestions (completions) using brigadier

The most flexible way for implementing brigadier in my opinion is events, as it is the cleanest way of doing it and it gives more flexibility (it would be the same code for handling for all your projects, so you won't need to rewrite it every time you're implementing it)

[marcbal]

The first event you listed is the one we are talking about since the beginning. Good idea to extends TargetedEvent, I didn’t though about it !

The second event you’re suggesting would be redundant with TabCompleteEvent. If we replace this event with a new one using Brigadier, we would have to uses 2 command dispatcher in Bungeecord: one to dispatch the command execution (BungeeCord implementation), the other one to handle the tab-completion (Brigadier implementation).

Ok so maybe it’s a better idea to implement the event(s) first.

I think in a far future (not so far, I hope), BungeeCord command dispatcher could be replaced with Brigadier, so all command execution, tab-completion and command declaration will be managed by the same code, and consistent with last Minecraft version ! (but this is not the purpose of this discussion)

[marcbal]

Ok so I’m starting to work on an implementation of the event, and I don’t know if the event must be called before Bungee adds it’s own stuff in the packet, or after.

If the event is called before, the plugins can declare their own command structure, and then bungee will add the remaining command (as it already do in the current code (see first post)), but the event wont have the ability to control what Bungee adds, after the event call.

If the event is called after, the Brigadier API don’t currently allow removal of command node from a command structure (that is, for a specific node, there is no method to remove a child node) so anything that is added by bungee (and the structure declared by the downstream server) can’t be removed.

However, while writing this comment, I’m thinking that the event class could have some methods that would take care of removing the structure of a specified command using reflexion, like it’s done in commodore: https://github.com/lucko/commodore/blob/945237eb8fa957ba20f3c5d5d38216fee1b38e31/src/main/java/me/lucko/commodore/CommodoreImpl.java#L213-L223

EDIT : do you think this is ok to keep raw generic type in the Bungee API or do we have to add type argument to Brigadier class (that are currently raw in BungeeCord’s code) ?

[Mystiflow]

What if you call the event before but add a Callback/Future method within the class that plugins can hook into after Bungee deals with its' commands?

[marcbal]

the Callback system seems to be used only by AsyncEvent, that is not the case here. Maybe 2 events, one before and one after ?

Anyway, for the "after" event, we must have the possibility to remove command node in the Brigadier structure (only doable with reflexivity), if we want this latter event to be useful anyway. The thing is that I don’t know if this is ok to use reflexivity in Bungeecord to alter the command structure of brigadier. If we can do this, calling the event after is sufficient, since the plugin devs can alter all the command structure, including Bungee’s.

[MrIvanPlays]

Why do we need unregistering? Unregistering is a bad practice. If you want something disabled, just don't enable it at first place or don't implement it.

EDIT: Either way, if we want command to not be sent, we could have something like CommandSendEvent which is called before the brigadier exposal event (where you modify all the juicy brigadier stuff)

[marcbal]

This is because all commands registered with Bukkit (on server side) and with Bungee API (on proxy side) declare a dummy command structure in the Commands packet (a literal node for the command name, followed by a argument <args> of type StringArgumentType.greedyString()). We have to remove this structure before adding a custom one, or the argument <args> will disturb the intended client side bahaviour. Technically, the CommandNode#addChild(CommandNode) merge the children with the same name together, instead of replacing everything.

I understand that removing a command structure is not the intended behaviour, and probably this is why there is no #removeChild() method in Brigadier. But since the goal of the event is to modify an already existing command tree that we don’t have full control when it is created (because it is generated by the downstream server), I find it useful to allow command removing or full replacement, at least.

EDIT : actually I didn’t intent to allow "remove a command" (the permissions are there for this purpose), but this functionnality was just intended as an utility to cleanup old data for a specific command before adding new stuff for that command.

[MrIvanPlays]

Or we can just have a setter method for the node except trying to get our heads around unregistering.

[marcbal]

I made a pull request with a new event: #2847 :)