Brigadier Command Support

[Owen1212055]

Adds the ability for plugins to register their own brigadier commands by using a CommandBuilder.

Summary

• Deprecates CommandMap, moving all logic to a brig-backed map for legacy support. (All custom brig commands are wrapped in a vanilla command wrapper)
• Moves away from simplecommandmap dispatch for command logic, now brig will always be used.
• Exposes some "primitive" vanilla types, with players having the ability to add their own custom arguments using WrapperArgumentType.
• ALL bukkit commands will now be turned into brig nodes, no more syncCommands logic.
• Exposes Server#getCommandDispatcher, which returns a "mirror" of the NMS dispatcher.
  • Command nodes that are passed into this are converted into NMS types then passed into the NMS dispatcher.
  • This conversion prevents "nms object leaking"
• Our intention is not to make a built in command framework. Instead, suggesting that people use popular frameworks instead.

:wave: Hello rich syntax checking!

They show in the help menu and such just like normal plugins! (Description can be set in the command builder)

Indirectly fixes: https://github.com/PaperMC/Paper/issues/6293 https://github.com/PaperMC/Paper/issues/8255

---

Download the paperclip jar for this pull request: paper-8235.zip

[Owen1212055]

Would people rather have the ability to override nodes sent to the client? Or just simple argument types?

Because currently, you can use a WrapperArgumentType inorder to send the client a different argument than is on the server. So basically this allows you to make custom argument types whilst still sending it as a string argument to the player.

Would it be useful to extend this to nodes in general?

[Owen1212055]

Rebased and tested with plugins like FAWE. If people have any plugins that do hacky things with the command map, if you would be willing to link that would be awesome. I want to make sure bukkit commands still properly work here. Although they seem fine.

[TheFruxz]

Tested it with my on-demand command registration system, which uses the command map and manipulates + updates it during runtime, and it worked just fine! 👍

[Andre601]

One thing I wonder here is what "popular" libraries there are that allow brigardier implementation.

I can only think of Commodore by Lucko and obviously dealing with brigardier directly...

To be honest, I feel like a more native support for Commodore - at least for the file - would benefit the community here, as you otherwise would need to include yet another library for the brigardier support. Tho that's just my opinion on this.

[Owen1212055]

We want to be able to provide native very low level support. If we decide to add a library natively we might do that in the future, but this is supposed to just allow for custom brig commands to be registered using the api.

Any api may be much more in the future, this is bare bones.

[Owen1212055]

Initial review is ready.

I have added support for signed command arguments. This allows you to retrieve a SignedMessage if a Message argument is provided.

[Owen1212055]

All command node registration will be done in the vanilla dispatcher.

When adding custom nodes, those nodes will be "unwrapped" which converts them into an NMS version which is then registered to the dispatcher.

When retrieving nodes from the dispatcher it will check to see if there is a cached "unwrapped" version, where then it will be able to assume that this was created using API. However, if no unwrapped version exists (like for vanilla commands) it will use a "shadow" node which provides restricted access.

[Owen1212055]

Please feel free to test this branch and ensure that all legacy commands work the same. So, just load your server and ensure that all commands (with plugins) seem to be working correctly. This includes stuff like namespaced commands, etc.

There is now a jar attached to the PR body.

[Owen1212055]

Got rid of duplicate command execution logic.

Player#chat will now use the same chat logic and will no longer throw exceptions to match vanilla behavior. (Todo, exceptions are thrown rn)

Fixed issue with redirect flattening on shadow nodes.

Also fixed many issues with tab completion/command execution in general when using redirects (eg: /execute)

In general things seem to be running quite smoothly, but PLEASE feel free to test on your server to ensure all commands work properly.

[A248]

Calling Map.values() on the knownCommands map causes the server to hang. BukkitBrigForwardingMap.values appears to be responsible. Steps to reproduce using the plugin LibertyBans:

1. Setup server, install the latest release of LibertyBans.
2. Run /libertybans restart from the server console
3. View server hang, including thread dump, and subsequent termination by WatchDog.

Nothing hacky is happening here - the plugin is unregistering its commands by calling map.values().remove(command) for each of its command objects. However, this removal is happening during a command execution (though the command executed is not the command unregistered).

[Owen1212055]

Thanks for your report,

In general, now it'll prefer to lazily convert command nodes and properly support mutability.

[A248]

Okay. I now have another issue. Command unregistration remains unsuccessful.

The server no longer hangs when a command is unregistered using the method I described above. However, the command remains accessible. I suppose it was never truly unregistered.

[JorelAli]

Hello! I'm the creator of the CommandAPI, a Brigadier-based command API for Bukkit/Spigot/Paper that lets users define brigadier commands.

The CommandAPI has two main users: developers (people who make plugins) and non-developers (people who don't make plugins - typically server admins). To accommodate non-developers, the CommandAPI provides a "command conversion" feature which lets the CommandAPI register brigadier-compatible commands on behalf of plugins that were written using the standard Bukkit command framework. The CommandAPI's command conversion feature is not only compatible with the built-in commands (e.g. /execute), but it is also compatible with datapacks, allowing Bukkit command plugins to be used in datapacks.

This PR breaks the CommandAPI's command conversion feature. If I have a plugin SimpleBukkitPlugin with a command mycommand, the CommandAPI would register the command (using the default Minecraft namespace) /minecraft:mycommand. If you have a datapack with a function mynamespace:myfunction which contains mycommand, the CommandAPI would normally allow /function mynamespace:myfunction to execute mycommand with no issues, however this PR instead fails to load the function with this error message:

[14:33:38 ERROR]: Failed to load function mynamespace:myfunction
java.util.concurrent.CompletionException: java.lang.UnsupportedOperationException: Not supported yet.
        at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[?:?]
        at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320) ~[?:?]
        at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1770) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]
Caused by: java.lang.UnsupportedOperationException: Not supported yet.
        at net.minecraft.commands.CommandSource$1.getBukkitSender(CommandSource.java:29) ~[?:?]
        at net.minecraft.commands.CommandSourceStack.getBukkitSender(CommandSourceStack.java:404) ~[?:?]
        at io.papermc.paper.command.brigadier.bukkit.BukkitCommandNode.lambda$new$0(BukkitCommandNode.java:31) ~[paper-1.19.3.jar:git-Paper-"0cf8c8e"]
        at com.mojang.brigadier.tree.CommandNode.canUse(CommandNode.java:81) ~[paper-1.19.3.jar:git-Paper-"0cf8c8e"]
        at com.mojang.brigadier.CommandDispatcher.parseNodes(CommandDispatcher.java:359) ~[paper-1.19.3.jar:?]
        at com.mojang.brigadier.CommandDispatcher.parse(CommandDispatcher.java:349) ~[paper-1.19.3.jar:?]
        at net.minecraft.commands.CommandFunction.fromLines(CommandFunction.java:61) ~[?:?]
        at net.minecraft.server.ServerFunctionLibrary.lambda$reload$2(ServerFunctionLibrary.java:80) ~[?:?]
        at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768) ~[?:?]
        ... 3 more

Steps to reproduce

• Download SimpleBukkitPlugin-1.0.0.jar from here: https://github.com/JorelAli/SimpleBukkitPlugin/releases/tag/1.0.0

• Download CommandAPI-8.7.1.jar from here: https://github.com/JorelAli/CommandAPI/releases/tag/8.7.1

• Create plugins/CommandAPI/config.yml (if it doesn't exist) and populate it with the following:

  verbose-outputs: true
  silent-logs: false
  messages:
    missing-executor-implementation: "This command has no implementations for %s"
  create-dispatcher-json: false
  use-latest-nms-version: false
  plugins-to-convert:
    - SimpleBukkitPlugin:
      - mycommand
  other-commands-to-convert: []
  skip-sender-proxy: []
  

• Create the following default datapack, where myfunction.mcfunction contains mycommand

  server/
  ├── world/
  │   ├── advancements/
  │   ├── data/
  │   ├── datapacks/
  │   │   └── bukkit/
  │   │       ├── pack.mcmeta
  │   │       └── data/
  │   │           └── mynamespace/
  │   │               └── functions/
  │   │                   └── myfunction.mcfunction
  │   └── ...
  ├── world_nether/
  ├── world_the_end/
  ├── ...
  └── paper.jar
  

• Load paper with the paperclip jar from the first comment.

While I'm aware that the CommandAPI's command conversion feature is fairly hacky (ideally, it shouldn't exist and commands should work naturally, as they sort of do with this PR with /execute), it would be nice to see some support for Bukkit commands registered via datapacks, especially given that this PR breaks my solution to this problem.

[Owen1212055]

@A248 Please take a look now, I have improved this removal logic and tested it locally.

[Owen1212055]

@JorelAli This should be fixed now. Although there is still an error at the beginning server log, I am now able to run the command.

https://nightly.link/PaperMC/Paper/actions/artifacts/495487438.zip

[Owen1212055]

Unfortunately, I have discovered that there is some logic that we need to account for (reloading). This makes this PR a bit more complicated, so we will need to wait for https://github.com/PaperMC/Paper/pull/8108 and some api to allow us to properly have API to register commands (accounting for reloading).

Please still feel free to test this PR, as bukkit commands should still continue to work even after reloading. This is more about testing that bukkit commands are correctly migrated to brig ones.

People using nms

Please move away from MinecraftServer.vanillaCommandDispatcher, instead, use MinecraftServer#getCommands().

This change supports the internal dispatcher being reloaded. We will need to figure out some api in order to properly support re-registering commands under some kind of lifecycle api.

[A248]

Owen, I now encounter a CME because LibertyBans' /libertybans restart command, while executing, unregisters other command registrations.

https://pastebin.com/2eGsK9g4

If you'd like to, you may test out the plugin yourself, which is freely available on Github, SpigotMC, Modrinth, and Ore. I assure you there's no malware.

[Akiranya]

ALL bukkit commands will now be turned into brig nodes, no more syncCommands logic.

Does this turn all the existing Bukkit command executions into async? (correct me if i'm wrong 👀 i'm just confused and concerned about this change). I had a impression that if the command executions interact with the world/entities/blocks in an async context, it'll just throw.

[Machine-Maker]

No, syncCommands has nothing to do with threading. the sync just means "match up". syncCommands is a method that makes the bukkit command map and the brig dispatcher have the same commands, it "syncs" them up. We don't want two command "managers", just one, so no need for a method to sync the two.

[Rickyboy320]

Would people rather have the ability to override nodes sent to the client? Or just simple argument types?

Because currently, you can use a WrapperArgumentType inorder to send the client a different argument than is on the server. So basically this allows you to make custom argument types whilst still sending it as a string argument to the player.

Would it be useful to extend this to nodes in general?

I think so. For example, if I wanted to make a "LocationArgumentType" that parses x, y, z, yaw, pitch, world, I don't think that's representable in a singular vanilla ArgumentType. Greedy string is an option, but then the client no longer understands any arguments that come after. Instead, I would prefer to convert this argument type into three client argument types / nodes:

Vec3Argument -> RotationArgument -> StringArgumentType

and then end up resolving and parsing them to a Location type at resolution time, as part of the LocationArgumentType.

[Andre601]

One thing I feel like could be considered (Haven't read all the messages here tbh, so may already be mentioned) is a direct support for Lucko's commodore lib and the .commodore file format.

Like that Paper would check if such a file is present and when one is found apply it for a command matching the file's name... Tho that system could be a bit bad, so maybe have a way to define the files in the paper-plugin.yml to link to a command...? Or at the very least pack it with Paper (which I assume you do here?), so that plugin authors don't need to include it in their plugin and have an easier way to include it within the plugin... Idk. Just a random thing I was thinking about.

@lucko Tagging you, so you could have an input on this perhaps. Feel like you may have some ideas for this?

[Owen1212055]

May be a hot topic but I personally disagree with “skeleton” like definitions for commands. I feel like it’s easy to just have them be defined via some kind of event or what not. We’ll see tho.

[A248]

There is no reason to include yet another of the command framework libraries in the Minecraft plugin ecosystem in the server jar. It will simply become useless deadweight for the servers that do not use it and plugins that cannot rely on it because of an outdated bundled version. No doubt these command frameworks are useful, but please, there is no reason to pick favorites.

[Andre601]

There is no reason to include yet another of the command framework libraries in the Minecraft plugin ecosystem in the server jar. It will simply become useless deadweight for the servers that do not use it and plugins that cannot rely on it because of an outdated bundled version. No doubt these command frameworks are useful, but please, there is no reason to pick favorites.

If you refer to my suggestion of Commodore, I don't think it technically qualifies as a command framework in the usual sense, which for me means, registering AND handling commands. It handles/adds brigarider tab-completion for commands, but command-handling (and afaik even tab completion in general?) is still handled by you with whatever you use.

I also believe it's one of the more popular solutions here used by plugin devs (Tho I don't have numbers for this).

Also, to counter your argument a bit: Why does Paper then add yet another configuration library (Configurate) barely anyone uses if SnakeYAML is already present and used? Why do they add MiniMessage if a lot of plugins stick with the legacy colour code format? There are libraries that are used by a lot of people or that Paper sees a major benefit in (or other reasons I can't think of), so it's never bad to suggest something that could be included if it improves a plugin in the long run.

Like with MiniMessage would you now have a better, more understandable and versatile formatting option and with Configurate a much more open file manager that isn't limited to just YAML.

That's obviously just my points here, and I could missunderstand your message entirely, so feel free to clarify if needed.

[masmc05]

Why does Paper then add yet another configuration library (Configurate) barely anyone uses if SnakeYAML is already present and used?

because paper uses it for its config?

Why do they add MiniMessage if a lot of plugins stick with the legacy colour code format?

again, paper itself uses it, but also, it's because it's related to the api that was integrated, i think adventure is too big and native support is a lot more convenient to ignore the possibility to add this in paper

Like with MiniMessage would you now have a better, more understandable and versatile formatting option and with Configurate a much more open file manager that isn't limited to just YAML.

just add it to your plugin, i see no issue or improvement that intergrating this into paper would give, some would still see ACF, cloud etc as better solutions, and i see no reason for this library to be used by paper itself, as it can register commands directly into nms if they would need that

[masmc05]

also

Our intention is not to make a built in command framework. Instead, suggesting that people use popular frameworks instead.

[Owen1212055]

@A248 After testing with libertybans, it appears to all work fine. However, if you'd like to give another second test that would be greatly appreciated.

[tal5]

Already posted this on Discord, but thought I might as well post here for future reference/tracking:

There seem to be inconsistencies with handling spaces when using bukkit's old command system (basically, it ignores/trims them): I tested typing /test-command Tab (with the space), and With normal Paper:

[17:08:36 INFO]: [test] Tab complete for: test-command; Args: T (1)
[17:08:38 INFO]: [test] Tab complete for: test-command; Args: Ta (1)
[17:08:38 INFO]: [test] Tab complete for: test-command; Args: Tab (1)
[17:08:39 INFO]: [test] Tab complete for: test-command; Args: Tab,  (2)

With this PR:

[17:11:07 INFO]: [test] Tab complete for: test-command; Args: T (1)
[17:11:09 INFO]: [test] Tab complete for: test-command; Args: Ta (1)
[17:11:10 INFO]: [test] Tab complete for: test-command; Args: Tab (1)
[17:11:10 INFO]: [test] Tab complete for: test-command; Args: Tab (1)

Testing code

[Owen1212055]

Thank you @tal5 this should be resolved.

[Malfrador]

I am getting wrong tab completions with this PR and tab completions created using the Bukkit TabCompleter. The first tab completion argument shows up again instead of the second one, the second argument does not show up at all.

Paper without this PR: With this PR:

Tab completions are just added using TabCompleter#onTabComplete (Code here)