valence icon indicating copy to clipboard operation
valence copied to clipboard

Command API

Open Jenya705 opened this issue 1 year ago • 9 comments

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

Jenya705 avatar Jun 02 '23 21:06 Jenya705

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

rj00a avatar Jun 05 '23 13:06 rj00a

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 avatar Jul 24 '23 15:07 Jenya705

What I think should also be done:

  • [x] 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

Jenya705 avatar Jul 24 '23 16:07 Jenya705

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.

JackCrumpLeys avatar Aug 01 '23 00:08 JackCrumpLeys

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.

hwittenborn avatar Aug 01 '23 00:08 hwittenborn

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.

dyc3 avatar Aug 01 '23 00:08 dyc3

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

Jenya705 avatar Aug 05 '23 09:08 Jenya705

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

rj00a avatar Aug 05 '23 23:08 rj00a

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

JackCrumpLeys avatar Oct 27 '23 02:10 JackCrumpLeys

@rj00a this should be closed.

JackCrumpLeys avatar Jun 06 '24 07:06 JackCrumpLeys