Cache Bukkit Command when wrapping CommandNodes #11789
Conversation
Resolves PaperMC#11378 by "restoring" the Spigot behavior where VanillaCommandNodes are only created once. Before this commit, BukkitBrigForwardingMap would create a new VanillaCommandWrapper each time a CommandNode was requested via the Bukkit CommandMap. This meant that calls to `Command#setPermission` would not persist between retrievals from the map. This allows command frameworks that insert CommandNodes directly into the Brigadier dispatcher to change the permission String of the VanillaCommandNodes created for their commands, rather than it always being the default `"minecraft.commands.<name>"`.
The issue that if at any point |
|
Indeed, if This PR only fixes But yeah, again, if we just want to fix #11378 by changing the implementation of |
| @@ -13,6 +13,7 @@ | |||
| + public CommandNode<net.minecraft.commands.CommandSourceStack> clientNode; // Paper - Brigadier API | |||
| + public CommandNode<io.papermc.paper.command.brigadier.CommandSourceStack> unwrappedCached = null; // Paper - Brigadier Command API | |||
| + public CommandNode<io.papermc.paper.command.brigadier.CommandSourceStack> wrappedCached = null; // Paper - Brigadier Command API | |||
| + public org.bukkit.command.Command wrappedBukkitCommandCached = null; // Paper - Brigadier Command API | |||
There was a problem hiding this comment.
Semantics but, I think cachedWrappedBukkitCommand would read better than what's currently there
There was a problem hiding this comment.
Yeah, makes sense to me. I think I originally went with wrappedBukkitCommand + Cached to match the naming of unwrapped + Cached and wrapped + Cache directly above.
Maybe Owen had a reason to go with the original naming, but in isolation I prefer cached + WrappedBukkitCommand.
Reopens #11385.
Resolves #11378 by "restoring" the Spigot behavior where VanillaCommandNodes are only created once. Before this commit,
BukkitBrigForwardingMapwould create a newVanillaCommandWrappereach time aCommandNodewas requested via the BukkitCommandMap. This meant that calls toCommand#setPermissionwould not persist between retrievals from the map.This allows command frameworks that insert
CommandNodes directly into the Brigadier dispatcher to change the permission String of theVanillaCommandNodes created for their commands, rather than it always being the default"minecraft.commands.<name>".As discussed on the original PR, it's probably a good idea to eventually migrate the behavior of
Bukkit#dispatchCommandto use Brigadier directly rather than working via the BukkitCommandMap, which causes issues like this. Now that the hardfork is real, Paper may eventually fully removeCommandMapin favor of direct Brigadier API, which would make fixingCommandMapbehavior like this PR does irrelevant. However, I feel like while Paper is still using theCommandMapfixes like this are still valuable.If we'd rather fix #11378 by changing the implementation of
Bukkit#dispatchCommandto use Brigadier directly, feel free to close this PR. Although, while not a requirement of the Map interface, it is also neat whenmap.get(a) == map.get(a)for alla, which isn't necessarily true for theBukkitBrigForwardingMapbefore this PR.