Delegate Bukkit command registering logic to CommandRegistrationStrategy - #560
Conversation
Sorry, something went wrong.
…tegy` Just a rough example implementation `NMS#getResourceDispatcher` replaced by `NMS#createCommandRegistrationStrategy`, which creates a `SpigotCommandRegistration` implementation using the resources dispatcher Alternate fix for #554. If we're on Paper-1.20.6-65, a `PaperCommandRegistration` implementation is used instead. This can be used to properly handle the internal changes made by PaperMC/Paper#8235.
…cher` It seems the underlying field accessed by running `this.<MinecraftServer>getMinecraftServer().getCommands().getDispatcher()` changes at some point. So, calling this during load actually results in just another copy of the `brigadierDispatcher`. However, if we call it later, it correctly returns the `resourcesDispatcher`.
…mands are unregistered Avoids reintroducing unregistered commands after `/minecraft:reload`
There was a problem hiding this comment.
What's our standpoint on those * imports?
Do we want them? I personally tried to avoid them in general but we can also leave them like they are if we think they're fine.
Sorry, something went wrong.
| public PaperCommandRegistration( | ||
| Supplier<CommandDispatcher<Source>> getBrigadierDispatcher, Predicate<CommandNode<Source>> isBukkitCommand | ||
| ) { |
There was a problem hiding this comment.
This is some me kind of nitpick but can we have this on one line?
Sorry, something went wrong.
There was a problem hiding this comment.
Fair enough. I think I expected to need to add more parameters (like SpigotCommandRegistration), but just two is a reasonable length.
Sorry, something went wrong.
We don't care. They don't change the output of the compiled code, the only downside to them is it's harder for humans to statically analyse code by staring at it in something other than an IDE, but it's 2024 and that's basically irrelevant. |
Sorry, something went wrong.
DerEchtePilz
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
This PR moves a lot of the current logic for registering commands on Bukkit into the class SpigotCommandRegistration. This allows an alternate implementation of the new CommandRegistrationStrategy interface to be loaded when internal changes from PaperMC/Paper#8235 (Paper-1.20.6-65 and later) are present. The CommandRegistrationStrategy is created by the method NMS#createCommandRegistrationStrategy.
This is a more permanent fix than #555 and resolves these issues when running on Paper-1.20.65 and later:
The implementation can still probably be refined by #517 (e.g. CommandAPI-Spigot should never need PaperCommandRegistration, and later Paper versions should never need SpigotCommandReigistration)
The NMS methods getResourcesDispatcher, isVanillaCommandWrapper, wrapToVanillaCommandWrapper, and isBukkitCommandWrapper were moved to SpigotCommandRegistration because they only make sense if Paper changes are not present. These methods are only used in the CommandAPI by SpigotCommandRegistration anyway, but developers can still access them using CommandAPIBukkit#getCommandRegistrationStrategy. PaperCommandRegistration provides a similar method: isBukkitCommand.
TODO:
Before merging I want to double check that the following works
On the following versions:
Latest Paper (Paper-1.20.6-131)
Paper-1.20.6-65
Paper-1.20.6-64
Paper-1.20.5
Paper-1.20.4
Spigot 1.20.6
Spigot 1.20.4
And of course~ code review is greatly appreciated!