Discourage custom `System::setup`

[torkleyy]

trafficstars

This issue is split out of #437.

Using System::setup for initialization, while often being convenient, brings three major drawbacks with it:

• implicit resource creation -> might be using the wrong resource, especially when generics are involved (one system uses Foo<i32>, the other one Foo<i64> and both will fail to see the changes of the other one)
• initializing system fields with it means:
  • fields need to be Options
  • resource initialization is decentralized (whether one considers that as a drawback or not)
• resources / system fields can hardly depend on each other when initialized using setup, possibly leading to (undocumented) ordering requirements of the setup calls

[Xaeroxe]

One alternative I’d considered previously is making System::setup a constructor for the system, and when registering a system you pass in the type instead of the constructed system.

[Xaeroxe]

Though I will say, decentralized implicit resource creation was one of the reasons I didn't care for this approach when we implemented it. I'm a big fan of the KISS principle and I feel like we kinda missed that with this API.

[torkleyy]

There's a couple of things I'd like to say.

Firstly, I'm in favour of the general idea: use a proper constructor for systems, so things are not initialized lazily.

Now, the part I don't like is encoding this in some generic Specs interface. I'd much rather make this a convention than a trait method, because the latter brings its own issues (more associated types, tuple unwrapping, etc..) with it.

Secondly, the initializing of system fields is an approach that works well in the most common case. This is also the major reason why I'm not suggesting we move away from it completely. However, I think it this point it might be good to show the ideal I've worked out for nitric:

• systems have no fields & are stateless
• for caching, systems have a MySystemCache resource they're passed

So all systems basically just are:

pub fn my_system(cache: &mut MySystemCache, entities: &EntitiesRes, ...);

I see that this is not the simplest approach, and the freedom of having one cache per world / data set / etc is most likely not necessary / not something we should enforce.

So considering all those points, I'd be in favour of making system fields the standard convention, discouraging initialization of them in setup and doing that in new as it's common for Rust types.

[AnneKitsune]

The issue with initializing all field in new is that if you need access to a resource to get that field's value, you are forced to create the resource manually(instead of having Default creating it for you), and then you need to add that resource into Resources.

I do like the idea of having the fields stored as a resource. Not only does this encourages systems to be stateless, this also encourages systems to be more configurable at runtime.

For example, if you have a shrev event channel used by two systems that are checking for component updates. One of the system modifies the component, but you don't want that to be "seen" by the other system. What you can do is: from the system writing to the component, grt the resource of the other system and clear the ReaderId.

This is just one example among others, and it probably isn't a good one because it is a consequence of how FlaggedStorage and EventChannel emiy to all ReaderId the same way.

[torkleyy]

Thanks for the input!

The issue with initializing all field in new is that if you need access to a resource to get that field's value, you are forced to create the resource manually(instead of having Default creating it for you), and then you need to add that resource into Resources.

I think that can be solved by

pub fn new(world: &mut World) -> Self {
    MySystem {
        foo_reader: world.entry::<Foo>().or_default().register_reader(),
    }
}

I don't think exposing the fields is a good idea (in general) or a reason to store system state in the world.

[distransient]

I think having initialization which takes either &mut World/&mut Resources and instead of taking a &mut self, just returns a Self would solve the problems here and handing &mut World would also help usability although I'm not sure what the downsides to handing what's essentially the Extended Resources type (aka World) instead of the Resources itself, I like the new(&mut World) -> Self approach!

[AnneKitsune]

I do like torkleyy's solution too. I think if there was a method on World (or Resources) get_or_default that would make the syntax easier to use and System::setup could probably be removed completely.

[torkleyy]

I'm still a bit unsure how to go about the deprecation; I don't think it's possible to generate warnings for implementing a method, so if we'd remove it entirely that'd be a major breaking change.

[distransient]

Could be part of the set of 1.0 beta api changes.

[torkleyy]

Yeah, true. I'm just trying to avoid too much breakage.

[AnneKitsune]

People expect breakage before something is stabilized. That's why we use alpha versions. :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Feel free to comment if this is still relevant.

[torkleyy]

Still relevant

[marotili]

I'm putting some initial thoughts here. Maybe someone can help me to flesh them out or challenge them.

As someone new to specs (and amethyst), storing the event reader in the system caused some problems. The lifespan of a system is different from that of the world, and at the moment there seems no way to clean up resources allocated by a system. Because of that, logic that I'd consider part of a system would go into amethyst's game state.

I'd consider a system that needs a state to run to be an entity. For example, FlyMovementSystem would benefit from that approach by making speed and the axis configuration changeable at runtime.

So systems would store an Entity and the data/cache they require to run is stored as components on this entity. If this entity is deleted (the owner also owns the dispatcher), the data of that system is cleaned up.

Anyway, removing or deprecating setup and moving towards either pub fn new(world: &mut World) -> Self or pub fn new(entity: Entity) would be the first step. I don't think specs needs to decide on one approach.

[ldesgoui]

I am also exploring the idea of keeping ReaderIds in Resources, and generally to never keep state in Systems: https://github.com/eia3de/eia3de/blob/e1df7f360abbdc410928f5b4065bc6835cd7ca36/eia3de_client/src/windowing.rs I just noticed that this could become a problem if the System that uses the ReaderIds is not part of the current Dispatcher, in which case I could delete the Resource or make sure the ReaderId is kept up to date with a System that just ignores events

[marotili]

The problem already exists and is worse if the system stores the reader itself because in that case, the system does not know it was paused. I think whoever controls the dispatcher should also be controlling the event readers.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Feel free to comment if this is still relevant.

[AnneKitsune]

not stale

[AnneKitsune]

not stale

[lcnr]

I do not have a lot of experience with using specs, so some of my points may not be completely accurate.

I am in favor of completely removing the System::setup and requiring the added systems to already be fully initialized as this allows for the most freedom during System creation and I am unable to think of a big disadvantage.

let mut world = World::new();
world.add_resource(Bar); // add resources needed by A to world

let b_data = "Foo";
let dispatcher = DispatcherBuilder::new()
    .with(A::new(&mut World), "a", &[]) // A only need resources during initialization
    .with(B::new(b_data), "b", &["a"]) // B does not need access to World
    .with(C, "c", &["a"]) // c does not require any initialization
    .with(D::new(&mut World, 42), "d", &["b", "c"]) // d requires both resources and other data
    .build();

---

Changing setup to a constructor probably requires a type parameter (type SetupData) for data which can't be added to the World and changing the method to fn setup(res: &mut Resources, data: SetupData) -> Self. This can end up being quite complicated in some cases, for example if two systems require the same mutable reference for initialization when using the current DispatcherBuilder. It is also causes the System trait to be more complicated.

I am also not in favor of changing systems to be stateless and storing their state as a resource, as this is problematic if 2 systems use a different object of the same type. While this could be solved by wrapping the data of each system in a custom struct, I do think that the added complexity as well as the possibly unexpected behavior if someone does not use a custom type for each struct make this approach not worthwhile.

[distransient]

Changing setup to a constructor probably requires a type parameter (type SetupData) for data which can't be added to the World and changing the method to

People would just write their own constructors, it would not be part of a trait.

I am also not in favor of changing systems to be stateless and storing their state as a resource

This does not make systems stateless.

...this is problematic if 2 systems use a different object of the same type. While this could be solved by wrapping the data of each system in a custom struct

Allowing multiple consumers to safely access the same resources across threads is the entire point of shred and specs :) however, you probably don't need to be doing initialization across threads... Initialization works just fine in serial

[lcnr]

People would just write their own constructors, it would not be part of a trait.

That's what I want to happen. What I meant is that it is not completely unreasonable to have trait for System initialization, as it would allow us to not manually handle the World while building a dispatcher.

With setup one could write

let dispatcher: Dispatcher = DispatcherBuilder::new()
    .with::<SystemA>(additional_data_for_a, "a", &[])
    .with::<SystemB>(additional_data_for_b, "b", &["a"])
    .build();

while without a trait for System initialization the following is required:

let mut world = World::new();

let dispatcher: Dispatcher = DispatcherBuilder::new()
    .with(SystemA::new(&mut world, additional_data_for_a), "a", &[])
    .with(SystemB::new(additional_data_for_b), "b", &["a"]) // in case B does not need any resources.
    .build();

This does not make systems stateless.

idk, I meant without any inner state/ having the property size_of::<Sys>() == 0. Similar/identical to the approach by @torkleyy

So all systems basically just are:

pub fn my_system(cache: &mut MySystemCache, entities: &EntitiesRes, ...);

---

Allowing multiple consumers to safely access the same resources across threads is the entire point of shred and specs :) however, you probably don't need to be doing initialization across threads... Initialization works just fine in serial

I am talking about systems which are currently implemented like this:

struct DashSystem {
    reader: ReaderId<InputEvent<GameAction>>>,
}

struct JumpSystem {
    reader: ReaderId<InputEvent<GameAction>>>,
}

which could naively be changed to

struct DashSystem;

impl<'s> System<'s> for DashSystem {
    type SystemData = (
        Write<'s,  ReaderId<InputEvent<GameAction>>>,
        // ...
    );
}

struct JumpSystem;

impl<'s> System<'s> for JumpSystem {
    type SystemData = (
        Write<'s,  ReaderId<InputEvent<GameAction>>>,
        // ...
    );
}

This would lead to one of the systems to not work correctly, as both actually share the ReaderId and the system which is initialized last overwrites the previous reader.

[distransient]

If you want dispatcher building with resource initialization, I'd recommend a solution that receives an FnMut for a reference to the World tied to that dispatcher. Thus you could define bundles in terms of taking &mut World for resource initialization and applying the initialized resource to your dispatch builder.

naively

Yes, you're right. That's naive, and wrong. You would instead write a newtype for each reader needed if you truly wanted your systems stateless. However, that doesn't have anything to do as this is done for Specs, since it seems to me Torkley was mentioning stateless systems only directly in relation to the neat ideas he's worked out while working on Nitric, and how they relate to driving him to thinking about cleaning up this part of the API, and there's been no decision I've personally seen to make that API change for Specs heading towards 1.0

[lcnr]

If you want dispatcher building with resource initialization

I don't. I mostly thought about ways to maintain the status quo while fixing the current problems with setup and did not consider using closures for initialization.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Feel free to comment if this is still relevant.