Command build rewrite #602
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Precursor work for #483 Notable changes: - No more flattening everything into a CommandAPICommand - CommandTrees are not flattened - CommandMetaData and Execution removed (only used to flatten CommandTrees) - MultiLiteralArgument and subcommands are no longer converted into LiteralArguments - Optional arguments are not chopped up into multiple arrays - In general, a single call to `ExecutableCommand#register` now creates the entire Brigadier tree for that command instance - Should only be converting Arguments to Brigadier nodes once instead of multiple times - CommandAPIHandler no longer creates Brigadier nodes - Delegated to AbstractCommandAPICommand, AbstractCommandTree, AbstractArgumentTree.java, and AbstractArgument - Problems that prevent a command from being registered have been unified under CommandRegistrationException - Exception messages tweaked - Bukkit-specific features removed from `commandapi-core` - Removed `isConverted` property of CommandAPICommand - Moved logic for flattening EntitySelectorArgument from `CommandAPIHandler#generateBrigadierCommand` to Converter - Moved special Bukkit command permission stuff into CommandAPIBukkit/SpigotCommandRegistration - Taking advantage of Brigadier redirects - Command alias nodes redirect to main command name - MultiLiteralArgument literal nodes use redirects - Instead of duplicating the argument node structure after these arguments, they can reference the same path TODO: - The checks performed by `CommandAPIHandler#hasCommandConflict` have not been reimplemented (I'm not sure if they did anything?) - MultiLiteralArgument is currently broken. See Mojang/brigadier#137 - Many changes not covered by tests (could be sneaky behavioral differences)
…ndTreeAndOptionalArgumentMethod` TODO: Figure out what to do about this
Part of `dev/command-build-rewrite` Solves the problem with 1803e4e where optional arguments in CommandTrees didn't work This commit implements option 3 from 193f237#r126326791 Basically, Arguments can no longer be optional by themselves. Instead, they are optional if they exist in a special list. This restricts the freedom of optional arguments in CommandTrees to avoid ambiguous cases, while CommandAPICommands work essentially the same via the `withOptionalArguments` method. Changes: AbstractArgument - Removed `boolean isOptional` - Removed method `setOptional` AbstractCommandAPICommand, AbstractCommandTree, AbstractArgumentTree - Added `List<Argument> optionalArguments` - Tweaked command-building logic to include `optionalArguments` list ExecutableCommand - Merged some registration logic shared between CommandAPICommand and CommandTree Kotlin DSL - `optional` boolean now chooses between calling `withArguments` or `withOptionalArgument`, instead of calling `setOptional` Tests - Added tests for making sure `RegisteredCommand` information is correct - Added tests for optional Arguments in CommandTree - Re-enabled test disabled by 193f237
Whoops, these actually are some silly mistakes I missed :P, thanks SonarCloud
…into CommandAPIHandler
…ing redirects The Brigadier CommandDispatcher CommandNode structure does not need to be a tree. This means one node can have multiple parents. In this case, the node after a MultiLiteralArgument has parent node for each literal in the MultiLiteral. This works around the issue documented in Mojang/brigadier#137, where arguments disappear from the CommandContext used when executing an argument when the command encounters a redirect. This way, we can remember which literal was chosen no matter which branch was taken, yet still converge on the same path.
Changes: - Implemented `FlagsArgument` (#483) - Moved var handles for `CommandNode` `children`,`literals`, and `arguments` to `CommandAPIHandler` - Added `FlagsArgumentCommon` FlagsArgumentRootNode` and `FlagsArgumentEndingNode` to handle the special node structure and parsing required - Updated `CustomArgument` - All `AbstractArgument` builder methods are delegated to the base argument - Replaced `CommandAPIExecutor` parameter of `AbstractArgument#addArgumentNodes` to a `Function` to allow objects that hold arguments to better control how those arguments are executed - Added package `dev.jorel.commandapi.commandnodes` for class that extend `CommandNode` and related classes - Tweaked some exceptions - `GreedyArgumentException` - Changed because the `FlagsArgument` is sometimes greedy - only greedy iff it has no terminal branches - Greedy arguments are now detected when `AbstractArgument#addArgumentNodes` returns an empty list - Tweaked the exception message - `DuplicateNodeNameException` - Changed because literal arguments can conflict with other nodes if they are listed - Now thrown when two listed arguments have the same node name - Added `UnnamedArgumentCommandNode` to make sure unlisted arguments do not conflict - Renamed `MultiLiteralCommandNode` to `NamedLiteralCommandNode` for use by listed `Literal` arguments - Tweaked the exception message TODO: - Clean up code - Add tests - Remove test commands in CommandAPIMain - Add javadocs and documentation - Hope Mojang/brigadier#144 is resolved, otherwise be annoyed :(
I messed this up when rebasing changes from #509 into `dev/command-build-rewrite`
Also fix typo in exception message
- Reordered constructor arguments to more closely match the order in `ExecutableCommand`
- This branch is now definitely backward incompatible (though not majorly)
- Replaced `List<String> argsAsStr` with `Node rootNode`
- Added `RegisteredCommand.Node` class
- Has an `argsAsStr` method as a replacement for the parameter of `RegisteredCommand`
- Simplified representation of the Brigadier's `CommandNode` structure for the command
- Removed `AbstractArgument#getHelpString` and `RegisteredCommand#arguments` (Added by #537)
- The `RegisteredCommand.Node` structure is used to generate usage
- `Literal` and `MultiLiteral` have separate logic for creating thier `RegisteredCommand.Node, wherein they define a different help string
- TODO: #537 didn't have any tests, so I'm only guessing this new implementation works the same. In general, add more tests for usage generation.
- Removed `ExecutableCommand#getArgumentsAsStrings` and `AbstractArgument#appendToCommandPaths`
- Added `AbstractArgument.NodeInformation` to help pass around enough information to generate Brigadier nodes and `RegisteredCommand.Node`s in one `AbstractArgument` tree traversal.
- Modified the signatures of a few methods to facilitate this, including overriding methods
- TODO: This broke `Previewable` arguments, so I'll have to tweak those
- Changed `CommandAPIHandler#registeredCommands` from an `ArrayList` to a `LinkedHashMap`
- Instead of creating one `RegisteredCommand` object for each command path, `RegisteredCommand`s are merged when they share thier command name and namespace
- One call to `ExecutableCommand#register` creates one `RegisteredCommand` object (if it has a namespace, an unnamespaced copy is created too)
- `CommandAPIPlatform#postCommandRegistration` was reverted to original signature
- NOTE: `CommandAPI#getRegisteredCommands` still returns a `List<RegisteredCommand>`
- Updated `CommandAPICommandRegisteredCommandTests` and `CommandTreeRegisteredCommandTests`
- Added `RegisteredCommandTestBase` to handle common utility methods
- Removed `CommandAPIHandler#previewableArguments` and related methods - Added `PreviewableCommandNode` for storing `Previewable` information directly in Brigadier's tree - Tweak `NMS_1_19_Common_ChatPreviewHandler` to build previews from the node tree rather than by the node path TODO: Should probably test these changes on a real server to verify. Also, another example of Mojang/brigadier#144 being annoying.
Revises github.com/JorelAli/CommandAPI/commit/ac8c06086f2e05015416b939028e1301ff95a87b
Notable changes:
- `RegisteredCommand` and `RegisteredCommand.Node` now have generic `<CommandSender>` parameter
- `RegisteredCommand.Node` includes the `CommandPermission permission` and `Predicate<CommandSender> requirements`, copied from the argument/command the node represents
- `RegisteredCommand#permission` is now acessibile as the permission of the `Node rootNode`
- Created `CommandAPIHelpTopic` in `dev.jorel.commandapi.help` package
- Replaced `shortDescription`, `fullDescription`, `usageDescription`, and `Object helpTopic` fields in `ExecutableCommand` and `RegisteredCommand` with `CommandAPIHelpTopic helpTopic`
- Extended by `EditableHelpTopic`
- Help builder methods of `ExecutableCommand` are delegated to its `CommandAPIHelpTopic` if it is editable
- More general API created for #528: short description, full description, and usage can be provided separately to take advantage of the formatting the CommandAPI uses for static Strings
- Extended by `BukkitHelpTopicWrapper` in `commandapi-bukkit-core`
- Wraps Bukkit's `HelpTopic` as a `CommandAPIHelpTopic` so full `HelpTopic` customization can still be used
- Created `CustomCommandAPIHelpTopic` in `commandapi-bukkit`
- Converts a `CommandAPIHelpTopic` into a Bukkit `HelpTopic` for adding to the help map
- Replaces usage of `NMS#generateHelpTopic`
- Help formatting code extracted from `CommandAPIBukkit#updateHelpForCommands`
- Resolves #470: `CommandSender` permissions and requirements are now checked when generating usage
- Changed the treatement of namespaced help topics (#546). Namespaced help topics are now created. In the case where the same command name is registered with different namespaces, this allows the user to see the unmerged help using `/help namespace:commandName` (see `CommandHelpTests#testRegisterMergeNamespaces`).
- Updated tests to fully cover changes
TODO: Update and write documentation
Note: This commit makes this branch backwards incompatible, especially for non-DSL Kotlin code (see `commandapi-documentation-code/.../Examples.kt`). "Standard" Java and DSL API code usage looks the same, but I'm pretty sure plugins will need to recompile due to changes to the `FunctionalInterface`s. There are also some smaller public API changes, mentioned below.
Notable changes:
- Removed `AbstractCommandSender` and all its implemenations (i.e. the `dev.jorel.commandapi.commandsenders` package is completely gone)
- Logic in `CommandAPIHandler#generateBrigadierRequirements` for checking if a sender satisfies a `CommandPermission` was moved to `CommandAPIPlatform#getPermissionCheck`
- Previously, methods in `AbstractCommandSender` provided access to `hasPermission` and `isOp`. `CommandAPIBukkit` and `CommandAPIVelocity` now handle these definitions.
- `CommandPermission.TRUE()` and `CommandPermission.FALSE()` added for computing short circuits when combining permissions and requirements
- `PreviewInfo` now has the generic parameter `Player` rather than an `AbstractPlayer`
- Backwards-incompatible (`(Player) player.getSource()` becomes `player`)
- Generic parameter propogates to `PreviewableFunction` and `Previewable`
- `CommandAPIPlatform` methods for convert Brigadier Source to CommandSender simplified
- `getSenderForCommand` removed
- just pass `CommandContext#getSource` into `getCommandSenderFromCommandSource`
- `forceNative` parameter was only used on Bukkit, which is now handled by `NMS#getNativeProxyCommandSender` (more on that below)
- `wrapCommandSender` removed
- Logic from #580 removed, since wrapping the NullCommandSender no longer needs to be handled as a special case
- Wrapping the sender no longer necessary for `getBrigadierSourceFromCommandSender`
- `CommandAPIVelocity` now does nothing in these methods :P
- `CommandAPIExecutor` reworked
- `ExecutorType` moved to platform modules (so Velocity can no longer try to define sender types that don't exist)
- Detecting whether a certain executor can be run by the given class delegated to `BukkitTypedExecutor` and `VelocityTypedExecutor`
- Priority of executors now depends on order of calling the `executes` methods (i.e the priority listed here https://commandapi.jorel.dev/9.4.1/normalexecutors.html#multiple-command-executor-implementations no longer applies)
- Normal executors are no longer ignored if resulting executors exist (not sure if this was a bug or intended)
- Tweaked `ExecutionInfo`
- Added `CommandContext<Source> cmdCtx` to `ExecutionInfo`
- This allows passing the information needed for a `NATIVE` executor on Bukkit to create a `NativeProxyCommandSender`
- Uses `NMS#getNativeProxyCommandSender`, which was adapted from the removed `getSenderForCommand` method
- Note: conflicts with #478, though should be easily resolved
- Note: Velocity can define the `CommandSource` object, though Bukkit has it as `?`. This makes it hard for non DSL Kotlin to infer the type parameters for some reason, which is annoying because you now need to specify the type parameter. I don't know if there's a better way to handle that.
- TODO: Make sure depdendents can still use `ExecutionInfo` without including Brigadier as a dependency
- `ExecutionInfo` is now a record (`BukkitExecutionInfo` and `VelocityExecutionInfo` removed)
- Simplified `dev.jorel.commandapi.executors` package
- Platform and sender specific executor classes (e.g. `PlayerCommandExecutor` and `EntityResultingExecutionInfo`) removed
- Just the functional interfaces `NormalExecutorInfo`, `NormalExecutor`, `ResultingExecutorInfo`, and `ResultingExecutor` now
- `BukkitExecutable` and `VelocityExecutable` link different command sender classes as type parameters to the `ExecutorType` enum values
TODO:
- Add executor tests to `dev/dev` to ensure same behavior
- Especially for `PROXY` and `NATIVE` senders
- Especially for non-DSL Kotlin to see how its lamdba type inference works
- Update documentation
Resolves #557 concerns Expand CommandConvertedTests and ArgumentEntitySelectorTests to cover argument flattening Miscellaneous changes to testing framework to make entity selectors work better
Acts like `MultiLiteralArgument`, but literals can be changed and differ for each CommandSender Resolves #513 Uses Paper's `AsyncPlayerSendCommandsEvent` and Velocity's `PlayerAvailableCommandsEvent` to change the client's view of the command tree. Note that a similar event does not exist on Spigot, so the suggestions are not dynamic, though the parsing still is. `DifferentClientNode` class added to handle creating client-server command tree de-syncs (I believe this can be used to make `FlagsArgument` platform-agnostic)
This was not made trivial by DifferentClientNode DifferentClientNode changes: - DifferentClientNode now an interface - Split into Argument and Literal subclasses - Node rewriting happens in two stages, `onRegister = true`, then `onRegister = false` - Each platform uses this differently b/c they're all special - Spigot/Paper run `onRegister = true` when registering to get rid of FlagsArgument loops, which otherwise cause a StackOverflow - Spigot runs `onRegister = false` when registering since it doesn't have a command send event (and so DynamicMultiLiteral can't display suggestions correctly) - Velocity runs `onRegister = true` during the command send event because `CommandNode.literals` doesn't exist FlagsArgument changes: - Generalized `executorCreator` from AbstractArgument node building with TerminalNodeModifier FunctionalInterface - Allows redirecting and wrapping nodes without reflection - FlagsArgumentEndingNode extends DifferentClientNode - Rewriting turns server-side children loop into redirects for the client (Mojang/brigadier#137) - Reference to root for setting the redirect is sneakily stored in HiddenRedirect command node, which gets properly updated when Velocity copies a node tree - Children loops are built by FlagsArgumentRootNode by calling `addChild`, rather than sharing Map instances with reflection, since that could de-sync the `hasLiterals` boolean on Velocity - FlagsArgument now only uses reflection indirectly through DifferentClientNode :O CommandAPIHandler#writeDispatcherToFile changes: - All platforms now use custom algorithm for serializing CommandNode to JSON - Does not StackOverflow if children loop - References duplicate instances of nodes in a tree by their shortest path - Serializes orphaned redirects - Supports serializing non-Brigadier CommandNode classes through NodeTypeSerializer - CommandAPIPlatform#createDispatcherFile replaced by #getArgumentTypeProperties - On Bukkit, SafeStaticMethodHandle used to access private static nms method ArgumentUtils#serializeArgumentToJson - On Velocity, ArgumentTypeSerializer added to extract ArgumentType properties - Slight updates to tests that verify dispatcher JSON Other changes: - Forgot to insert `<>` on PaperImplementations constructors in last commit - Removed deprecated `MultiLiteralArgument(String[] literals)` constructor because the null node name caused a NPE when hashing its command nodes - Tweaked DynamicMultiLiteralCommandNode to make it work when wrapped inside a FlagsArgumentEndingNode - Removed warning when using empty namespace on Velocity - Removed warning when `CommandNode.literals` field not found on Velocity - Switched commandapi-velocity-core to depend on Velocity's custom Brigadier fork TODO: - DynamicMultiLiteral still needs tests - Test NMS ArgumentUtils reflection on relevant Bukkit versions - Remove test commands - Implement custom NodeTypeSerializers
Removed subcommand execution test from ArgumentMultiLiteralTests since subcommands are no longer converted to MultiLiterals
Add NodeTypeSerializerTests
…registeredCommands` Also made unregistering on Velocity respect the `unregisterNamespaces` parameter and update the dispatcher file
The logic for detecting when CommandSourceStacks have a target Entity different from the sender (e.g. when using `execute as`) was incorrectly removed, causing `executesProxy` executors to never be taken on a real server
For some reason, on those specific versions, `JsonArray#deepCopy` has protected visibility ¯\_(ツ)_/¯. So, just don't use that method ig.
This was
linked to
issues
Sep 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wow, I created this branch 1 year and 1 day ago (must have started working on the initial commit a bit before that). This PR rewrites the internal logic that turns CommandAPI commands into Brigadier and Bukkit commands, with the general goal being to fix issues, add features, and expand test coverage. Reading through all the commit descriptions is a decent way to see all the changes that were made, and I tried to write good comments for the new code, but I'll try to summarize things here. This PR relates to the following GitHub issues:
FlagsArgumentfor Bukkit and VelocityDynamicMultiLiteralArgumentfor Bukkit and Velocity(java|bedrock), rather than being expanded into each literal pathThis PR also addresses various oddities that I noticed while going through all the code. I believe these old behaviors are unwanted, though they evidently weren't enough of a problem to be discovered before and reported as an issue.
CommandTreewith no executions did not log any error - This now throws aMissingExecutorExceptionjust likeCommandAPICommand.CommandAPI.getRegisteredCommands()commandapi-coreAbstractCommandAPICommand.isConvertedand converted argument flattening logicCommandAPIBukkit#postCommandRegistrationCommandMetaDataandRegisteredCommandOptional<Object> helpTopicreplaced byCommandAPIHelpTopicinterface and subclassBukkitHelpTopicWrapperExecutorTypeenum for each platform. E.g. a Velocity command can no longer try to define an execution with typeExecutorType.ENTITY.The largest changes come from refactoring the Brigadier node building logic out of
CommandAPIHandlerand intoAbstractCommandAPICommand,AbstractCommandTree,AbstractArgumentTree, andAbstractArgument. This has the following implications:MultiLiterals share children nodes instead of creating duplicate command paths (seeArgumentMultiLiteralTests). This reduces the total amount of nodes in the tree.FlagsArgumentandDynamicMultiLiteralArgumentcan use custom CommandNode implementationsPreviewablearguments use a custom CommandNode to store the information needed to generate chat preview directly in the Brigadier tree, rather than in aMap<List<String>, Previewable<?, ?>> previewableArgumentsin CommandAPIHandlerCommandTrees, subcommands, and optional arguments don't need to be flattened into multipleCommandAPICommands. This theoretically reduces the amount of work that happens when registering these sorts of branching commands, as each argument should now only be processed once, though I haven't benchmarked anything.While this PR is mostly internal changes, there are a few public-facing changes that make it backwards-incompatible:
AbstractArgument#setOptional. As discussed here (193f237#r126281889), allowing unrestricted access to optional arguments inCommandTrees can create ambiguous situations. After further discussion in the CommandAPI discord (ongoings-development > Optional argument rewrite), it was resolved that optional arguments inCommandTrees are unnecessary when they can already have multiple executors defined. Optional arguments are still usable as before inCommandAPICommands using thewithOptionalArgumentsmethod.RegisteredCommand(seeCommandAPICommandRegisteredCommandTestsandCommandTreeRegisteredCommandTestsfor examples in code)shortDescription,fullDescription, andusageDescriptionfields withCommandAPIHelpTopicCommandAPICommand, and each of those get their ownRegisteredCommandput into a list. With this PR, a singleRegisteredCommandobject gets created for each command literal node placed in the dispatcher. I.e. theMap<String, RegisteredCommand<CommandSender>> registeredCommandsinCommandAPIHandlershould roughly reflect the CommandAPI commands added to theMap<String, LiteralCommandNode<S>> childrenin the Brigadier dispatcher's root node.RegisteredCommands inCommandAPIHandleris now a map. However, I didn't change the signature ofCommandAPI#getRegisteredCommands, so it is still only exposed as a list.List<String>. Now, since aRegisteredCommandmay represent multiple command paths, an innerNoderecord is used to represent the Brigadier command tree. AList<List<String>> argsAsStrmethod was defined to provide the arguments in a similar format as before, which is still used when logging the commands being registered. TheNoderecord also allows accessing the permission and requirements of each Argument, which wasn't possible before.executesmethods changesAbstractCommandSenderand all its subclass removed (no longer necessary to wrap command senders)PlayerExecutionInfo->NormalExecutorInfo<Player, ?>Examples.ktdid.At the moment, I believe I have finished all the code changes I intended and resolved all the TODOs I discovered along the way. Hence, I'm opening this PR for code review. Questions, comments, or concerns from anyone who has the time to look through all this or just a part is greatly appreciated! Implementations are always subject to review, and I haven't explained everything about the new code in this description, as I think it would be more useful to write things down in review comments where the code being addressed is more easily visible.
This PR is currently a draft as I still have to write or update documentation for the following:
EditableHelpTopicFlagsArgumentDynamicMultiLiteralArgumentRegisteredCommand