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

Command API #8

Open
SpaceWalkerRS opened this issue Jun 30, 2023 · 26 comments
Open

Command API #8

SpaceWalkerRS opened this issue Jun 30, 2023 · 26 comments
Labels
enhancement New feature or request

Comments

@SpaceWalkerRS
Copy link
Member

Discussion about a Command API, features it should have and how they should be implemented.

Some ideas:

  • Add an event for registering commands.
  • Utilities for parsing custom command arguments (for 1.12-).
  • Maybe even going so far as to implement Brigadier in 1.12- and allow mods to use that?
@SpaceWalkerRS
Copy link
Member Author

Maybe adding a framework for client-side commands?

@SRAZKVT
Copy link
Member

SRAZKVT commented Jun 30, 2023

Maybe adding a framework for client-side commands?

imo should have ability to define if a commend only exists on client side, only on server side, or allow on both, but being able to make a client side command is definitely a big plus

@SpaceWalkerRS
Copy link
Member Author

or allow on both

what would that mean in practice, that the command runs both client-side and server-side?

@SRAZKVT
Copy link
Member

SRAZKVT commented Jul 1, 2023

or allow on both

what would that mean in practice, that the command runs both client-side and server-side?

if command available on server, run on server, otherwise, run on client, useful for some basic commands like simulations

@SpaceWalkerRS
Copy link
Member Author

or allow on both

what would that mean in practice, that the command runs both client-side and server-side?

if command available on server, run on server, otherwise, run on client, useful for some basic commands like simulations

I see. I guess we need some communication between the client and server then, to sync which commands are available on each side. Time for a Networking API!

@SRAZKVT
Copy link
Member

SRAZKVT commented Jul 1, 2023

or allow on both

what would that mean in practice, that the command runs both client-side and server-side?

if command available on server, run on server, otherwise, run on client, useful for some basic commands like simulations

I see. I guess we need some communication between the client and server then, to sync which commands are available on each side. Time for a Networking API!

don't the server send client which commands it can do ?

@SpaceWalkerRS
Copy link
Member Author

don't the server send client which commands it can do ?

In 1.13 it does, but in 1.12 and below the client has no idea which commands are available.

@SRAZKVT
Copy link
Member

SRAZKVT commented Jul 1, 2023

don't the server send client which commands it can do ?

In 1.13 it does, but in 1.12 and below the client has no idea which commands are available.

fair, just client and server is probably best then

@SpaceWalkerRS SpaceWalkerRS added the enhancement New feature or request label Jul 2, 2023
@SpaceWalkerRS
Copy link
Member Author

don't the server send client which commands it can do ?

In 1.13 it does, but in 1.12 and below the client has no idea which commands are available.

fair, just client and server is probably best then

I think we can do all 3, we need to communicate between the client and server anyway, in case both the client and server define a command with the same name.

@SRAZKVT
Copy link
Member

SRAZKVT commented Jul 3, 2023

don't the server send client which commands it can do ?

In 1.13 it does, but in 1.12 and below the client has no idea which commands are available.

fair, just client and server is probably best then

I think we can do all 3, we need to communicate between the client and server anyway, in case both the client and server define a command with the same name.

i'd argue that in this case the client should have its command handled, and the server never gets the command. It's a pretty rare occurance, and if it does happen, two commands having the same name will likely do similar things.

@SpaceWalkerRS
Copy link
Member Author

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 1, 2023

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

command suggestions / autocomplete should just be tree traversal no ?

@SpaceWalkerRS
Copy link
Member Author

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

command suggestions / autocomplete should just be tree traversal no ?

we need to combine both Vanilla suggestions and our custom Brigadier suggestions somehow, maybe add the Vanilla commands to the Brigadier tree but only for validation and suggestions but not for parsing? idk

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 1, 2023

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

command suggestions / autocomplete should just be tree traversal no ?

we need to combine both Vanilla suggestions and our custom Brigadier suggestions somehow, maybe add the Vanilla commands to the Brigadier tree but only for validation and suggestions but not for parsing? idk

the suggestions are parsed right from the command tree structure though ? i don't see how there's a problem with parsing

@SpaceWalkerRS
Copy link
Member Author

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

command suggestions / autocomplete should just be tree traversal no ?

we need to combine both Vanilla suggestions and our custom Brigadier suggestions somehow, maybe add the Vanilla commands to the Brigadier tree but only for validation and suggestions but not for parsing? idk

the suggestions are parsed right from the command tree structure though ? i don't see how there's a problem with parsing

I want to avoid running all the vanilla commands through Brigadier, so as to avoid introducing different behavior to Vanilla. So basically leave the Vanilla commands untouched and add custom Brigadier commands on top of that. That way the vanilla commands are not part of the Brigadier command tree.

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 1, 2023

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

command suggestions / autocomplete should just be tree traversal no ?

we need to combine both Vanilla suggestions and our custom Brigadier suggestions somehow, maybe add the Vanilla commands to the Brigadier tree but only for validation and suggestions but not for parsing? idk

the suggestions are parsed right from the command tree structure though ? i don't see how there's a problem with parsing

I want to avoid running all the vanilla commands through Brigadier, so as to avoid introducing different behavior to Vanilla. So basically leave the Vanilla commands untouched and add custom Brigadier commands on top of that. That way the vanilla commands are not part of the Brigadier command tree.

yes ? that's what i said, we can just have an abstract class extending command and having default for all that and we just define the command tree ?

@SpaceWalkerRS
Copy link
Member Author

If we choose to implement Brigadier for 1.12 and below, I think we should leave all Vanilla commands untouched. Porting them all would introduce subtle differences in how they are parsed, which we should avoid imo. This does make it a little trickier as we'll have to run both command systems in tandem. We'll have to figure out things like command suggestions and syntax highlighting.

command suggestions / autocomplete should just be tree traversal no ?

we need to combine both Vanilla suggestions and our custom Brigadier suggestions somehow, maybe add the Vanilla commands to the Brigadier tree but only for validation and suggestions but not for parsing? idk

the suggestions are parsed right from the command tree structure though ? i don't see how there's a problem with parsing

I want to avoid running all the vanilla commands through Brigadier, so as to avoid introducing different behavior to Vanilla. So basically leave the Vanilla commands untouched and add custom Brigadier commands on top of that. That way the vanilla commands are not part of the Brigadier command tree.

yes ? that's what i said, we can just have an abstract class extending command and having default for all that and we just define the command tree ?

Sorry, I don't understand. Can you elaborate a bit more?

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 1, 2023

the command tree would essentially just replace the implementation of the different commands, not replace the command class itself, some extra boilerplate, but i doubt it really matters that much to write a few extra lines of code in case the entire command system is more sane

@zeichenreihe
Copy link

without any idea of how Brigadier works:
couldn't you try parsing the command with Brigadier, and if it fails because it doesn't exist, catch that and let vanilla handle the rest?
Or even better: vanilla has definitely something like a Set<String> or Map<String, ? super Command> storing the command names, so you can figure out if there's a vanilla command that would match (commandString.startsWith(vanillaCommandName + " ")), and if that is true then don't let Brigadier handle the command. This aproach would also not break other command registered in the "old" way, via mixin and registering it.

@SpaceWalkerRS
Copy link
Member Author

the command tree would essentially just replace the implementation of the different commands, not replace the command class itself, some extra boilerplate, but i doubt it really matters that much to write a few extra lines of code in case the entire command system is more sane

So if I understand correctly, the Brigadier tree would contain all Vanilla commands as well as any commands provided by mods, but Vanilla command execution would still be handled by the Vanilla command classes?

Or even better: vanilla has definitely something like a Set<String> or Map<String, ? super Command> storing the command names, so you can figure out if there's a vanilla command that would match (commandString.startsWith(vanillaCommandName + " ")), and if that is true then don't let Brigadier handle the command. This aproach would also not break other command registered in the "old" way, via mixin and registering it.

That's roughly how I envisioned it I think. Combined with the idea above we could then rely entirely on the Brigadier tree for command suggestions and syntax highlighting. Although I think there is an argument to be made we should also do Vanilla-style autocomplete in addition to the Brigadier provided suggestions. This is because the Brigadier provided suggestions require both the server and client to have a copy of the command tree. If we want to keep the modded server compatible with Vanilla clients we need to be able to default back to the Vanilla implementation for autocomplete even for custom commands.

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 2, 2023

the command tree would essentially just replace the implementation of the different commands, not replace the command class itself, some extra boilerplate, but i doubt it really matters that much to write a few extra lines of code in case the entire command system is more sane

So if I understand correctly, the Brigadier tree would contain all Vanilla commands as well as any commands provided by mods, but Vanilla command execution would still be handled by the Vanilla command classes?

that is completely unnecessary, we can just let all vanilla commands untouched and have the command tree implementations directly called inside the command classes, just like we would in vanilla, as boilerplate, rather than have to write the logic directly.

Or even better: vanilla has definitely something like a Set<String> or Map<String, ? super Command> storing the command names, so you can figure out if there's a vanilla command that would match (commandString.startsWith(vanillaCommandName + " ")), and if that is true then don't let Brigadier handle the command. This aproach would also not break other command registered in the "old" way, via mixin and registering it.

That's roughly how I envisioned it I think. Combined with the idea above we could then rely entirely on the Brigadier tree for command suggestions and syntax highlighting. Although I think there is an argument to be made we should also do Vanilla-style autocomplete in addition to the Brigadier provided suggestions. This is because the Brigadier provided suggestions require both the server and client to have a copy of the command tree. If we want to keep the modded server compatible with Vanilla clients we need to be able to default back to the Vanilla implementation for autocomplete even for custom commands.

Brigadier doesn't store anything. Brigadier doesn't change anything. There is nothing here that is of any issue of just incorporating a brigadier like api. Vanilla clients will request autocomplete to servers (just tested with tapestry, i don't have tapestry on my client, and i get tapestry commands autocomplete, do need to tab, but that's necessary pre 1.13 anyway). We don't need to work around vanilla commands at all, we can just use the exact same mechanism as vanilla commands, and everything will just work out, it's the same structure : we have a class with command implementation and we register it.

@SpaceWalkerRS
Copy link
Member Author

the command tree would essentially just replace the implementation of the different commands, not replace the command class itself, some extra boilerplate, but i doubt it really matters that much to write a few extra lines of code in case the entire command system is more sane

So if I understand correctly, the Brigadier tree would contain all Vanilla commands as well as any commands provided by mods, but Vanilla command execution would still be handled by the Vanilla command classes?

that is completely unnecessary, we can just let all vanilla commands untouched and have the command tree implementations directly called inside the command classes, just like we would in vanilla, as boilerplate, rather than have to write the logic directly.

Huh I think I see what you mean now. Your idea is to implement a Brigadier-like tree within the old command system, thus a custom command just extends the Command class but stores a Brigadier-like tree for its own command structure. Is that right?

This is different from how I envisioned it. My idea was to implement the 1.13 system alongside the old system. So when you register a Brigadier command it's not wrapped inside the old system or anything like that, but effectively the same as you would do it in 1.13. When running a command, the command handler would first check if an old-style command of that name exists, and if so, run that, and otherwise call the Brigadier dispatcher to run the command.

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 2, 2023

Huh I think I see what you mean now. Your idea is to implement a Brigadier-like tree within the old command system, thus a custom command just extends the Command class but stores a Brigadier-like tree for its own command structure. Is that right?

yes, would make it much easier imo, going out of our way to make it more similar to 1.13 system isn't worth it imo

This is different from how I envisioned it. My idea was to implement the 1.13 system alongside the old system. So when you register a Brigadier command it's not wrapped inside the old system or anything like that, but effectively the same as you would do it in 1.13. When running a command, the command handler would first check if an old-style command of that name exists, and if so, run that, and otherwise call the Brigadier dispatcher to run the command.

would work too, but i am not sure if it's worth the effort

@SpaceWalkerRS
Copy link
Member Author

Huh I think I see what you mean now. Your idea is to implement a Brigadier-like tree within the old command system, thus a custom command just extends the Command class but stores a Brigadier-like tree for its own command structure. Is that right?

yes, would make it much easier imo, going out of our way to make it more similar to 1.13 system isn't worth it imo

That's why I thought we'd just plop the Brigadier system alongside the old system. It seems to me that developing a system like it within the old system is more work, especially when Brigadier can be used as is and a lot of the work for the Minecraft-specific argument types and serialization has already been done for us in 1.13.

@SRAZKVT
Copy link
Member

SRAZKVT commented Aug 2, 2023

Huh I think I see what you mean now. Your idea is to implement a Brigadier-like tree within the old command system, thus a custom command just extends the Command class but stores a Brigadier-like tree for its own command structure. Is that right?

yes, would make it much easier imo, going out of our way to make it more similar to 1.13 system isn't worth it imo

That's why I thought we'd just plop the Brigadier system alongside the old system. It seems to me that developing a system like it within the old system is more work, especially when Brigadier can be used as is and a lot of the work for the Minecraft-specific argument types and serialization has already been done for us in 1.13.

wouldn't brigadier itself need some tweaking to work in older versions though ?

@SpaceWalkerRS
Copy link
Member Author

wouldn't brigadier itself need some tweaking to work in older versions though ?

I don't think so, afaik it's a completely stand-alone library. The Minecraft-specific argument types will likely need tweaking from their 1.13 implementations, though, for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants