Command API #351

Jenya705 · 2023-06-02T21:15:18Z

Description

Will solve issue: #332
This is draft, because I want to get feedback on early stage.

Nodes are entities, that are connected with each other using NodeFlow
Executable nodes have NodeSystem component, which contains a bevy's system, which will be executed when the command is executed
Argument nodes have NodeParser component, which contains a boxed trait, which parses reader and puts the parsed object into special struct called ParseResultsWrite
Literal nodes have just NodeName on them (no NodeParser)
Root nodes can have NodeExclude component on them to exclude nodes that this specific root node doesn't want to use
Multiple nodes which can be chosen using EntityNode component (if there is not such component on an entity then PrimaryRootNode will be chosen)
Now commands have a few methods to manipulate with node's entities ( spawn_node, node, spawn_root_node)

Check out example command.rs

dyc3

Very excited to see progress on this! I haven't looked too hard at this yet, just a couple notes.

crates/valence_core/src/game_mode.rs

crates/valence_command/src/reader.rs

crates/valence_command/src/argument.rs

Cargo.toml

crates/valence_core/src/game_mode.rs

crates/valence_nbt/src/compound.rs

crates/valence_command/src/reader.rs

crates/valence_command/src/parser.rs

rj00a · 2023-06-05T13:46:22Z

Not a full review from me yet, but that's what I've noticed so far.

crates/valence_command/src/argument.rs

crates/valence_command/src/parser.rs

crates/valence_command/src/argument.rs

crates/valence_command/README.md

Jenya705 · 2023-07-24T15:57:32Z

I changed the whole system again. Now:

Nodes are entities, that are connected with each other using NodeFlow
Executable nodes have NodeSystem component, which contains a bevy's system, which will be executed when the command is executed
Argument nodes have NodeParser component, which contains a boxed trait, which parses reader and puts the parsed object into special struct called ParseResultsWrite
Literal nodes have just NodeName on them (no NodeParser)
Root nodes can have NodeExclude component on them to exclude nodes that this specific root node doesn't want to use
Multiple nodes which can be chosen using EntityNode component (if there is not such component on an entity then PrimaryRootNode will be chosen)
Now commands have a few methods to manipulate with node's entities ( spawn_node, node, spawn_root_node)

Check out example command.rs

Jenya705 · 2023-07-24T16:12:08Z

What I think should also be done:

Suggestions - the same as execution systems but they will only accept ReadOnlySystemParam
Better error handles - many of error cases are not handled correctly, they just panic
Better safety comments - I didn't write a lot of comments for unsafe blocks
Command macro - a command macro, which will automatically insert all required nodes into Root
Super root node - a root node, which doesn't need to have a specific Entity. The same as NodeRoot but has all not root nodes in it
Node name -> Node entity - helper to get a node by it's name
Parallel execution - we can do the same what bevy does with their systems

dyc3 · 2023-07-24T16:35:54Z

examples/command.rs

+        .with_child(|child| {
+            child.name(Cow::Borrowed("creative")).executor(
+                |In(arguments): In<CommandArguments>, mut query: Query<&mut GameMode>| {
+                    if let RealCommandExecutor::Player(client) = arguments.1 {
+                        if let Ok(mut game_mode) = query.get_mut(client) {
+                            *game_mode = GameMode::Creative;
+                        }
+                    }
+                },
+            );
+        })


Interesting... I'd like to see another example command that implements /tp x y z or /give. It's not really clear to me how one would go about that.

examples/command.rs

JackCrumpLeys · 2023-08-01T00:02:53Z

I know that this is likely completely of the rails compared to the current implementation but cant we just use bevy plugins and events? this way we are tightly coupled with bevy and would make it much nicer to code. I have a few ideas on how this could look here is one (I just typed these up on the textbox so could have errors):

struct TeleportCommand;

impl Command for TeleportCommand {
    fn command(&self) -> impl Into<String> {
        "tp"
    }
    fn params(&self) -> impl Into<Vec<CommandParameter>> { // with CommandParameter being some kind of enum
        vec![CommandParameter::Position]
        // This might also be vec![CommandParameter::Position(None)] or even vec![CommandParameterMarker::Position]
    }
    // ...
}

// ...
app.add_plugins(CommandHandler::from_command(TeleportCommand)) 
// this should send out events that look something like CommandExecutionEvent<TeleportCommand> 
// this event should contain the params.

I have good ideas on how to make this happen and if you think it is a good idea I could work on the code? again I am really just throughing something out into the blue here but I would like to see an as ergonomic as possible API for this.

hwittenborn · 2023-08-01T00:49:12Z

I haven't looked too much into this PR yet, but would it be possible to make command parsing work via the clap crate? I feel like that'd make things a lot simpler and reliable long-term.

Sorry if this PRs already doing that or anything, it was just a thought in my head and I thought I'd go ahead and mention it.

dyc3 · 2023-08-01T00:57:29Z

No, the way that minecraft commands work is not compatible with clap. IIRC, this was explicitly discussed on the thread in discord. Minecraft commands can have cyclic references for suggestions, and parsers for stuff like relative coordinates and target selectors (eg, @p) would be very hard under clap's model.

JackCrumpLeys

I had a little skim of the code and reviewed a little.

JackCrumpLeys · 2023-08-01T00:09:25Z

crates/valence_command/src/boolean.rs

+        answer: &mut crate::suggestions::SuggestionAnswerer,
+        suggestions: Self::Suggestions,
+        command: String,
+        world: &bevy_ecs::world::World,


Accessing the whole world is not a very good idea especially for something as simple as a suggestion.

JackCrumpLeys · 2023-08-02T07:13:01Z

crates/valence_command/src/command.rs

+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum RealCommandExecutor {
+    Player(Entity),
+    Console,


Why would console exist? It should be up to the user of valance to make logic to execute commands though the console. Is this used anywhere in the protocol?

It is not covered by protocol because it has nothing to do with it, console's command executions handled by server anyway (the same as block's command executions). And I think that console can be a command executor, the same as a block.

JackCrumpLeys · 2023-08-02T07:22:25Z

crates/valence_command/src/nodes.rs

+pub(crate) type PCRelationVec = SmallVec<[Entity; 2]>;
+
+#[derive(Component, Debug)]
+pub struct NodeName(pub(crate) Cow<'static, str>);


Why not just store a String?

JackCrumpLeys · 2023-08-02T07:25:01Z

crates/valence_command/src/nodes.rs

+use crate::pkt::{self, RawCommandTreeS2c};
+use crate::suggestions::RawParseSuggestions;
+
+pub(crate) type PCRelationVec = SmallVec<[Entity; 2]>;


Does this ever exceed 2 things? If not why not just use a tuple or array?

It is a vec for parents and children relation, so yeah it can exceed 2 elements

In directed graphs generally you have a source and a target for edges, how is this data able to excede 2 elements?

I don't know how this graph called but each node can have many parents and many children

Generally when you have a graph like this you want to use edges between the nodes. How do you Tel if an item in this list is parent or child?

JackCrumpLeys

Some more comments.

JackCrumpLeys · 2023-08-02T07:37:43Z

crates/valence_command/src/suggestions.rs

+    }
+}
+
+pub const CONSOLE_EVENT_ID: u32 = 0;


Keep consts at top of file

JackCrumpLeys · 2023-08-02T07:39:22Z

crates/valence_command/src/suggestions.rs

+
+#[derive(Clone, Debug)]
+pub struct Suggestion<'a> {
+    pub message: Cow<'a, str>,


Why are you using Cow<'a, str> everywhere?

JackCrumpLeys · 2023-08-02T07:41:45Z

crates/valence_command/src/suggestions.rs

+            SuggestionsTransaction::Event { id } => {
+                self.event.send(SuggestionAnswerEvent {
+                    suggestions: suggestions
+                        .map(|v| v.into_iter().map(|v| v.clone_static()).collect()),


Name v better: suggestion same goes for everywhere you use v.

JackCrumpLeys · 2023-08-02T08:14:19Z

crates/valence_command/src/nodes.rs

+use crate::pkt::{self, RawCommandTreeS2c};
+use crate::suggestions::RawParseSuggestions;
+
+pub(crate) type PCRelationVec = SmallVec<[Entity; 2]>;


In directed graphs generally you have a source and a target for edges, how is this data able to excede 2 elements?

Jenya705 · 2023-08-05T09:39:24Z

Suggestions are done. @rj00a, can you review current code? If you have any questions feel free to ask

rj00a · 2023-08-05T23:21:32Z

Sorry, I might be a bit slow to get to a review.

JackCrumpLeys · 2023-10-27T02:03:39Z

This should be closed or renamed as #446 has been merged.

JackCrumpLeys · 2024-06-06T07:24:36Z

@rj00a this should be closed.

Jenya705 added 5 commits May 7, 2023 09:13

Some descriptions

ff803cd

Some bugs

27f11bf

Some objects

12b1ce3

primitives, vectors, relative values, angles

365d1c6

BlockPredicate

858892e

rj00a self-requested a review June 3, 2023 05:22

Jenya705 added 2 commits June 3, 2023 11:28

Merge branch 'valence-rs:main' into brigadier

da177ba

merge fix

5258fb0

dyc3 reviewed Jun 3, 2023

View reviewed changes

crates/valence_core/src/game_mode.rs Outdated Show resolved Hide resolved

crates/valence_command/src/reader.rs Outdated Show resolved Hide resolved

Jenya705 added 5 commits June 4, 2023 17:28

Some more api and Block argument

f58bf16

Merge branch 'main' into brigadier

de3179d

CEnum trait

6f87ba6

Merge branch 'brigadier' of github.com:Jenya705/valence into brigadier

5f06bff

GameMode

ffa9a3a

rj00a reviewed Jun 5, 2023

View reviewed changes

crates/valence_command/src/argument.rs Outdated Show resolved Hide resolved

rj00a reviewed Jun 5, 2023

View reviewed changes

crates/valence_command/src/parser.rs Outdated Show resolved Hide resolved

rj00a reviewed Jun 5, 2023

View reviewed changes

crates/valence_command/src/argument.rs Outdated Show resolved Hide resolved

Jenya705 added 6 commits June 5, 2023 17:13

argument.rs -> many modules

838f0f1

Cenums + fixes

485c5e7

Range

3f10625

StrCursor contains chars and bytes offsets

3bfb1d1

EntitySelector

9a51965

Readme.md, selectors PROP_NAMES

663b88d

dyc3 reviewed Jun 13, 2023

View reviewed changes

crates/valence_command/README.md Outdated Show resolved Hide resolved

Jenya705 added 3 commits June 15, 2023 21:50

Item

79c0116

More convenient parse trait

275f5d8

query -> param

34970c4

rj00a self-requested a review June 17, 2023 13:41

Jenya705 added 5 commits June 18, 2023 20:30

Suggestion change

ba7ff7c

Merge branch 'valence-rs:main' into brigadier

8492d70

Command exec system

dc61fd4

Right command node tree and suggestion's event

dbd358c

Callback execution systems, real nodes, compilation

d205de1

dyc3 reviewed Jul 24, 2023

View reviewed changes

Integer's parsers and a small tp command

aa0a1b5

dyc3 reviewed Jul 24, 2023

View reviewed changes

examples/command.rs Outdated Show resolved Hide resolved

Jenya705 added 2 commits July 24, 2023 19:50

tp zfloat, to prove that multiple parsers work

1c555c2

Merge branch 'valence-rs:main' into brigadier

6d5ce1b

JackCrumpLeys reviewed Aug 2, 2023

View reviewed changes

Suggestions

fbfe2b9

path fixes

2ea8f51

Jenya705 marked this pull request as ready for review August 5, 2023 10:03

Jenya705 marked this pull request as draft August 5, 2023 10:04

Parse::Item, last_entity fix

e3ba4be

Nodes are now in graph

30da824

rj00a mentioned this pull request Aug 15, 2023

Add support for commands & completion #35

Closed

JackCrumpLeys mentioned this pull request Oct 1, 2023

Cryptographic chat support #303

Open

rj00a closed this Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command API #351

Command API #351

Jenya705 commented Jun 2, 2023 •

edited

Loading

dyc3 left a comment

rj00a commented Jun 5, 2023

Jenya705 commented Jul 24, 2023 •

edited

Loading

Jenya705 commented Jul 24, 2023 •

edited

Loading

dyc3 Jul 24, 2023

JackCrumpLeys commented Aug 1, 2023

hwittenborn commented Aug 1, 2023

dyc3 commented Aug 1, 2023 •

edited

Loading

JackCrumpLeys left a comment

JackCrumpLeys Aug 1, 2023

JackCrumpLeys Aug 2, 2023

Jenya705 Aug 2, 2023

JackCrumpLeys Aug 2, 2023

JackCrumpLeys Aug 2, 2023

Jenya705 Aug 2, 2023

JackCrumpLeys Aug 2, 2023

Jenya705 Aug 2, 2023

JackCrumpLeys Aug 2, 2023

JackCrumpLeys left a comment

JackCrumpLeys Aug 2, 2023

JackCrumpLeys Aug 2, 2023

JackCrumpLeys Aug 2, 2023

JackCrumpLeys Aug 2, 2023

Jenya705 commented Aug 5, 2023 •

edited

Loading

rj00a commented Aug 5, 2023 •

edited

Loading

JackCrumpLeys commented Oct 27, 2023

JackCrumpLeys commented Jun 6, 2024

Command API #351

Command API #351

Conversation

Jenya705 commented Jun 2, 2023 • edited Loading

Description

dyc3 left a comment

Choose a reason for hiding this comment

rj00a commented Jun 5, 2023

Jenya705 commented Jul 24, 2023 • edited Loading

Jenya705 commented Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

JackCrumpLeys commented Aug 1, 2023

hwittenborn commented Aug 1, 2023

dyc3 commented Aug 1, 2023 • edited Loading

JackCrumpLeys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackCrumpLeys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jenya705 commented Aug 5, 2023 • edited Loading

rj00a commented Aug 5, 2023 • edited Loading

JackCrumpLeys commented Oct 27, 2023

JackCrumpLeys commented Jun 6, 2024

Jenya705 commented Jun 2, 2023 •

edited

Loading

Jenya705 commented Jul 24, 2023 •

edited

Loading

Jenya705 commented Jul 24, 2023 •

edited

Loading

dyc3 commented Aug 1, 2023 •

edited

Loading

Jenya705 commented Aug 5, 2023 •

edited

Loading

rj00a commented Aug 5, 2023 •

edited

Loading