Feature: Add API to allow native Brigadier node merging behavior when registering BrigadierCommand

[willkroboth]

Summary

This issue comes from a discussion on the Velocity discord, though hopefully all the ideas from over there will be present here.

When LiteralArgumentBuilder nodes that have the same name are registered to a Brigadier CommandDispatcher, they are merged together. However, when you wrap those nodes in Velocity's BrigadierCommand and register them using CommandManager, one node overrides the other, so only one is executable.

BrigadierCommands overriding each other when they have the same name is currently intended behavior. However, it would be nice if there was an API method that made it possible to use the CommandDispatcher logic where the commands are merged together.

Also, on a bit of a side note, it would probably be useful if Velocity added this behavior to their documentation. Coming from a Brigadier standpoint, I expected the commands to be merged, so I was very confused when that didn't happen. That confusion could be avoided if The Command API page mentioned when a command is registered, previous commands with the same name are removed.

How exactly is Brigadier and Velocity's behavior different?

Click to expand

When Brigadier registers nodes that have the same name, it merges them together. On the other hand, Velocity will unregister the first node and replace it with the second. To see this in action, consider this Brigadier code (no Velocity other than CommandSource):

LiteralArgumentBuilder<CommandSource> literal1 = LiteralArgumentBuilder.<CommandSource>literal("test")
        .executes(context -> {
            context.getSource().sendMessage(Component.text("This is the test1 node"));
            return Command.SINGLE_SUCCESS;
        });

LiteralArgumentBuilder<CommandSource> literal2 = LiteralArgumentBuilder.<CommandSource>literal("test")
        .then(RequiredArgumentBuilder.<CommandSource, String>argument("argument", StringArgumentType.word())
                .executes(context -> {
                    String arg = context.getArgument("argument", String.class);
                    context.getSource().sendMessage(Component.text("This is the test2 node"));
                    context.getSource().sendMessage(Component.text(arg));
                    return Command.SINGLE_SUCCESS;
                })
        );

CommandDispatcher<CommandSource> dispatcher = new CommandDispatcher<>();

dispatcher.register(literal1);
dispatcher.register(literal2);

This code defines two LiteralArgumentBuilder nodes -- literal1 and literal2 -- which represent the commands /test and /test <argument> respectively. These nodes are both registered to a Brigadier CommandDispatcher using the CommandDispatcher#register method. After running this code, the CommandDispatcher's structure looks like this:

{
  "type": "root",
  "children": {
    "test": {
      "type": "literal",
      "children": {
        "argument": {
          "type": "argument",
          "argumentType": "com.mojang.brigadier.arguments.StringArgumentType",
          "executable": true
        }
      },
      "executable": true
    }
  }
}

What is important to notice is that both the test child and the argument child are marked "executable": true. Even though the definitions of these two paths were registered separately, CommandDispatcher#register merged them together so that both paths were executable.

Now, consider this code using Velocity's BrigadierCommand and CommandManager:

BrigadierCommand literal1 = new BrigadierCommand(LiteralArgumentBuilder.<CommandSource>literal("test")
        .executes(context -> {
            context.getSource().sendMessage(Component.text("This is the test1 node"));
            return Command.SINGLE_SUCCESS;
        })
);

BrigadierCommand literal2 = new BrigadierCommand(LiteralArgumentBuilder.<CommandSource>literal("test")
        .then(RequiredArgumentBuilder.<CommandSource, String>argument("argument", StringArgumentType.word())
                .executes(context -> {
                    String arg = context.getArgument("argument", String.class);
                    context.getSource().sendMessage(Component.text("This is the test2 node"));
                    context.getSource().sendMessage(Component.text(arg));
                    return Command.SINGLE_SUCCESS;
                })
        )
);

CommandManager commandManager = server.getCommandManager();

commandManager.register(literal1);
commandManager.register(literal2);

This code uses the same LiteralArgumentBuilder nodes, it just wraps them in BrigadierCommand, and uses CommandManager#register to register them to Velocity's CommandDispatcher. However, this code does not produce the same structure as before. Here is the structure that results from this code:

{
  "type": "root",
  "children": {
    "test": {
      "type": "literal",
      "children": {
        "argument": {
          "type": "argument",
          "argumentType": "com.mojang.brigadier.arguments.StringArgumentType",
          "executable": true
        }
      }
    }
  }
}

Here, only the argument child is executable, and test is not executable. You can see this in-game. Only the /test <argument> command works, not /test.

This is the intended behavior. When Velocity registers a LiteralCommandNode, it uses this code:

abstract class AbstractCommandRegistrar<T extends Command> implements CommandRegistrar<T> {
  private final @GuardedBy("lock") RootCommandNode<CommandSource> root;
  private final Lock lock;

  protected void register(final LiteralCommandNode<CommandSource> node) {
    lock.lock();
    try {
      // Registration overrides previous aliased command
      root.removeChildByName(node.getName());
      root.addChild(node);
    } finally {
      lock.unlock();
    }
  }
}

Compare that to CommandDispatcher:

public class CommandDispatcher<S> {
    private final RootCommandNode<S> root;

    public LiteralCommandNode<S> register(final LiteralArgumentBuilder<S> command) {
        final LiteralCommandNode<S> build = command.build();
        root.addChild(build);
        return build;
    }
}

Before Velocity registers the /test <argument> node, it uses removeChildByName to get rid of any nodes with the test name, hence why /test becomes unexecutable. In contrast, CommandDispatcher immediately uses addChild, which merges /test <argument> with the old /test node. So, it is not a bug that these behaviors are different. That is simply how the methods work.

Why do I want to register separate LiteralArgumentBuilder nodes that have the same name?

Click to expand

The previous section explained why the following Velocity code results with only the /test <argument> command executable, even though a /test command is also registered.

BrigadierCommand literal1 = new BrigadierCommand(LiteralArgumentBuilder.<CommandSource>literal("test")
        .executes(context -> {
            context.getSource().sendMessage(Component.text("This is the test1 node"));
            return Command.SINGLE_SUCCESS;
        })
);

BrigadierCommand literal2 = new BrigadierCommand(LiteralArgumentBuilder.<CommandSource>literal("test")
        .then(RequiredArgumentBuilder.<CommandSource, String>argument("argument", StringArgumentType.word())
                .executes(context -> {
                    String arg = context.getArgument("argument", String.class);
                    context.getSource().sendMessage(Component.text("This is the test2 node"));
                    context.getSource().sendMessage(Component.text(arg));
                    return Command.SINGLE_SUCCESS;
                })
        )
);

CommandManager commandManager = server.getCommandManager();

commandManager.register(literal1);
commandManager.register(literal2);

This is intended behavior, and there is a perfectly reasonable alternative method that makes both /test and /test <argument> work. Here's that code:

BrigadierCommand literal1 = new BrigadierCommand(LiteralArgumentBuilder.<CommandSource>literal("test")
        .executes(context -> {
            context.getSource().sendMessage(Component.text("This is the test1 node"));
            return Command.SINGLE_SUCCESS;
        })
        .then(RequiredArgumentBuilder.<CommandSource, String>argument("argument", StringArgumentType.word())
                .executes(context -> {
                    String arg = context.getArgument("argument", String.class);
                    context.getSource().sendMessage(Component.text("This is the test2 node"));
                    context.getSource().sendMessage(Component.text(arg));
                    return Command.SINGLE_SUCCESS;
                })
        )
);

CommandManager commandManager = server.getCommandManager();

commandManager.register(literal1);

Instead of registering the /test and /test <argument> commands separately, Brigadier allows you to merge them together into a single LiteralArgumentBuilder. Velocity can then register this as a single BrigadierCommand, and there is no problem registering these two command paths.

So, a good question is, "Who would ever register separate LiteralArgumentBuilder nodes that have the same name?" Since I'm creating this issue, the answer is obviously me :). I'm currently working on porting a Spigot plugin called CommandAPI to Velocity. The CommandAPI allows developers to create and register Brigadier commands. For example, the /test and /test <argument> commands could be created like so:

new CommandAPICommand("test")
        .executes((sender, arguments) -> {
            sender.sendMessage(Component.text("This is the test1 node"));
        })
        .register();

new CommandAPICommand("test")
        .withArguments(new StringArgument("argument"))
        .executes((sender, arguments) -> {
            String arg = arguments.getUnchecked("argument");
            
            sender.sendMessage(Component.text("This is the test2 node"));
            sender.sendMessage(Component.text(arg));
        })
        .register();

The CommandAPI processes these CommandAPICommand objects and turns them into LiteralArgumentBuilder nodes. Note that two command paths are declared separately, hence why I want to register separate LiteralArgumentBuilder nodes that have the same name.

Alternate solutions (Velocity doesn't change)

Click to expand

This section describes alternate solutions to my problem where Velocity keeps the same behavior.

First, my problem. Once the CommandAPICommand objects are turned into LiteralArgumentBuilder nodes, the registerCommandNode method is called. Here is what that originally looked like:

public LiteralCommandNode<CommandSource> registerCommandNode(LiteralArgumentBuilder<CommandSource> node) {
    BrigadierCommand command = new BrigadierCommand(node);
    commandManager.register(command);
    return command.getNode();
}

This doesn't work when two nodes with the same name are passed, since when registering a BrigadierCommand with the same name, Velocity will unregister old nodes with that name, as discussed.

One solution I thought of looks like this:

CommandDispatcher<CommandSource> mergeDispatcher = new CommandDispatcher<>();

public LiteralCommandNode<CommandSource> registerCommandNode(LiteralArgumentBuilder<CommandSource> node) {
	LiteralCommandNode<CommandSource> builtNode = mergeDispatcher.register(node);

	LiteralCommandNode<CommandSource> mergedNode = (LiteralCommandNode<CommandSource>) mergeDispatcher.getRoot().getChild(builtNode.getName());
	BrigadierCommand command = new BrigadierCommand(mergedNode);
	commandManager.register(command);

	return builtNode;
}

A separate CommandDispatcher mergeDispatcher keeps track of all the nodes registered by the CommandAPI. If there is ever a node with the same name, the register method of mergeDispatcher is easily accessible to get the merging behavior. I can then take the result of merging the nodes together, and give that to BrigadierCommand instead. Even when Velocity unregisters the old /test command, I have the merged version, so /test and /test <argument> are registered correctly.

One downside of this method is that it keeps track of a duplicate CommandDispatcher structure for every command registered with the CommandAPI. This seems unnecessary when you could just merge the nodes in Velocity's CommandDispatcher. That's the second alternative solution:

private CommandDispatcher<CommandSource> dispatcher;

{
    commandManager = proxyServer.getCommandManager();

    Field dispatcherField = commandManager.getClass().getDeclaredField("dispatcher");
    try {
        dispatcher = (CommandDispatcher<CommandSource>)dispatcherField.get(commandManager);
    } catch(Exception ignored) {
        logError("Could not access Velocity's Brigadier CommandDispatcher");
    }
}

public LiteralCommandNode<CommandSource> registerCommandNode(LiteralArgumentBuilder<CommandSource> node) {
    return dispatcher.register(node);
}

I want to merge the command directly into Velocity's CommandDispatcher, so just call the register method on Velocity's CommandDispatcher. The problem with this one is that you need reflection to access Velocity's CommandDispatcher. This is the solution I'm currently using, but it would be nice if reflection wasn't needed.

Alternate solutions (Velocity does change) (Why this is a feature request)

Click to expand

I think it would be best if something like my original solution worked out:

public LiteralCommandNode<CommandSource> registerCommandNode(LiteralArgumentBuilder<CommandSource> node) {
    BrigadierCommand command = new BrigadierCommand(node);
    commandManager.register(command);
    return command.getNode();
}

Of course, as discussed, it doesn't currently work, so Velocity would need to change somehow.

My first proposal is to never remove the old nodes when registering new commands. That would change this code to be this:

abstract class AbstractCommandRegistrar<T extends Command> implements CommandRegistrar<T> {
  private final @GuardedBy("lock") RootCommandNode<CommandSource> root;
  private final Lock lock;

  protected void register(final LiteralCommandNode<CommandSource> node) {
    lock.lock();
    try {
      root.addChild(node);
    } finally {
      lock.unlock();
    }
  }
}

Here, the call to removeChildByName is gone, avoiding the issue where the /test command is unregistered before the /test <argument> command is registered.

There is some concern that this would cause problems when registering a SimpleCommand or RawCommand. I'm not certain of the consequences, but I think it would be okay. SimpleCommand and RawCommand both have a single greedy argument. In my experience, when Brigadier registers two nodes in the same spot that accept the same input, the node that is registered first always gets precedence. So, in this scenario, I think registering, for instance, two RawCommand with the same name would mean only the first version would be runnable. This is reversed to the current behavior (where the second version would be runnable), but since it isn't documented I don't expect anyone depends on it. Again, I'm not certain this is what actually would happen, but I think it's worth looking into.

If there are problems with RawCommand and SimpleCommand, an alternative would be to provide an API method that allows this behavior only for BrigadierCommand. Maybe a CommandManager#register(BrigadierCommand command, boolean merge) method that could be used like so:

public LiteralCommandNode<CommandSource> registerCommandNode(LiteralArgumentBuilder<CommandSource> node) {
    BrigadierCommand command = new BrigadierCommand(node).merge(true);
    commandManager.register(command);
    return command.getNode();
}

The API method can really be anything IMO. I would appreciate any way to be able to use the native Brigadier merging behavior, and I think it would be a good addition to Velocity's API.

[Icosider]

I also ran into this problem, only when developing the same commands on Forge and Velocity