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: simple_system macro #498

Closed
Xaeroxe opened this issue Oct 23, 2018 · 11 comments
Closed

RFC: simple_system macro #498

Xaeroxe opened this issue Oct 23, 2018 · 11 comments

Comments

@Xaeroxe
Copy link
Member

Xaeroxe commented Oct 23, 2018

Summary

Provide a macro to simplify the creation of unit structure systems. i.e.

simple_system! {
    pub Test(arg: Read<'s, i32>, arg2: Write<'s, u32>) {
        *arg2 = 5;
        println!("{}", *arg);
    }
}

Motivation

System creation is one of the first things a new specs user has to interact with in order to do anything, meaning creating these in a low verbosity and straightforward manner should be a high priority goal.

Guide-level explanation

We need to create a new system for specs to use. Systems are structures with some execution instructions and metadata attached to them. Many systems don't need to store any data inside their own structure though, so for these we've provided a macro to simplify the declaration called simple_system!. This macro makes declaring the system very similar to declaring a function. First we enter the macro body:

simple_system! {

}

Then inside here we need first, the visibility of the system, i.e. "pub" or "pub(crate)". Or if you just want it private, you can omit this. Then after that we'll need the name of the system. Rust structure names are usually written in PascalCase i.e. MyCoolSystem. So thus far we should have something like

simple_system! {
    pub MyCoolSystem
}

After the name we'll take the resource parameters the system needs, surrounded by parentheses. This is where systems start to differ from a typical Rust function, the dispatcher needs to know what mutability you're expecting for your parameters. You specify this with Read and Write for resources. You'd use ReadStorage and WriteStorage for component storages.

simple_system! {
    pub MyCoolSystem(
        resource: Read<'s, MyResource>,
        writable_resource: Write<'s, MyWritableResource>,
        components: ReadStorage<'s, MyComponent>,
        writable_components: WriteStorage<'s, MyWritableComponent>,
    )
}

But wait! What's 's? That's the system lifetime. Most of the time you won't need this in your system body, but if you need to describe the lifetime of your resources it's exposed there.

Finally, let's add the system body. It's created by adding { and } after the parameter declaration i.e.

simple_system! {
    pub MyCoolSystem(
        resource: Read<'s, MyResource>,
        writable_resource: Write<'s, MyWritableResource>,
        components: ReadStorage<'s, MyComponent>,
        writable_components: WriteStorage<'s, MyWritableComponent>,
    ) {
        println!("I just read this resource: {}!", *resource);
    }
}

(Assuming MyResource has a Display implementation.)

Once you've added the system to a dispatcher with .with(MyCoolSystem) whatever you write inside this body will be executed every time you call dispatch() on the dispatcher.

Reference-level explanation

This proposal would add the following macro to shred.

#[macro_export]
macro_rules! simple_system {
    ($viz:vis $name:ident($($arg:ident: $type:ty),*)$body:block) => {
        $viz struct $name;

        impl<'s> $crate::System<'s> for $name {
            type SystemData = ($($type),*);

            #[allow(unused_mut)]
            fn run(&mut self, ($(mut $arg),*): Self::SystemData) $body
        }
    }
}

This macro utilizes existing traits and capabilities of the library, it just changes the way System structures might be declared. They can still be declared the original way, making this completely backwards compatible.

Drawbacks

The macro somewhat obscures what's going on behind the scenes in an effort to make specs more approachable from a front-end perspective. This can be resolved with good macro documentation, but it might make the fact that a trait is being implemented not quite as apparent.

Rationale and alternatives

The primary rationale is to reduce boilerplate in system declarations. We make a lot of systems so having a dedicated syntax and macro would be a boon.

Here's a comparison of this macro syntax and the current system syntax:

struct MyFirstSystem;

impl<'a> System<'a> for MyFirstSystem {
    type SystemData = ();

    fn run(&mut self, data: Self::SystemData) {
        println!("Hello!");
    }
}

vs

simple_system! {
    pub SimpleSystem() {
        println!("Hello World!");
    }
}

Alternatives:

  • Use a procedural macro instead. It could result in a nicer syntax for the macro but we don't have a Proof of Concept yet.
  • Do nothing. Leave system declaration in a mandatory verbose state.
@azriel91
Copy link
Member

azriel91 commented Oct 24, 2018

POC'ed the first alternative in #499:

#[specs_system(SysA)]
fn sys_a(mut pos: WriteStorage<Pos>, vel: ReadStorage<Vel>) {
    for (pos, vel) in (&mut pos, &vel).join() {
        pos.0 += vel.0;
    }
}

Pros:

  • No magic lifetime 's in the definition (but there is one behind the scenes, probably need to amend)
  • No extra indentation

Cons / Constraints:

  • Raises minimum Rust version to 1.30
  • More complex to implement, therefore higher effort to maintain -- PR doesn't cover all cases such as a tuple parameter, also needs better error checking

if it doesn't make it into specs, I might push it into its own crate since it'll probably be handy for debugging

@Xaeroxe
Copy link
Member Author

Xaeroxe commented Oct 24, 2018

cc @torkleyy

@torkleyy
Copy link
Member

I'm still in favour of putting it into its own crate as I said earlier.

@torkleyy
Copy link
Member

Additional drawback: different syntax for system with field and "simple system".

@randomPoison
Copy link
Member

Personally I think @azriel91's alternative is the way forward. Free-standing functions without needing to declare lifetimes makes systems way more user-friendly, especially to devs who are new to Rust. When I showed Amethyst to a friend of mine who's familiar with Rust but hasn't used it much, he immediately got put off by how complex the definition for a System is. The simple_system! macro as proposed improves the situation, but having bare fns as systems is drastically simpler for new users in my opinion.

A note on implementation: Conceptually, simple_macro! and #[specs_system] do the same thing: They allow you to write a bare fn and use code generation to automate away the boilerplate. But while the proc macro attribute is more complex to implement and maintain, it is also easier for users to use, it can produce better error messages, and is more familiar to users coming from languages with attributes/directives (which will be important as Unity's ECS becomes more widely used, since it heavily uses C# attributes). Overall I think it makes sense for us to start investing heavily in proc macros now that they're going to be stable in 1.30.

@torkleyy
Copy link
Member

Yeah, I wrote on Discord what compromise I'm fine with:

  • #[shred(gen = "FnSystem")] in shred-derive
  • call the standalone function from the generated System implementation

@zakarumych
Copy link
Member

I'd like to remind that lifetime elision rules changes in 2018 edition.

@Xaeroxe
Copy link
Member Author

Xaeroxe commented Oct 25, 2018

Ok, I'm content with the proc-macro approach. Do we have any implementation PRs yet? If not I'll make one when I get back from my vacation.

@torkleyy
Copy link
Member

Not yet, but as you probably know I try to maintain compatibility with older Rust versions, so given that 1.30 just released, I'd like to wait before merging.

@Xaeroxe
Copy link
Member Author

Xaeroxe commented Oct 26, 2018 via email

@Xaeroxe
Copy link
Member Author

Xaeroxe commented Dec 11, 2018

Closing this issue since we've decided to go with the proc macro approach

@Xaeroxe Xaeroxe closed this as completed Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants