Commands: Fix Bug, Refactor Arguments, Add /tp

[user622628252416]

Description

• Fix bug in command dispatch
• refactor command argument parsers: replace one-dimensional posititon by position2d and position3d
• support relative coordinates in position3d (using ~)
• refactor and implement arg_player/arg_entity
• add arg_entities as a placeholder for when other entities than players are a thing
• remove some curly braces where they are not necessary
• add teleport command (#15)
• add bounded number argument type
• replace simple args in cmd_worldborder with appropriate number arg types
• move args to seperate module/folder
• remove second argument parsing step for better performance and more concise code
• fix #210

Testing

ran commands and evaluated their effects a lot (especially affected commands: help/tp/worldborder)

Checklist

• [x] Code is well-formatted and adheres to project style guidelines: cargo fmt
• [x] Code does not produce any clippy warnings cargo clippy

[Snowiiii]

I would prefer to have all arguments in a folder like the commands, makes it much cleaner

[user622628252416]

I would prefer to have all arguments in a folder like the commands, makes it much cleaner

I've already been working on that actually, but before I can commit that I need another issue to be resolved: IMO ArgumentConsumer's consume function should just return an Option instead of a Result<String, Option>. As discussed in the comment in CommandDispatcher, we can really use these optional error strings anyways, at least not without a lot of complexity that will slow us down.

See dispatcher.rs

is_fitting_path being Err only means that THIS path does not fit Do not break/return here or else other paths aren't even tried! I don't really know what to do with this _error string, because there will be one error for every path that was tried. Since there are sometimes many possible paths, aggregating these errors and showing them all to the user probably does not make sense. We can't really know which path the user intended, and just show the error for that one path, either. (we could guess but that'd be hard). IMO is_fitting_path should therefore just be a bool.

Removing this Error in favor of just using an option makes the Argument Consumers much much cleaner too.

Additionally, the aforementioned CommandArgument would be an enum that has a variant for each arguments type: This way we can simply store parsed data in the HashMap, instead of converting consumed data back to a String and having a second parse step.

[Snowiiii]

I would prefer to have all arguments in a folder like the commands, makes it much cleaner

I've already been working on that actually, but before I can commit that I need another issue to be resolved: IMO ArgumentConsumer's consume function should just return an Option instead of a Result<String, Option>. As discussed in the comment in CommandDispatcher, we can really use these optional error strings anyways, at least not without a lot of complexity that will slow us down.

See dispatcher.rs

is_fitting_path being Err only means that THIS path does not fit Do not break/return here or else other paths aren't even tried! I don't really know what to do with this _error string, because there will be one error for every path that was tried. Since there are sometimes many possible paths, aggregating these errors and showing them all to the user probably does not make sense. We can't really know which path the user intended, and just show the error for that one path, either. (we could guess but that'd be hard). IMO is_fitting_path should therefore just be a bool.

Removing this Error in favor of just using an option makes the Argument Consumers much much cleaner too.

Additionally, the aforementioned CommandArgument would be an enum that has a variant for each arguments type: This way we can simply store parsed data in the HashMap, instead of converting consumed data back to a String and having a second parse step.

Sure, We can use an Option. But i would like to have proper "Error Handling" in some way. I don't want to just give the player back "Wrong Argument", This is confusing.

[user622628252416]

We could go through all possible paths, and find the path that we can traverse the furthest before consuption of an argument fails. I believe that's what Minecraft does. Then we could print the syntax of that path up to that point. This will have to wait tho, as it depends on being able to print possible continuations from any node in a path. This might also be related to command suggestions

[Snowiiii]

We could go through all possible paths, and find the path that we can traverse the furthest before consuption of an argument fails. I believe that's what Minecraft does. Then we could print the syntax of that path up to that point. This will have to wait tho, as it depends on being able to print possible continuations from any node in a path. This might also be related to command suggestions

I belive this is what we did before i made custom error reporting. Im not totally against this. But for "not experienced players". An proper error message like "Player not Found". Is more clear than " wrong Argument" or something like that

[user622628252416]

But for "not experienced players". An proper error message like "Player not Found". Is more clear than " wrong Argument" or something like that

I honestly think when #94 is implemented this is not really a problem, or at least relatively low priority. If you think this is important you could open a seperate issue for that

[Snowiiii]

btw you have a typo you wrote arg_postition_2d.rs

[Snowiiii]

Is this ready for merge ?

[user622628252416]

I just did some last testing, I think it's ready 👍

[Snowiiii]

Big Thanks @user622628252416. Really appreciate your work :D