Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect with brigadier API does not work as expected #10827

Open
xaanndeer opened this issue May 29, 2024 · 4 comments · May be fixed by #10952
Open

Redirect with brigadier API does not work as expected #10827

xaanndeer opened this issue May 29, 2024 · 4 comments · May be fixed by #10952
Labels
priority: low This issue only describes a minor inconvenience. status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. type: bug Something doesn't work as it was intended to. version: 1.20.6 Game version 1.20.6

Comments

@xaanndeer
Copy link

xaanndeer commented May 29, 2024

Expected behavior

When redirecting aliases to a command using LiteralArgumentBuilder.redirect(LiteralCommandNode) the alias should execute said command.

Observed/Actual behavior

Aliases get sent to the client, however upon executing (without namespace) an unknown or incomplete command exception gets thrown. When using an alias without a namespace arguments don't get recognized and are marked as incorrect as well.

Steps/models to reproduce

Create a command using the brigadier API and add an alias that redirects to this command.

        LiteralArgumentBuilder<CommandSourceStack> builder = Commands.literal("teleport");
        builder.requires(commandSourceStack -> commandSourceStack.getSender().hasPermission("example.command.teleport"))
                .then(Commands.argument("target", ArgumentTypes.player())
                .executes(source -> {
                    Player player = source.getArgument("target", PlayerSelectorArgumentResolver.class).resolve(source.getSource()).getFirst();
                    source.getSource().getSender().sendMessage("Teleporting to %s".formatted(player.getName()));
                    return 1;
                }));
        LiteralCommandNode<CommandSourceStack> literalCommandNode = builder.build();
        commands.register(javaPlugin.getPluginMeta(), literalCommandNode, "Teleport command", Collections.emptySet());
        commands.register(javaPlugin.getPluginMeta(), Commands.literal("tp").redirect(literalCommandNode).build(), "Teleport command", Collections.emptySet());

Try to run this command by using the alias without namespace: Arguments are marked as incorrect and Unknown or incomplete command

Plugin and Datapack List

3ceeae3645d821a0d27cf3402021b675
datapack_list

Paper version

30.05 01:35:37 [Server] INFO This server is running Paper version 1.20.6-121-master@a31dc90 (2024-05-29T21:07:31Z) (Implementing API version 1.20.6-R0.1-SNAPSHOT)
30.05 01:35:37 [Server] INFO You are 3 version(s) behind
30.05 01:35:37 [Server] INFO Download the new version at: https://papermc.io/downloads/paper
30.05 01:35:37 [Server] INFO Previous version: 1.20.6-115-9d6f2cc (MC: 1.20.6)

Other

No response

@xaanndeer xaanndeer added status: needs triage type: bug Something doesn't work as it was intended to. labels May 29, 2024
@papermc-sniffer papermc-sniffer bot added the version: 1.20.6 Game version 1.20.6 label May 29, 2024
@papermc-projects papermc-projects bot moved this to 🕑 Needs Triage in Issues: Bugs May 29, 2024
@Machine-Maker
Copy link
Member

Well, first thing is that the structure of this command is probably not what you thought it was. You aren't marking the "argument" as executable. You need to call .executes() on the argument like this:

LiteralArgumentBuilder<CommandSourceStack> builder = Commands.literal("teleport");
builder
    .requires(commandSourceStack -> commandSourceStack.getSender().hasPermission("example.command.teleport"))
    .then(Commands.argument("target", ArgumentTypes.player())
        .executes(source -> {
            Player player = source.getArgument("target", PlayerSelectorArgumentResolver.class).resolve(source.getSource()).getFirst();
            source.getSource().getSender().sendMessage("Teleporting to %s".formatted(player.getName()));
            return 1;
        })
    );

Then at least doing /teleport @p does successfully teleport the player to itself.

@Machine-Maker
Copy link
Member

As for the problem that still exists if you fix ^, I guess I'd recommend just avoiding brigadier's "redirect" system if possible. There are inconsistencies within the library itself which can lead to all sorts of unexpected behavior. For example, the actually dispatcher logic does not handle double redirects at all (a redirect to a redirect to an executable). For root literals, I would just use the "aliases" collection parameter in the various register methods.

@xaanndeer
Copy link
Author

xaanndeer commented May 30, 2024

Yea sorry, it's pretty late and I quickly put together this example with a foggy brain. However, even with a correct structure the issue still persists. I was told to open up an issue on the Discord server as this is not intented behavior. Using the aliases collection in the register method does indeed work correctly, but I wanted to override the vanilla alias so I had to use redirect as using the register method will not override these.

EDIT: I updated the post with correct code and the correct observed behavior. Sorry for my negligence!

@lynxplay
Copy link
Contributor

Imo, while the brig redirect system seems to be a bit of a mess, this is still solvable by us.
Some may not be.

A) A node that does not have children cannot be redirected to from a single node.
The CommandDispatcher only resolves redirects if the reader can read post the initial literal node (e.g. arguments are provided).
Annoying.
B) When flattening an alias redirect (either because child.len == 0 or the internal flag was passed) the logic does not take into
account that the root of the alias itself might be a redirect.

So we'd need to fix brigadiers redirect BS by flattening them all (both the directly registered redirect and the then registered alias) to directly redirect or be a flattened copy of the redirect target in case of 0 children.

@papermc-projects papermc-projects bot removed the status in Issues: Bugs Jun 19, 2024
@codebycam codebycam added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. priority: low This issue only describes a minor inconvenience. labels Jun 19, 2024
@papermc-projects papermc-projects bot moved this to ✅ Accepted in Issues: Bugs Jun 19, 2024
ookiegajwa added a commit to ookiegajwa/Paper that referenced this issue Jun 21, 2024
When flattening any command or registering an alias to a node with no direct children (such as a redirect), the code now takes into account any potential redirect/fork/forward on the target node. This fixes the issue where, when registering a command that was simply a redirect, only the namespaced literal would work, and not any aliases of the command.
ookiegajwa added a commit to ookiegajwa/Paper that referenced this issue Jun 21, 2024
When flattening any command or registering an alias to a node with no direct children (such as a redirect), the code now takes into account any potential redirect/fork/forward on the target node. This fixes the issue where, when registering a command that was simply a redirect, only the namespaced literal would work, and not any aliases of the command.
kennytv pushed a commit to ookiegajwa/Paper that referenced this issue Jul 15, 2024
When flattening any command or registering an alias to a node with no direct children (such as a redirect), the code now takes into account any potential redirect/fork/forward on the target node. This fixes the issue where, when registering a command that was simply a redirect, only the namespaced literal would work, and not any aliases of the command.
kennytv pushed a commit to ookiegajwa/Paper that referenced this issue Jul 15, 2024
When flattening any command or registering an alias to a node with no direct children (such as a redirect), the code now takes into account any potential redirect/fork/forward on the target node. This fixes the issue where, when registering a command that was simply a redirect, only the namespaced literal would work, and not any aliases of the command.
ookiegajwa added a commit to ookiegajwa/Paper that referenced this issue Aug 10, 2024
When flattening any command or registering an alias to a node with no direct children (such as a redirect), the code now takes into account any potential redirect/fork/forward on the target node. This fixes the issue where, when registering a command that was simply a redirect, only the namespaced literal would work, and not any aliases of the command.
ookiegajwa added a commit to ookiegajwa/Paper that referenced this issue Oct 16, 2024
When flattening any command or registering an alias to a node with no direct children (such as a redirect), the code now takes into account any potential redirect/fork/forward on the target node. This fixes the issue where, when registering a command that was simply a redirect, only the namespaced literal would work, and not any aliases of the command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low This issue only describes a minor inconvenience. status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. type: bug Something doesn't work as it was intended to. version: 1.20.6 Game version 1.20.6
Projects
Status: ✅ Accepted
4 participants