Alternative low boilerplate system syntax

[cart]

First: this is a really cool project and I've enjoyed playing with it so far :)

Current system declarations

Right now there are three ways to declare systems and each has its own tradeoffs:

Struct with System trait impl

struct DoThing;
impl<'a> System<'a> for DoThing {
    type Data = (&'a Position, &'a mut Health);
    fn run((positions, mut healths): <Self::Data as SystemData>::View) {
        (&positions, &mut healths).iter().for_each(|(position, mut health)| {
            // do stuff here
        });
    }
}

// later
world.run_system::<DoThing>();

• Pros: Pure rust. Portable. Extends nicely to other scenarios like System Setup #34
• Cons: Lots of boilerplate

Function with macro

#[system(DoThing)]
fn run(position: &Position, mut health: &mut Health) {
    (&position, &mut health).iter().for_each(|(position, mut health)| {
        // do stuff here
    });
}

// later
world.run_system::<DoThing>();

• Pros: Low boilerplate. Portable
• Cons: Domain specific language that isn't fully clear on types and breaks IDEs

Anonymous function

world.run::<(&Position, &mut Health),_,_>(|(positions, healths)| {
    (&positions, &mut healths).iter().for_each(|(position, mut health)| {
        // do thing
    });
});

• Pros: Low boilerplate. Pure rust
• Cons: Needs to be declared within the context of a world / less portable. Requires extra type annotations. Nameless

Proposed system declaration

fn do_thing<'a>((positions, mut healths): (View<'a, Position>, ViewMut<'a, Health>)) {
    (&positions, &mut healths).iter().for_each(|(position, mut health)| {
        // do thing
    });
}

// later
world.run_fn(do_thing);

Additionally I think the lifetime could be elided in this case:

fn do_thing((positions, mut healths): (View<Position>, ViewMut<Health>)) {
    (&positions, &mut healths).iter().for_each(|position, mut health| {
        // do thing
    });
}

// later
world.run_fn(do_thing);

• Pros: Dead simple and very low boilerplate (especially with elided lifetime). Pure rust. No type abstractions or DSLs. Portable
• Cons: Works with views (View<Positition>) instead of type references (&Position). But I view this as a win because its honest about the types being used / has parity with other storages like Unique<T>.

I personally think this the most ergonomic / straightforward solution (provided its actually possible).

It also looks like this couldn't build off of the existing Run trait because it relies on T::SystemData, which assumes types are declared as (&Position, &mut Health) instead of (View<Position, ViewMut<Health>). But implementing this seems doable because it is actually less abstraction over the types used to run the system.

Does this seem doable? Is it desirable?

[cart]

im realizing now that iter().for_each() is not optional. looks like normal for loops aren't possible with shipyard. I'll edit the examples above to accommodate that fact.

[leudz]

Hi! Thank you, that's very nice of you =)
Let's ping @almetica just in case, they're probably interested in this issue too.

I tried to do something like this in the past and systems might be functions in the end (or at least they'll be accepted too). I think my attempt was with &/&mut, it might work if the type doesn't change.
With GAT it would be easy, negative trait bounds and specialization would also be great. When all these features are available, the library will likely be very different.

Using "actual types" is another topic.
You're the second person to bring this up so I'll explain in great detail here why I designed it this way to link here if someone else brings it up. And if you (or anyone) can refute my arguments, I might change my mind 😄

Currently you can use 6 "storage types" and 4 modifiers in order to explain what you want from the World. None of them or any combination will return the type you put in Data nor a reference to said type.
Furthermore, modifiers (like Unique) are never created. They're defined like this:

pub struct Unique<T: ?Sized>(T);
pub struct NonSend<T: ?Sized>(T);
pub struct NonSync<T: ?Sized>(T);
pub struct NonSendSync<T: ?Sized>(T);

I could use PhantomData and they would work exactly the same.

The chapter in the guide probably doesn't have the right name and is misleading. Currently unique storages are stored in a SparseSet, just like any component and it doesn't really matter at the end of the day. With #51 it won't be the case anymore but they still won't be stored in a Unique.

Despite all that, Unique would still work with your approach, we'd use UniqueView or UniqueViewMut.
The problems arise with the other 3 modifiers and the future 5th modifier.
NonSend, NonSync, NonSendSync and Local don't modify the type you get back. I can imagine three workarounds:

• remove all of them - that would solve the issue 😄 but we'd loose important functionalities
• wait for specialization and rework Local - !Send and !Sync can be removed with specialization (even though I don't think it's a good idea) and Local would be a wrapper type
• make them return a different type - that's probably the best workaround, it would complicate shipyard's internal code but abusing a bit AsRef/AsMut/Deref might be enough to not make it a burden on the user side

ThreadPool would also have to change as it is currently defined as:

pub struct ThreadPool;

Regarding & and &mut, I thought it would be easier to understand than Read/Write or View/ViewMut. Even though many new rustaceans think of them as pointers or in term of mutability, it's a bad idea and multiple people fight this concept on the forum. Some even want(ed) to use &shared and &uniq or something like this.

This concept is exactly the same in an ECS, you have shared and exclusive borrow, to components this time. I concede that having &usize refer to a unique storage and Many<&usize> refer to a regular storage might be less surprising for some people. But regular storages are used far more often and that's what I'd expect an ECS default to be, plus Unique came later.

Lastly I just want to point out that the macro would still be useful. Not as much as today but would still have a few advantages:

• identifier and type side by side
• infinite number of storage without having to think about it
• wrong borrowing caught at compile time

If systems become simpler, it might become an opt-in feature instead of opt-out.
It's a shame that it breaks some IDE plugins but I'm confident they'll get there.

---

Regarding for loops you can use them but you have to add an into_iter call when you're done setting up the iteration.

for (position, mut health) in (&positions, &mut healths).iter().into_iter() {
    // do thing
}

This function will transform a Shiperator to a std::iter::Iterator. I should probably rename it into_iterator.

[cart]

Thanks for the in depth response! I really appreciate that you took the time to explain everything.

• New rust features: yeah i'm also waiting for a lot of these. I'm already being "bad" and using specialization in one of my projects 😄. But I think in general it makes sense to wait for them to hit stable, especially for a core library like this one. It will definitely be cool to see if/how shipyard adapts when they land.
• UniqueView: That would be my preferred interface I think.
• "Showing the real type" / Storage Types: I definitely like that the backing storage is currently abstracted out from systems. If you change from a "tight pack" to a "loose" or "update" pack, you don't need to update your systems. I'm definitely not suggesting that you change that aspect of shipyard.
  • The main issues I have with the current macro are:
    • The semantics you are using already mean something in rust. &Health already reads as "a borrowed health struct". It is very confusing to overload that to mean "a borrowed shipyard component storage". I imagine it would be especially confusing for people new to the language.
    • Domain specific language: using this code means we're no longer writing "Rust", we're writing "Rust/Shipyard". I don't think this use case justifies "language additions". However this is largely an aesthetic argument. Other people might not have the same hangups as me in this regard.
  • I do agree with the points you made on the additional value a macro can provide, but in my opinion that value does not outweigh the above costs. Fortunately the macro is optional and everyone can make their own cost/benefit analysis. I would personally be more inclined to use the macro if it read like "standard" rust. But in general im opposed to any macro that isn't deriving a trait or generating "list" implementations for types arguments. like impl Trait for (A,) {}, impl Trait for (A,B) {}, etc
• storage modifiers: This does seem like a problem for my proposal. We're at the point where I don't know enough about that part of shipyard to have a good opinion on workarounds. My preference here would be additional types that line up with View/ViewMut. In my case I mainly just want View/ViewMut/UniqueView/UniqueViewMut with the interface above, so I would be fine if the other modifiers couldn't be expressed in the proposed "function style" declaration. But im sure others would feel differently.
• std iterators: ooh awesome thats good to know! whats the rationale behind not returning an std iterator by default when calling .iter()?

In general it sounds like we're on the same page in most regards. Thanks again for your time!

[almetica]

  * The main issues I have with the current macro are:
    
    * The semantics you are using already mean something in rust. `&Health` already reads as "a borrowed health struct". It is very confusing to overload that to mean "a borrowed shipyard component storage". I imagine it would be especially confusing for people new to the language.
    * Domain specific language: using this code means we're no longer writing "Rust", we're writing "Rust/Shipyard". I don't think this use case justifies "language additions". However this is largely an aesthetic argument. Other people might not have the same hangups as me in this regard.

I think @cart is actually explaining the issue I have with the macro much more elegant. I actually switched back to the first, explicit approach to define the systems, since using this DSL, that's changing the meaning of standard rust language, breaks my cognitive flow and the usability of my programming environment.

[leudz]

I've been thinking about the storage abstraction because of shared components recently. I agree that a single View type for regular storages is a good thing. The enum might switch places but it'll still be View/ViewMut regardless of the pack.

My goal with &/&mut was to make it easy to understand, obviously it's not the case for everyone.
I'll try to make the functional approach work, if it works, great! I'll just have to think if it should replace the current trait or be in addition to it.
If it isn't possible yet, I think we can reach an acceptable compromise: add all actual types to the list.
This way if you're weirded out by the references you can use the types directly and the others can keep a more sugary experience.
For modifiers that don't modify the storage I can keep wait a bit, maybe a better workaround will come up.

Std's Iterator is fairly flexible but not enough sadly. While you can choose the behavior of all its methods you're stuck with their return type.
This becomes an issue with filter on an update pack because I can't modify std::iter::Filter. I thought about redesigning update pack to make it work with it but events have the same problem and I won't be able to fix those.
Once Shiperator has done its part there's no problem going back to an Iterator.
Until today I tried a few ways but got this:

conflicting implementations of trait `std::iter::Iterator` for type `&mut _`:
conflicting implementation in crate `core`:
- impl<I> std::iter::Iterator for &mut I
  where I: std::iter::Iterator, I: ?Sized;

Or this:

conflicting implementations of trait `std::iter::IntoIterator`:
conflicting implementation in crate `core`:
- impl<I> std::iter::IntoIterator for I
  where I: std::iter::Iterator;

But I just realized: I don't have to do something so generic.
Shiperator is still necessary but soon™ it'll work with for loops seamlessly. This is awesome!

---

So I'm currently finishing the last chapter of the guide. When this is done I'll try to get a POC functional system to work and I'll report back to this issue.
Regardless of the result of this experiment, if you're on board with the "actual type" proposal I'll implement that. If the experiment is successful it won't be implemented right away so you can still enjoy a reference-free life 😄 What do you say?

[cart]

Haha your response makes me very happy. That all sounds brilliant.

[almetica]

Indeed. This sounds like a good plan. Happy to test it in the future.

[leudz]

I got way too curious 😄 I continued to think about it and came to the conclusion that this issue will change the library way too much if it works so I had to test.

This is the result:

use shipyard::*;

fn add_entities((mut entities, mut usizes): (EntitiesViewMut, ViewMut<usize>)) -> [EntityId; 3] {
    [
        entities.add_entity(&mut usizes, 0),
        entities.add_entity(&mut usizes, 1),
        entities.add_entity(&mut usizes, 2),
    ]
}

#[test]
fn test() {
    let world = World::new();
    let [entity1, entity2, entity3] = world.run_functional(add_entities);
    world.run_functional(|usizes: View<usize>| {
        assert_eq!(usizes.get(entity1), Ok(&0));
        assert_eq!(usizes.get(entity2), Ok(&1));
        assert_eq!(usizes.get(entity3), Ok(&2));
    });
}

run_functional is just a placeholder name of course.

So the question is: and now?

The compiler's type inference isn't good enough to have the same code work seamlessly with references.
I think there's two way to go about it: duplicate run, borrow and all functions related to systems or throw the reference syntax out the window.
I talked about it with @dakom and @eldyer on zulip and we came to the conclusion that the latter option is probably the best.
I still have to check that everything will work with this transition. I'll have to make special types for some modifiers and maybe other changes.

Returning values from workloads came up and I'll also look into it.

Since this change will change a lot of things it'll be released a lot sooner than what I said earlier.
It takes priority over everything else and will be released soon after it's done, assuming everything works of course.

[dakom]

Really appreciate your careful thought on this, checking in with the community (small as it is right now), and ability to prioritize and pivot! Thanks @leudz !

[cart]

That is excellent news. I also really appreciate the quick turnaround and your openness to community feedback.

[leudz]

I tried to get workloads to work but I don't think it's possible without GAT.
I had the feeling I already done all that so I went back to find my last experiment.

this weekend I tried to do something like this (with a proc macro to hide ViewMut):

fn add1((usizes, u32s): (ViewMut<usize>, ViewMut<u32>)) {
    (usizes, u32s).iter().for_each(|(x, y)| {
        *x += 1;
        *y += 1;
    });
}

let mut world = World::new::<(usize, u32)>();
world.run::<(EntitiesMut, &mut usize, &mut u32), _>(|(mut entities, mut usizes, mut u32s)| {
    entities.add_entity((&mut usizes, &mut u32s), (0, 1));
});

world.add_new_system(add1);
world.run_new_system();

but the compiler doesn't understand what I'm doing and it can't find a correct lifetime...
so I looked at what legion is doing and this works (with closures too):

fn function_system() {
    fn add1(world: &World) {
        world.run::<(&mut usize, &mut u32), _>(|(usizes, u32s)| {
            (usizes, u32s).iter().for_each(|(x, y)| {
                *x += 1;
                *y += 1;
            });
        });
    }

    let mut world = World::new::<(usize, u32)>();
    world.run::<(EntitiesMut, &mut usize, &mut u32), _>(|(mut entities, mut usizes, mut u32s)| {
        entities.add_entity((&mut usizes, &mut u32s), (0, 1));
    });

    world.add_new_system(add1, &[TypeId::of::<usize>(), TypeId::of::<u32>()]);
    world.run_new_system();
}

I really don't like the fact that add_new_system requires users to define the access they'll use inside the system, plus if it's incorrect run_new_system might stop with an error

This is from december 10th, as you can see this is the same function based approach you proposed.

I tried a new workaround with async/await under the hood:

fn main() {
    let world = World::new();
    world
        .add_workload("Test")
        .with_system(add_entity)
        .with_system(add_entity)
        .build();
    world.run_default();
}

fn add_entity(mut entities: EntitiesViewMut) {
    dbg!(entities.add_entity((), ()));
}

It works great... once.
Because the functions are called immediately, there's no problem with GAT, we only need a single lifetime.
But if I try to store the function that generates the Future, I'm back to GAT.

Here's what I think are the two best workarounds:

fn main() {
    let world = World::new();
    world
        .add_workload("Do things")
        .with_system(|world: &World| world.run(add_entity), add_entity)
        .with_system(|world: &World| world.run(print), print)
        .build();

    world.run_default();
}

fn add_entity(mut entities: EntitiesViewMut) {
    entities.add_entity((), ());
}

fn print() {
    println!("Done");
}

This is the same thing as what I did back in december but less error prone.

Workaround 2:

fn main() {
    let world = World::new();
    world
        .add_workload(
            "Do things",
            (
                add_entity_system,
                print_system,
            ),
        );
    // with async closure (not stable yet)
    world
        .add_workload(
            "Do things",
            (
                async |world: &World| { world.async_run(add_entity).await; },
                async |world: &World| { world.async_run(print).await; },
            ),
        );

    world.run_default();
}


fn add_entity(mut entities: EntitiesViewMut) {
    entities.add_entity((), ());
}

fn print() {
    println!("Done");
}

async fn add_entity_system(world: &World) {
    world.async_run(add_entity).await;
}
async fn print_system(world: &World) {
    world.async_run(print).await;
}

This workaround is very different from the previous one or what shipyard does today. I'll have to modify the library a lot to make it work somewhat efficiently.

---

So except if a better approach comes up, we'll have to choose between workaround 1, workaround 2 or keep the current trait.

My opinion: I don't think workaround 2 is worth it. I'm very new to async/await, it'll take me between weeks to months to get the library back to where it is today. Plus it's very misleading that it needs an async function when systems can't be async themselves. Once async systems are implemented it'll be totally different.

I don't think workaround 1 is horrible, the repetition could be problematic during refactoring but since they are in the same function call and most likely the same line, I think it's ok.
Under the hood it would work like today so no need for big changes. A macro could even be added to hide the repetition and ensure the two functions are the same.

[eldyer]

Workaround 1 seems totally reasonable to me

[almetica]

Workaround 1 seems fine indeed.

About those async closures: I had to implement a workaround for that, since I use closures to give my test in it's own isolated test database on the fly and handle setup / cleanup (also on panic using panic::catch_unwind()) using local, embedded functions.

[dakom]

Agree w/ Workaround 1.

I like the chaining here, feels a little more explicit about the order systems are run in when they can't be in parallel as opposed to the current "left-to-right" approach

Feels a little funny having to repeat the function name but not a big deal at all, especially since workloads are really just specified once, and if all you're waiting for is a planned Rust feature to get rid of that then it's a great tradeoff!

I assume world.run(add_entity) by itself, outside of a workload, is still fine - and that the current rules of multithreading still apply (e.g. it's about which storages are mut)?

[leudz]

The chaining works the same way as left to right currently. Systems will run top to bottom and the scheduler will try to run them in parallel as much as possible based on what the systems borrow.

Yep it's a bit silly, the first function is the system that will be stored and run, the second one is there to give all information about what the system is borrowing.
I'm already doing that for some time now but without GAT I can't hide it.

Yes run, on it's own, doesn't have to do anything special.

[cart]

Workaround 1 seems like the best option to me as well. Personally if I have to choose between system declaration clarity / ergonomics and scheduling clarity / ergonomics, i'll pick systems every time.

[leudz]

Workaround 1 seems to be the big winner.
I'm finishing the migration from the old traits to the new ones. When this is done I'll just have to adapt the scheduler code a bit and the transition will be mostly done.
After that I'll make a 0.3 -> 0.4 migration chapter.
And finally I'll modify all doc and tests.

[leudz]

The migration chapter is up.
The proc macro name might change, but I think that's it.
And the view links should work when 0.4 is published.