Is it safe to use non-mb_* prefixed methods while parsing? Haven't looked code deeply but I saw a lot of substr and strlen used in code - wouldn't this affect to non-english commands?

It should be fine. We only care about byte lengths, and splittable whitespace chars will never be more than 1 byte.

@inxomnyaa raised concerns about performance when processing subcommands. I'm not sure to what extent this is a problem, but I have seen some plugins create much more complicated command signatures than vanilla. The way that overloading works, "subcommands" can be implemented as overloads using literals in this system, but it currently has to try each one, one at a time, to find a matching overload. This could become a problem if many overloads are present. This is sometimes the case with complex plugin commands, where nested commands blow up to create a large number of overload permutations.

Possibly, it might be advantageous to detect which overloads have the same first N parameters, so that if other overloads have the same leading arguments, we can eliminate multiple overloads at once in some cases without having to reparse the entire command over and over again.

It might also be worth exploring whether we can avoid bruteforce lookups when some or all overloads have a literal as their first parameter(s). This would be a common case for subcommands.

Optimizing overload selection shouldn't be a blocker for this PR. Plugins that are particularly concerned about huge number of overloads could always fall back to RawParameter to leverage their own optimized parsing algorithm.

Optimizing overload selection shouldn't be a blocker for this PR. Plugins that are particularly concerned about huge number of overloads could always fall back to RawParameter to leverage their own optimized parsing algorithm.

Yeah, but then they'd lose client-side command hints, which we all know is the thing plugin devs really care about, for some reason...

any plan to add PlayerArgument ?

Adds a tick() method to populate static::$VALUES with online player names,
quoted if they contain spaces. This improves compatibility with command input
parsing and tab-completion for string-based arguments.

PlayerArgument resolves actual Player instances, not raw name strings. This
approach allows dynamic, formatted suggestions without requiring resolution,
making it suitable for custom argument types that expect player names as strings.

Implementation:

• Iterates over Server::getInstance()->getOnlinePlayers()
• Quotes names with spaces: "Mr Hacker"
• Leaves simple names untouched: Steve, Alex
• Uses array_combine() to build a key-value map for suggestion use

I don't know about the tick stuff, that doesn't seem necessary to me, but yes there are plans to expand the system.

I'm currently unsure how to deal with target args because of the .self vs .other permissions for various commands.

Enums, soft (dynamic) enums, and composite types like Vector3 are also not supported yet.

This PR implements the minimum of what was needed to get PM's builtin commands moved over to the new system.

I'm considering possible options to introduce this API in PM5 to let people adopt it early, but it's not too clear how it would work. Current thinking is to introduce OverloadedCommand which implements the new features of this PR over Command, and then scrap the legacy API in PM6.

However, a PM6 scrapping would then mean renaming OverloadedCommand to Command and/or merging their functionality, which would then be an API break for early adopters anyway. Preferably I'd like a way of doing it that allows early adopters to get to PM6 with no effort. That way we're not punishing early adopters.

Or maybe I'm thinking too hard about this, since changing OverloadedCommand references to Command would be much less work if your code is already upgraded as opposed to having to do this all at once at PM6...

Demo of how LuckPerms command tree might be implemented using the API in this PR as of e762b12: https://gist.github.com/dktapps/57a0a2ba161592d0e18577c4e93f6264

in my opinion, a Target parameter is necessary for the initial release of this

Yeah, target and enum are on my todo list. I've been more focused on initial system architecture so far though.

Overview of changes since last reviews:

• Overload registration is now substantially changed.
  • Each command now has only one root overload, which may be either an ExecutorOverload (which accepts a callback) or a BranchingOverload (delegates to sub-overloads).
  • Simple commands with one callback can just pass a new ExecutorOverload directly to Command::__construct().
  • Overloaded commands (commands with multiple callbacks) must now use BranchingOverloadBuilder to create an overload tree.
    • A branching overload must have at least two child overloads. These children can be added to BranchingOverloadBuilder easily using ->executor() or ->branch(). (Constructing overloads directly is not allowed for registering nested overloads, because they need to know about all of their parent parameters to check the provided callback.)
    • Branching overloads can have nested branches, allowing nested subcommands and similar patterns.
    • If the first unique argument of a nested overload is a literal (i.e. a subcommand name), the branching overload will put it in a map, allowing fast lookups without bruteforce searches. This eliminates potential performance bottlenecks for common subcommand design patterns.
    • A branching overload may declare common leading parameters, which all of its child overloads will accept before their own unique parameters.
      • For example, /effect uses a branching overload which accepts player as a common arg, and then has two executor overloads, one of which accepts clear, and the other accepts args to apply an effect.
      • This avoids unnecessary leading parameter info duplication.
      • This is also better for performance, as leading arguments can be parsed once for many overloads which belong to the same branch. Previously they would be reparsed multiple times.
• Usage message generation on invalid command syntax now shows only the most likely usages, rather than showing all of them. For example, typing /time set without the time parameter will now only show /time set <time name> and /time set <timestamp>, and won't show the usage for /time add et al.
• Implemented RelativeXYZ parameters (3x relative floats).
• Stripped out legacy commands.*.usage and pocketmine.command.*.usage translation keys.

Remaining items of interest:

• Targeting parameters. These are a headache because of their interactions with permissions.
  • All PM commands currently declare different permissions for targeting self vs other targets. Since Parameters currently have no context, they don't have a way to check whether a target is permitted or not.
  • Technically we can just do a MappedParameter<Player> and leave the permission checking up to the handler, but this feels like an easy way to screw up, and the necessary permissions wouldn't be visible in overload metadata.
• Enums. I'm not sure exactly how I want to implement these.
  • The easiest thing would be to just pass the string directly to the handler, but I don't like that much.
  • Could also run it through a MappedParameter
  • Really it'd be nice if we could just do something like new EnumParameter(GameMode::class) and have all the aliases and mapping stuff automatically handled. However we don't want to force people to declare a new enum Whatever for every command parameter.
  • Not all command enums will correspond to code enums also. Consider stuff like item names, which will be a fixed list, but are registered dynamically in StringToItemParser rather than being declared as an enum.
• Usage messages going only to the next branch, rather than spamming the console with multiple overloads for a subcommand we might not want to use.
  • In the LuckPerms demo above, typing lp will generate 5 usage messages starting with lp verbose. Ideally we'd just show something like lp verbose ... instead, and then show the more specific messages only when the user actually types lp verbose. This would avoid unnecessary console spam.
  • I tried to implement this already, but it was a headache. Will revisit it.
• Possibly, we might want the ability to do optional literals before required parameters. This was an item of interest for cmdalias, where in practice the global token could be an optional literal. However I haven't figured out what the rules for this would need to be to keep the system sane and predictable.
• Translation keys for parameter display names. Vanilla doesn't do this, but PM has had translated usage messages for donkey's years, so it'd be kinda lame if we didn't still support that after introducing this system.
• Builtin commands to just construct Command directly, rather than extending it, since extending isn't needed anymore
• A way to potentially implement this system in PM5 without BC breaks.
  • Currently, the only things that Command actually directly tracks are namespace, name, root overload, custom permission message, and root description.
    • Namespace, name and root overload could be tracked directly by CommandMap
    • I'm thinking about allowing ExecutorOverload to have individual descriptions. This would be useful for subcommands to have different usage messages.
    • There's also an argument to be made for moving custom permission messages to overloads, or possibly getting rid of them.
  • Essentially, it's looking like Command might become completely obsolete, which would allow us to implement this system in PM5 in theory, while leaving legacy systems untouched.
• A way to get both the mapped value and original input for MappedParameter. This is needed for stuff like /give, where we want both the Item instance (to give to the player) and the alias used to give it (for the audit messages).
• Handlers really shouldn't be throwing InvalidCommandSyntaxException. Explore what's still doing this and why.

For enums, perhaps we could just pass closures that returns a list of string into the last parameter of MappedParameter?
For enum classes, add helpers for converting enums into list of enums for stuff like GameMode and etc.

pseudo code be like

BranchingOverloadBuilder::make(commonParameters: [
	new StringParameter("target", "target")
])
	->executor(
		["clear"],
		//TODO: our permission system isn't granular enough for this right now - the permission required
		//differs not by the usage, but by the target selected
		self::OVERLOAD_PERMS, self::removeEffect(...),
	)
	->executor([
		new MappedParameter("effect", "effect name", static fn(string $v) : Effect =>
			StringToEffectParser::getInstance()->parse($v) ??
				throw new ParameterParseException("Invalid effect name")
		, static fn() => StringToEffectParser::getInstance()->getKnownAliases()),
		new IntRangeParameter("duration", "duration", 0, (int) (Limits::INT32_MAX / 20)),
		new IntRangeParameter("amplifier", "amplifier", 0, 255),
		new BoolParameter("bubbles", "bubbles")

		//TODO: our permission system isn't granular enough for this right now - the permission required
		//differs not by the usage, but by the target selected
	], self::OVERLOAD_PERMS, self::modifyEffect(...))
->build()

in MappedParameter

/**
 * @tempate T of \UnitEnum
 * @phpstan-param class-string<T>
 */
public static function fromEnum(
	string $codeName,
	Translatable|string $printableName,
	string $enumClass
) : MappedParameter{
	$cases = $enumClass::cases();
	return new MappedParameter($codeName, $printableName, static function(string $v) use ($cases){
		foreach($cases as $case){
			if(strtolower($case->name) === strtolower($v)){
				return $case;
			}
		}
		throw new ParameterParseException("Invalid enum name");
	}, static fn() => array_map(static fn(\UnitEnum $enum) => strtolower($enum->name), $cases));
}

Would prefer to bind a mapping for it. But also ideally I'd like to implement it in such a way that cases can't suddenly become invalid later on.

There's also the matter of enum constraints to consider, although what we'd use those for I'm not very clear yet.

I haven't read the code that much so I'm sorry if I'm missing something. 3 years ago, I written a virion framework Ovommand that tried to do the same and recently, I came across this repo Blockception/BC-Minecraft-Bedrock-Command that may not help you much :L

I'd like to address something along my old experients, tho... these are what I came up limited by virion scope so it may not suit this well:

1. Position

• Position parameter has expandable size parameter in Bedrock. The parameter has maximum of three spans but they can be written as one or two spans.
  eg: 123 456 789, ~~~, ~ ~ ~, ~ ~-1.2~ are all valid inputs.
  My solution to this was to try to read until you found three valid param; otherwise, return an invalid syntax. Another solution may be to split this at the start of the parsing so each notation will be its own param.
• The position parameter can be local. This type of param will begin with caret notation '^' instead of tilde notation, and it cannot be combined with relative position. Previously, I wrote CoordinateResult to retain the position value and any notation with it. Then, call CoordinateResult::asPosition() to get the exact position depending on the provided entity input.

2. Enums

The way I implemented it was having enum being a dictionary where upon parsing will return the value from the key if there is any.

Really it'd be nice if we could just do something like new EnumParameter(GameMode::class) and have all the aliases and mapping stuff automatically handled.

I just create a native enum DefaultEnums and map out all of those...

enum DefaultEnums : string{
	case BOOLEAN = "Boolean";
	case VANILLA_GAMEMODE = "GameMode";
	case PM_GAMEMODE = "PMGameMode";
	case ONLINE_PLAYERS = "OnlinePlayers";

	public function encode() : IDynamicEnum | IStaticEnum{
		return match ($this) {
			self::BOOLEAN => new HardEnum($this->value, ["true" => true, "false" => false], isProtected: true, isDefault: true),
			self::PM_GAMEMODE => new HardEnum($this->value,
				["survival" => GameMode::SURVIVAL, "creative" => GameMode::CREATIVE, "adventure" => GameMode::ADVENTURE, "spectator" => GameMode::SPECTATOR],
				visibleAliases: ["survival" => "s", "creative" => "c", "adventure" => "a", "spectator" => "v"],
				hiddenAliases: ["survival" => "0", "creative" => "1", "adventure" => "2", "spectator" => "6"],
				isProtected: true, isDefault: true
			),
			self::VANILLA_GAMEMODE => new HardEnum($this->value,
				["survival" => GameMode::SURVIVAL, "creative" => GameMode::CREATIVE, "adventure" => GameMode::ADVENTURE, "spectator" => GameMode::SPECTATOR],
				["survival" => "s", "creative" => "c", "adventure" => "a"],
				isProtected: true, isDefault: true
			),
			self::ONLINE_PLAYERS => new SoftEnum("OnlinePlayers", isProtected: true, isDefault: true)
		};
	}
}

where it maps some handy enums such as DefaultEnums::BOOLEAN returns true/false based on input string or DefaultEnums::GAMEMODE for gamemode. For those soft enum suchs as online players, we will update it on player join and quit event.

So in my command setup, I can register the enum parameter with this code

$this->registerParameters(
	new PositionParameter("a"),
	new EnumParameter("aa", DefaultEnums::PM_GAMEMODE),
);

However we don't want to force people to declare a new enum Whatever for every command parameter.

They can save those newly created classes and reuse them :> jk. As two enum parameters can have different names, I think that new may be needed. I might misunderstand your words here; you might mean the same thing as returning a new HardEnum when the DefaultEnum gets encoded.

Would prefer to bind a mapping for it. But also ideally I'd like to implement it in such a way that cases can't suddenly become invalid later on.

Were those cases where they become invalid due to the item being dynamically removed? Anyway, we can use soft enums instead of hard enums for those... but we need to update the new changes via UpdateSoftEnumPacket every time the list gets changed.

Possibly, we might want the ability to do optional literals before required parameters. This was an item of interest for cmdalias, where in practice the global token could be an optional literal. However I haven't figured out what the rules for this would need to be to keep the system sane and predictable.

Can you explain this more? I may not understand this… Why not just create two overloads, where one has the optional literal and the other doesn't?

Since literals are string enums? I also noticed these kind of enums are kind of buggy? There are cases where the hints break in when you try the string enum with other datatypes.

Why not just create two overloads, where one has the optional literal and the other doesn't?

That's what I did, but it's double the number of usages

For the enums thing, as I said, I don't want to force people (or be forced myself) to define a new enum class every time they want to add a new set of options to a command. It should be possible to use an enum if desired, but not required.

Anyway, we can use soft enums instead of hard enums for those

Alvin's suggestion for binding StringToItemParser to an enum would have cases magically vanish from existence if the parser mappings were modified at runtime, and the command system wouldn't have any way to know about it. We don't want to use soft enums for everything because it'll complicate things for users who just want a fixed set of options. So the enums need to capture their valid value set and ensure that the mapper accepts every value specified. Not exactly sure how that's going to work currently.

Since literals are string enums? I also noticed these kind of enums are kind of buggy? There are cases where the hints break in when you try the string enum with other datatypes.

Literals are not enums in this system, not on the server side anyway. They may be sent as enums over the network, but that's kinda irrelevant.

My priority here is building a system for plugin devs to use. The client side command suggestions are a happy bonus, but if they get in the way I'm not gonna cry about breaking them. Already had this headache with --flags in the style that LuckPerms uses. Plugin use cases are far more complex than anything vanilla does.

To be clear, the holdup isn't a lack of solutions, but that decisions haven't been made in affected areas

Coordinate params were already implemented, those were quite trivial. I didn't initially implement them because they weren't necessary and I needed to refactor some code to avoid duplication. But that's dealt with now.

Though, I wasn't aware that the inputs can be combined with no whitespace (which seems kinda sus to me, that's probably a bug in Bugrock), but if I wanted to support that here it'd just be a case of deleting some zero-whitespace checks in RelativeXYZParameter::parse().
I haven't done ^ yet either because it hasn't been needed to support PM commands yet and I'm unfamiliar with how it works. That can come later on. The goal of this PR is to implement the framework for the new system and make sure it's possible to do everything we want. The smaller stuff can come later on.