commands
commands copied to clipboard
aikar
Support for parameters in middle of command nodes
A format supported by Brigadier for Minecraft is:
/bossbar add <id> <name>
/bossbar set <id> color <color>
/bossbar set <id> name <newname>
/bossbar remove <id>
This is complicated to support, but desirable for defining a common parameter for a group of commands that all have same requirements, instead of duplicating it.
I've designed the following concept to represent this structure:
class BossBarCommand extends BaseCommand {
@CommandParameter("id")
NamespacedKey id;
@Subcommand("add <id>")
public void onAdd(CommandSender sender, String name) {}
@Subcommand("remove <id>")
public void onAdd(CommandSender sender) {}
@Subcommand("set <id>")
private class SetCommands extends BaseCommand {
@Subcommand("name")
public void onName(CommandSender sender, String name) { }
@Subcommand("color")
public void onName(CommandSender sender, Color color) { }
}
}
I think this is super clean and no boilerplate, however one main drawback: This is dangerous for when we add Asynchronous Execution of commands.
Proposed solution to async problem: Clone the BaseCommand instance anytime ASYNC dispatch occurs (won't be as common). This seems inefficient, but commands aren't exactly hot, nor would people be async'ing as much.
By cloning before switching to a new thread, the execution can get a snapshot of the parameter as it was. It would have to be highly documented, noting that you wouldn't be able to mutate any fields on the class in an async dispatcher (but you could call into the objects it points to, it would not be a deep clone -- the standard java clone), and that your responsible for concurrency still.
The clone would be just to avoid the instances field being updated for a 2nd command execution in the middle of the first. For sync commands, there is no need to clone.
Feedback?
This sounds sane.
We should note that CommandParameter is already a class name in the project.
This leads me to think that this may not be the most appropriate annotation-name choice, as the existing class refers to the method's parameters. And this is not located in the command-method's parameters. Not a deal breaker, just does not seem ideal.
At first I thought of @CommandArgument instead. This seems like a logical choice, but a caveat is that this would be inappropriate for something that can resolve without input (IssuerOnlyContext) - as those are arguably not a command argument, not consuming any input.
So my suggested annotation-name is dependent on whether that is a desired feature of the new annotation. That desired extra feature being, fields with this annotation attempt to resolve, if not referenced in the resolving subcommand and are an issuer resolving context.
Potential use, would be if you knew you wanted the executor to only be a player, and want that for every subcommand without explicitly declaring it in each method's parameter list.
Would be nice to also support.
@Subcommand("add <id> name") public void onAdd(CommandSender sender, @CommandParameter("id") NamespacedKey id, String name) { }
as an option to do the same thing as described above.
as a solution for the Async Concern, we can mandate that default params defined at the class level have to be done as a
@CommandParameter("param")
private CommonParameter<Foo> param = CommonParameter.of(Foo.class);
Then using this param requires Foo foo = param.get(); which accesses a backing ThreadLocal defined in CommonParameter
No haven't had time due to other work, but have been thinking on doing some prep work to reorder internals to better prep for this and ease integration with Brigadier.