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

RFC: Std I/O driven application (aka amethyst_commands) #13

Open
azriel91 opened this issue Oct 6, 2018 · 7 comments
Open

RFC: Std I/O driven application (aka amethyst_commands) #13

azriel91 opened this issue Oct 6, 2018 · 7 comments

Comments

@azriel91
Copy link
Member

azriel91 commented Oct 6, 2018

Issue 999, this is gonna be epic!

Summary

Ability to control an Amethyst application using commands issued through stdin, with human-friendly terminal interaction.

Motivation

Inspecting and manipulating the state1 of an application at run time is a crucial part of development, with at least the following use cases:

  • Determining that the application is behaving as expected.
  • Experimenting with new features.
  • Triggering certain cases.
  • Investigating / troubleshooting unexpected behaviour.
  • Automatically driving the application for integration tests.

A command terminal will greatly reduce the effort to carry out the aforementioned tasks.

1 state here means the runtime values, not amethyst::State

Prior Art

Expand -- copied from #995 (warning: code heavy)

okay, so this post is code heavy, but it's how I've done commands in my game (youtube). It shouldn't force people to use the state machine, since event types are "plug in if you need it".

Crate: stdio_view (probably analogous to amethyst_commands)

  • Reads stdin strings, uses shell_words to parse into separate tokens.
  • Parses the first token into an AppEventVariant to determine which AppEvent the tokens correspond to. On success, it sends a tuple: (AppEventVariant, Vec<String>) (the tokens) to an EventChannel<(AppEventVariant, Vec<String>)>.

Changes if put into Amethyst:

  • StdinSystem would be generic over top level types E and EVariant, which would take in AppEvent and AppEventVariant.

Crate: application_event

  • Contains AppEvent and AppEventVariant.

  • AppEvent is an enum over all custom event types, AppEventVariant is derived from AppEvent, without the fields.

    Example:

    use character_selection_model::CharacterSelectionEvent;
    use map_selection_model::MapSelectionEvent;
    
    #[derive(Clone, Debug, Display, EnumDiscriminants, From, PartialEq)]
    #[strum_discriminants(
        name(AppEventVariant),
        derive(Display, EnumIter, EnumString),
        strum(serialize_all = "snake_case")
    )]
    pub enum AppEvent {
        /// `character_selection` events.
        CharacterSelection(CharacterSelectionEvent),
        /// `map_selection` events.
        MapSelection(MapSelectionEvent),
    }

This would be an application specific crate, so it wouldn't go into Amethyst. If I want to have State event control, this will include an additional variant State(StateEvent) from use amethyst_state::StateEvent;, where StateEvent carries the information of what to do (e.g. Pop or Switch).

Crate: stdio_spi

  • StdinMapper is a trait with the following associated types:

    use structopt::StructOpt;
    
    use Result;
    
    /// Maps tokens from stdin to a state specific event.
    pub trait StdinMapper {
        /// Resource needed by the mapper to construct the state specific event.
        ///
        /// Ideally we can have this be the `SystemData` of an ECS system. However, we cannot add
        /// a `Resources: for<'res> SystemData<'res>` trait bound as generic associated types (GATs)
        /// are not yet implemented. See:
        ///
        /// * <https://users.rust-lang.org/t/17444>
        /// * <https://github.com/rust-lang/rust/issues/44265>
        type Resource;
        /// State specific event type that this maps tokens to.
        type Event: Send + Sync + 'static;
        /// Data structure representing the arguments.
        type Args: StructOpt;
        /// Returns the state specific event constructed from stdin tokens.
        ///
        /// # Parameters
        ///
        /// * `tokens`: Tokens received from stdin.
        fn map(resource: &Self::Resource, args: Self::Args) -> Result<Self::Event>;
    }

    Args is a T: StructOpt which we can convert the String tokens from before we pass it to the map function. Resource is there because the constructed AppEvent can contain fields that are constructed based on an ECS resource.

  • This crate also provides a generic MapperSystem that reads from EventChannel<(AppEventVariant, Vec<String>)> from the stdio_view crate. If the variant matches the AppEventVariant this system is responsible for, it passes all of the tokens to a T: StdinMapper that understands how to turn them into an AppEvent, given the Resource.

    /// Type to fetch the application event channel.
    type MapperSystemData<'s, SysData> = (
        Read<'s, EventChannel<VariantAndTokens>>,
        Write<'s, EventChannel<AppEvent>>,
        SysData,
    );
    
    impl<'s, M> System<'s> for MapperSystem<M>
    where
        M: StdinMapper + TypeName,
        M::Resource: Default + Send + Sync + 'static,
        AppEvent: From<M::Event>,
    {
        type SystemData = MapperSystemData<'s, Read<'s, M::Resource>>;
    
        fn run(&mut self, (variant_channel, mut app_event_channel, resources): Self::SystemData) {
        // ...
        let args = M::Args::from_iter_safe(tokens.iter())?;
        M::map(&resources, args)
        // ... collect each event
    
        app_event_channel.drain_vec_write(&mut events);
    }
    }

Crate: character_selection_stdio (or any other crate that supports stdin -> AppEvent)

  • Implements the stdio_spi.

  • The Args type:

    #[derive(Clone, Debug, PartialEq, StructOpt)]
    pub enum MapSelectionEventArgs {
        /// Select event.
        #[structopt(name = "select")]
        Select {
            /// Slug of the map or random, e.g. "default/eruption", "random".
            #[structopt(short = "s", long = "selection")]
            selection: String,
        },
    }
  • The StdinMapper type:

    impl StdinMapper for MapSelectionEventStdinMapper {
        type Resource = MapAssets;         // Read resource from the `World`, I take a `MapHandle` from it
        type Event = MapSelectionEvent;    // Event to map to
        type Args = MapSelectionEventArgs; // Strong typed arguments, rather than the String tokens
    
        fn map(map_assets: &MapAssets, args: Self::Args) -> Result<Self::Event> {
            match args {
                MapSelectionEventArgs::Select { selection } => {
                    Self::map_select_event(map_assets, &selection)
                }
            }
        }
    }
  • The bundle, which adds a MapperSystem<MapSelectionEventStdinMapper>:

    builder.add(
        MapperSystem::<MapSelectionEventStdinMapper>::new(AppEventVariant::MapSelection),
        &MapperSystem::<MapSelectionEventStdinMapper>::type_name(),
        &[],
    );

Can use it as inspiration to drive the design, or I'm happy to push my code up for the reusable parts (stdio_spi should be usable as is, stdio_view probably needs a re-write).

Detailed Design

TODO: discuss

Alternatives

The amethyst-editor will let you do some of the above tasks (inspecting, manipulating entities and components). It doesn't cater for:

  • Server side inspection (e.g. SSH to an application running on a headless server)
  • Automated tests
  • Easy repeatability (source controlled actions)
@azriel91
Copy link
Member Author

azriel91 commented Oct 6, 2018

Previous comments #995:

The amethyst_terminal crate which I will be adding will need to reference the Trans struct. This cannot be done while this struct is in the base crate, as the base crate depends on all other sub crates (cyclic dependency).
Then I need to move everything else for the same reason.
StateMachine -> (Trans -> State) -> StateEvent
and its not possible to go back up the crate hierarchy

@Jojolepro

Why exactly does terminal need to depend on Trans? Can't you make a command system that is generic enough for that ?

@Rhuagh

we would like to keep the state machine an optional component.

@torkleyy

re: amethyst_terminal Trans dependency
The amethyst_terminal crate is the parser that converts the command string into the concrete types that need to be sent into the EventChannels. For example, the command

$ statechange "MainState"

will try to parse the string as ron data, and if it succeeds, it will push a Trans instance through the global EventChannel.

I'm not sure how I could make that generic enough, considering all commands have a different way of being parsed, some have side effects, etc.

Most commands will be in the amethyst_command crate, because that crate will be access by most other crates like core::TransformSystem to check for "Teleportation" events for example.
For the Trans command however, it cannot be inside of amethyst_commands, because that will create a cyclic dependency (almost everything will depend on amethyst_commands, so it cannot depend on anything in return).

The StateMachine will be behind a feature gate. I forgot to include it in the list I made yesterday on discord.

@Jojolepro

StateEvent carries the information of what to do (e.g. Pop or Switch).
@azriel91

That proves why this PR is necessary.

The design you have is effectively what I have on paper. Following rhuagh comments on discord however, the enum containing the different commands will be replaced by something more dynamic. See the opened PR talking about state handle_event made by rhuagh

@Jojolepro

In short, the command crate should be generic enough so different frameworks (e.g. state machine) can register themselves. I like the current structure because it forces us to properly design things and to not couple stuff too much.

@torkleyy

@torkleyy
Copy link
Member

torkleyy commented Oct 6, 2018 via email

@AnneKitsune
Copy link
Contributor

I was planning on having three frontends (or more) for the command stuff:

  • From the terminal (terminal duplex)
  • From the editor (via networking, also adds rpc support)
  • From the in-game ui

@torkleyy
Copy link
Member

torkleyy commented Oct 6, 2018

@Jojolepro "in-game ui"? You mean the one for editing, right?

Independent of that, it should be generic enough so crates can register themselves instead of depending on all of them.

@AnneKitsune
Copy link
Contributor

amethyst_ui? :P

@Rhuagh
Copy link
Member

Rhuagh commented Oct 12, 2018

By making a generic framework where anything can register their commands we also upon up for the user to have terminal commands added from the game logic.

@fhaynes
Copy link
Member

fhaynes commented Jan 8, 2019

Transferring this to the RFC repo.

@fhaynes fhaynes transferred this issue from amethyst/amethyst Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants