Velocity icon indicating copy to clipboard operation
Velocity copied to clipboard

Opaque argument types

Open hugmanrique opened this issue 2 years ago • 3 comments

This introduces the OpaqueArgumentType interface, that is useful when a plugin wants the client to parse the contents and provide suggestions for an ArgumentCommandNode according to one of the built-in argument parsers. The following example constructs a simplified version of the /give command:

CommandManager commandManager = ...;
OpaqueArgumentType itemType = commandManager.getOpaqueArgumentType(Key.key("item_stack"));
final LiteralCommandNode<CommandSource> literal = LiteralArgumentBuilder
        .<CommandSource>literal("give")
        .then(argument("item", itemType))
        .build();

The execution of a command whose parse results contain an argument node with an opaque type is automatically forwarded to the backend server (since the proxy doesn't know how to actually parse the argument). This is done by forcing an opaque type to consume all of the remaining input. VelocityCommandManager then checks if the parse results contain such a node, and if so returns false in executeCommand. The suggestion provider needs no changes, since the suggestions of an opaque argument type are already provided by the client, which doesn't ask the server.

We provide ~~two~~ one way to obtain an OpaqueArgumentType: ~~either~~ by its Minecraft <=1.18 string identifier, ~~or by its Minecraft >=1.19 version-dependent numeric identifier~~. The registration on the ArgumentPropertyRegistry class is reused. ~~The OpaqueArgumentType provides no methods to retrieve the string nor numeric identifiers directly; it is unlikely for a plugin to need access to these methods, and it prevents breaking the API if Mojang decides to change the identifier format yet again.~~

~~Remaining work~~

~~We only handle property-less parser types for now; We could unify opaque types with ModArgumentProperty once we figure out how we want API users to specify these properties. Perhaps a builder with writeVarint/Double/String/etc. methods could work? This might be going too far, however. At which point are we just rewriting buffer accessors? The API doesn't have access to Netty ByteBufs, but regular byte[]s or ByteBuffers could do the job. This is further motivated by the small size of the properties for all vanilla parser types.~~ The OpaqueArgumentType interface now has a PropertySerializer sub-interface that can be used to produce a byte array containing the serialized form of the properties for a specific ProtocolVersion. This has been checked both through unit tests and in-game. The javadoc notes that users are discouraged from implementing the PropertySerializer for vanilla parser types directly; although it makes no explicit mention about any library, I've released opaque-argument-types, soon to be available in Maven Central [Update: already published to Maven Central].

You might be wondering what's the reason for not providing these directly in Velocity. The property serialized form and the string identifiers may change between Minecraft versions, so they would constitute API breakage. Publishing these as a separate library published as snapshots in a Maven repo allows us/me to introduce breaking changes freely. In addition, this is yet another aspect of the game Velocity must update on each update. Of course, plugin authors can use the methods in Velocity to construct their own PropertySerializers; it's just that maintaining these across versions might become painful in the future. I'm open to moving these to API if you believe otherwise.

hugmanrique avatar Feb 05 '23 02:02 hugmanrique

I should add here: Velocity has the identifier namespaced:key for all types regardless of version. Even for 1.19+. This is required anyway if we want to interact with the singable registry. So exposing the ID is redundant / not a good idea imo.

Xernium avatar Feb 05 '23 16:02 Xernium

Ref: see https://github.com/PaperMC/Velocity/blob/8761d02def69d9c2ed8e026ba348a7468a4c6d7b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/brigadier/ArgumentPropertyRegistry.java#L174 for the registry.

Xernium avatar Feb 05 '23 16:02 Xernium

Yes, VelocityCommandManager uses that registry when fetching parser types (both by string and by integer). Will remove the integer variants :+1:

hugmanrique avatar Feb 05 '23 16:02 hugmanrique