New command API

[hugmanrique]

Introduces a new command framework that supports both legacy String[] args commands and 1.13+ Brigadier commands.

As discussed in #329 (comment) and #232, the current command framework lacks support for new command features. This PR adds 3 subinterfaces to Command: BrigadierCommand, LegacyCommand and RawCommand. All execution-related methods such as execute and hasPermission now take a single CommandInvocation argument that includes the command source and additional execution metadata such as the String[] args for legacy commands.

TODO:

• Decide on command execution context name. Candidates so far are CommandExecutionContext, CommandContext and CommandInvocation
• Move built-in commands to Brigadier
• Test all the things!!1!
  • Brigadier aliases as redirects (see Redirected command needs to have arguments Mojang/brigadier#46, do we need to create our own redirects system?)

[hugmanrique]

Alright, here's some usage examples:

import static com.mojang.brigadier.arguments.IntegerArgumentType.getInteger;
import static com.mojang.brigadier.arguments.IntegerArgumentType.integer;
import static com.mojang.brigadier.builder.RequiredArgumentBuilder.argument;
import static com.velocitypowered.api.command.BrigadierCommand.argumentBuilder;

import com.google.inject.Inject;
import com.mojang.brigadier.CommandDispatcher;
import com.velocitypowered.api.command.BrigadierCommand;
import com.velocitypowered.api.command.CommandManager;
import com.velocitypowered.api.command.CommandSource;
import com.velocitypowered.api.command.LegacyCommand;
import com.velocitypowered.api.event.Subscribe;
import com.velocitypowered.api.event.proxy.ProxyInitializeEvent;
import com.velocitypowered.api.plugin.Plugin;
import com.velocitypowered.api.proxy.ProxyServer;
import net.kyori.adventure.text.TextComponent;

@Plugin(id = "test-plugin")
public class TestPlugin {

    private final ProxyServer server;

    @Inject
    public TestPlugin(final ProxyServer server) {
        this.server = server;
    }

    @Subscribe
    public void onProxyInitialization(final ProxyInitializeEvent event) {
        CommandManager commandManager = server.getCommandManager();

        BrigadierCommand command = commandManager.brigadierBuilder().register(
            argumentBuilder("foo")
                .then(argument("bar", integer()))
                    .executes(c -> {
                        System.out.println("Bar is " + getInteger(c, "bar"));
                        return 1;
                    })
                .executes(c -> {
                    System.out.println("Called foo with no arguments");
                    return 1;
                })
        );

        LegacyCommand legacyCommand = commandManager.legacyBuilder().register(context -> {
            CommandSource source = context.source();
            String[] arguments = context.arguments();

            source.sendMessage(TextComponent.of("foo"));
        });
    }
}

[hugmanrique]

I reached a middle ground so current plugins can still directly register commands while adding a fluent command builder some may prefer (not many? do we even need it?).

[astei]

My API compatibility concerns are assuaged for now. In 2.0.0 we'll just outright force the explicit use of LegacyCommand or BrigadierCommand.

[alexstaeding]

CommandExecutionContext is a bit too wordy in my opinion. Why not just CommandContext?

[alexstaeding]

For the sake of conventions and consistency with the rest of the API, methods should start with a verb. I think CommandSource source(); should rather be CommandSource getSource();.

[hugmanrique]

It's common for immutable classes with getter accessors. See https://stackoverflow.com/a/1680515/4514467 for a more in depth answer.

[hugmanrique]

Alright, tested execution, tab complete and permission checks on 1.12.2, 1.16.1 and the console and everything looks fine. This is now ready for review.

One note that should be added to the docs: Brigadier commands should not use #requires to perform permission checks. This leads to the command being omitted from the parse tree and #execute throwing dispatcherUnknownCommand, meaning the execute is redirected to the backend server. Instead, they should use BrigadierCommand.Builder#permission. This also has the advantage of passing the entire CommandContext instead of just the CommandSource.

We now clone the entire children tree on redirect, no matter what. LiteralArgumentBuilder#redirect breaks the argument-less command even if it is manually added to the builder. LiteralCommandNode#createBuilder performs a shallow clone with the same alias. Instead, we use our own builder and add the destination children nodes to the builder. This avoids expensive copying on build, and the Available Commands packet size will be almost the same (if not better) thanks to compression.

[astei]

Just to clarify your comment opening this to further review, the Brigadier requires behavior is very similar to the current behavior with hasPermission.

[astei]

Going to look for some more eyes on this but LGTM

[Proximyst]

This looks terrific!

[astei]

Without any further comments left to make here, I will merge this in. Thanks for your contribution @hugmanrique, it is greatly appreciated.

[WesternPine]

That's huge, congrats @hugmanrique. Looks like a great implementation!