Parameters for user selectors, user or target, and multiple users

[WillBAnders]

The Parameter class contains helpers for player() (which supports selectors) and playerOrTarget(), but not similar methods for users. There is also a ResourceKeyedValueParameters.MANY_GAME_PROFILES parameter (as well as one for MANY_PLAYERS), but these do not have helpers in Parameter so I am unsure if they are intended to be used publicly.

I believe what's happening here is that because users must be loaded, it is more performant to start with the player related methods and then fall back to users with some type of Parameter.firstOf(...) approach, though how exactly that's done I'm not sure. Still, it would be helpful to have some methods in Parameter for these situations if possible.

[dualspiral]

The Parameter class contains helpers for player() (which supports selectors) and playerOrTarget(), but not similar methods for users.

As intended. ServerPlayers are live instances of actively playing players and it makes sense to be able to select them. For Users, it does not - how would you select them? Same for orTarget. The correct way to handle this is to use firstOf,

Parameter.firstOf(Parameter.player()..., Parameter.user())

and you handle the case where you have a player, vs the case where you have a UUID.

I believe what's happening here is that because users must be loaded, it is more performant to start with the player related methods and then fall back to users with some type of Parameter.firstOf(...) approach, though how exactly that's done I'm not sure

Not quite - though that's why the User methods return a UUID. In the vast majority of cases, you don't need a User right that second because they are offline. This includes in the body of a command - if you need a User, nine times out of ten you can split off the rest of the execution into its own thread if you wanted to and let the command itself "complete" successfully if the fetch goes async.

As I said above though, firstOf should be used here -- the real reason for the split is the reality that players and users are two quite different concepts under the hood. Of course, if you don't need selectors, you can just use user and check to see if the UUID corresponds to a player in your executor before heading to the user manager.

There is also a ResourceKeyedValueParameters.MANY_GAME_PROFILES parameter (as well as one for MANY_PLAYERS), but these do not have helpers in Parameter so I am unsure if they are intended to be used publicly.

They are intended to be used publically - if it wasn't destined to exist in the API then it wouldn't be there! I just never added it to the Parameter class, mostly because I didn't see it as being used significantly and the parameter class is already larger. It could be added though.

[WillBAnders]

I did try using Parameter.firstOf(Parameter.player(), Parameter.user()) and then handling them separately in the executor. This works as expected for the complete command, however there is weird behavior with the usage and executing incomplete commands.

The command should have the usage (<player> | <user>) <key> <quantity>, but somehow ends up with <user> [<key>] | <player> [<key>] - the key becomes optional while quantity is lost completely. Executing /crate key give WillBAnders hard errors as key is not available in the executor at runtime. The usage, stacktrace, and source are below.

• Stacktrace

    private static final Parameter.Value<ServerPlayer> PLAYER = Parameter.player().key("player").build();
    private static final Parameter.Value<UUID> USER = Parameter.user().key("user").build();

    public static Command.Parameterized COMMAND = Command.builder()
        .permission("cratecrate.command.key.give.base")
        .addParameter(Parameter.firstOf(PLAYER, USER))
        .addParameter(Parameter.choices(Key.class, Config.KEYS::get, Config.KEYS::keySet).key("key").build())
        .addParameter(Parameter.rangedInteger(1, Integer.MAX_VALUE).key("quantity").build())
        .executor(Give::execute)
        .build();

[dualspiral]

Welcome to Brigadier!

On the (player | user) thing - there is little we can do about it, unfortuantely - we have no control over the client, we just send the nodes and the client handles the hints like that. Coupled with this PR not yet being merged, we're kind of stuck with what we can do (redirects would make this nicer...).

key being optional is a weird quirk and it must be an optional being set somewhere due to some other condition. I'll take a look when I get a moment.

[dualspiral]

I fixed the issue in your last comment about optional parameters - inverted boolean.

As for everything else - I don't think there is really anything I want to do. You mentioned in your commit for your plugin about selectors being partially blocked by this issue - but it's not really, as I say, selectors only work on ServerPlayers and it's a concious decision to not to have User parameters, only UUID parameters that correspond to users you can fish out in the executor.

If you want to collapse this into a single parameter, I think my answer to that is "do it yourself", but I would strongly advise using the parameters as set and do minimal additional logic in the parsers, preferring the executor whenever possible. We're trying to gear the system for performance, you don't want to be pausing the world to retrieve a user during parse time (which could be called multiple times for command completions -- you probably don't want that to happen).