Replace System with FnMut

[msiglreith]

The current code for defining a new task looks like this (from the README)

struct PrintSystem;

impl<'a> System<'a> for PrintSystem {
    type SystemData = PrintData<'a>;

    fn work(&mut self, bundle: PrintData<'a>) {
        println!("{:?}", &*bundle.a);
        println!("{:?}", &*bundle.b);
        
        *bundle.b = ResB; // We can mutate ResB here
                          // because it's `FetchMut`.
    }
}

Every system needs an additional struct (often unit type from my own experience) which becomes quite verbose. Alternative might be replace the Task trait with a function:

fn system_print(_: &mut (), bundle: PrintData<'a>) {
    println!("{:?}", &*bundle.a);
    println!("{:?}", &*bundle.b);
    *bundle.b = ResB;
}

The dispatch builder would then take a F: FnMut(&mut T, U), U: SystemData (or something similar).

let mut dispatcher = DispatcherBuilder::new()
    .add((), system_print, "print", &[]) // Adds a system "print" without dependencies
    .finish();

I'm not 100% sure if this will be possible, tests some time ago looked positive..

[torkleyy]

@msiglreith I highly support making this more convenient, but this is actually not that easy at the moment because the FnMut has to provide an implementation for every possible lifetime 'a, but you cannot express that without associated types (and FnMut's parameters are type parameters).

[torkleyy]

This issue could be solved with Associated Type Constructors (it would be a benefit for the whole crate). @msiglreith If you have any ideas, please don't hesitate to comment!

[msiglreith]

@torkleyy I'll give it a try later on!

[msiglreith]

Hm, indeed quite challenging!
I got some first results, but need to cleanup and I hope to reduce the complexity a bit ..

[torkleyy]

It seems we cannot solve this at the moment. Let's come back to it when we have HKTs. I plan to publish a new version in the next days (fyi, in case anybody wants to make changes).

[ebkalderon]

I don't think is a good idea. Many systems have some form of persistent state they need to maintain, e.g. a rendering system might contain the renderer itself, a window handle, and the scene representation, while an audio system would contain the audio sink and context. Where would these be stored if systems were only functions? Would they be stored directly within the engine? If so, that seems to run counter to the idea of systems in the ECS sense (each system contains all the logic necessary to handle a particular task and should be as self-contained as possible).

I get that the code examples look pretty verbose with all these empty structs and boilerplate, but if you're implementing any non-trivial system, functions simply cannot suffice.

[WaDelma]

I think this change would be good if and when implementing FnMut becomes stable. Then you could just implement it to your custom structs same way as System.

Can we blanket impl System for FnMut?

[torkleyy]

Can we blanket impl System for FnMut?

No, we cannot; the parameters of FnMut are a type param, which you cannot constrain with an associated type.

[msiglreith]

@ebkalderon The shown example might be not optimal but the but approaches are equally powerful.
F: FnMut(&mut T, U), U: SystemData, the T here would be your persistent state/system.

A bit independent from the topic:

but if you're implementing any non-trivial system, functions simply cannot suffice.

I kinda disagree here as storing data in system vs data in resources mainly differs in the fact that the first one is private and the other one can be accessed by multiple systems. I would argue that shared access is more common.

[kvark]

Oh wow, much controversial, so arguments! 🔫 ✌️

@msiglreith
I really appreciate the way of treating a system just as a function. It echoes with the concept of systems being immutable functions that generate events, so perhaps your proposal is a nice step towards supporting this model (along with shared mutability that we have).
Besides, the new syntax is more concise, looks better 👍

@torkleyy

I highly support making this more convenient, but this is actually not that easy at the moment because the FnMut has to provide an implementation for every possible lifetime 'a, but you cannot express that without associated types (and FnMut's parameters are type parameters).

Could you elaborate further? Let's see if the technical limitations are truly blocking us here.

@ebkalderon
I can sympathise with your argument here. It appears that moving to FnMut doesn't make old approaches impossible: you can either pass the initial state during the dispatcher construction (to then work with it as a parameter to your function), or use a resource (almost free, perf-wise, if it's only accessed by one system).

[torkleyy]

@kvark Sure:

Let's first look at the source code for FnMut:

pub trait FnMut<Args>: FnOnce<Args> {
    fn call_mut(&mut self, args: Args) -> Self::Output;
}

As you can see, Args is a type parameter. Now, the blanket implementation would roughly look like this (pseudo-Rust):

impl<T, U> System for T where T: FnMut(U) {
    type SystemData = U;

    fn run(&mut self, u: U) {
        self(u);
    }
}

right? Now, let's revise the two core difference between an associated type (SystemData in this case) and a type parameter:

struct Foo;

impl WithParam<i32> for Foo {}
impl WithParam<String> for Foo {} // Why not?

impl WithAssoc for Foo { type A = i32; }
impl WithAssoc for Foo { type A = String; } // Nope

As you can see, you can only have a single associated type for a given type, while type parameters don't have such a restriction.

Going from that, a type T implementing FnMut could implement this trait for multiple parameters; thus, we would end up with multiple System implementations for the same type which is, however, not possible.

I hope you could follow what I'm trying to explain here. If you want to, you can also see the error in this Playground example I created.

[ebkalderon]

@kvark Fair enough, but could something like a GL context be safely stored as a resource? You can specify systems to be thread-local to the Dispatcher, which means you can safely store thread-unsafe structures in there, but could you do the same for resources?

[kvark]

@torkleyy

Going from that, a type T implementing FnMut could implement this trait for multiple parameters; thus, we would end up with multiple System implementations for the same type which is, however, not possible.

Hmm, don't we provide the argument when scheduling a system? In this case, only the Fn implementation taking this argument will be selected, and this is not ambiguous.

@ebkalderon that appears to be a very strong concern, and I don't see how a GL context can be a resource. Maybe we can have &mut GlContext in parameters of a system that is fixed to the dispatcher thread?.. @torkleyy would probably tell better.

[Binero]

I think prohibiting Systems from storing state is, well, too prohibitive, especially given there is no way to specify thread-local resources. (e.g. non-Sync ones)

[kvark]

but could something like a GL context be safely stored as a resource?

See #45 for the solution

[torkleyy]

I think we can close this. The original issue seems to be resolved and the current discussion is off topic.