JorelAli / CommandAPI

A Bukkit/Spigot API for the command UI introduced in Minecraft 1.13
https://commandapi.jorel.dev
MIT License
529 stars 67 forks source link

The CommandAPI should use MinecraftServer#getCommands instead of MinecraftServer.vanillaCommandDispatcher #406

Open JorelAli opened 1 year ago

JorelAli commented 1 year ago

Description

As per this comment:

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.

Expected code

Replace the use of MINECRAFT_SERVER.vanillaCommandDispatcher in NMS implementations of NMS#getBrigadierDispatcher https://github.com/JorelAli/CommandAPI/blob/2efd3a3ffdee797ab8b2a4b12784c9e5741d197a/commandapi-nms/commandapi-1.19.3/src/main/java/dev/jorel/commandapi/nms/NMS_1_19_3_R2.java#L395-L398

with MINECRAFT_SERVER.getCommands():

@Override
public final com.mojang.brigadier.CommandDispatcher<CommandSourceStack> getBrigadierDispatcher() {
    return MINECRAFT_SERVER.getCommands().getDispatcher();
}

Extra details

No response

willkroboth commented 1 year ago

I notice that comment is from a Paper PR. If this change was applied, would the CommandAPI still work on Spigot? If not, I suppose we would just need to migrate calls to getBrigadierDispatcher over to PaperImplementations.

Also, does this change apply to Paper right now, or only after that PR is merged?

JorelAli commented 1 year ago

I notice that comment is from a Paper PR. If this change was applied, would the CommandAPI still work on Spigot? If not, I suppose we would just need to migrate calls to getBrigadierDispatcher over to PaperImplementations.

Also, does this change apply to Paper right now, or only after that PR is merged?

Yes, this will still work on Spigot. The MinecraftServer#getCommands method exists in Spigot and has always existed since 1.13. There isn't any particular reason why the CommandAPI should not be using this method, other than the fact it hasn't been tested and testing will be required to ensure it's a viable solution (i.e. make sure everything works as it should!)