Idea - retrying setup

[idanarye]

I was looking at the Amethyst UI example and the way they update their FPS display text thingie seems horrid - they check every frame if the field that remembers the entity is None, if so they look for the relevant entity in the world, and if they find it they store in the game data struct. Then, they check again (because it's possible that they couldn't find it yet) and only then they can use it to fetch the relevant UiText component.

I tried doing it in a system, finding the entity in the setup:

struct UpdateText {
    ui_entity: Option<Entity>,
}

impl<'s> System<'s> for UpdateText {
    type SystemData = (
        WriteStorage<'s, UiText>,
        // ...
    );

    fn setup(&mut self, res: &mut Resources) {
        let ui_finder = UiFinder::fetch(res);
        self.ui_entity = ui_finder.find("my_text_display");
    }

    fn run(&mut self, (mut ui_texts, /*...*/): Self::SystemData) {
        if let Some(ui_text) = self.ui_entity.and_then(|entity| ui_texts.get_mut(entity)) {
            // update the text
        }
    }
}

But that didn't work - it turns out the entity did not exist yet (!!!) when setup was invoked. So I had to move it to run:

struct UpdateText {
    ui_entity: Option<Entity>,
}

impl<'s> System<'s> for UpdateText {
    type SystemData = (
        UiFinder<'s>,
        WriteStorage<'s, UiText>,
        // ...
    );

    fn run(&mut self, (ui_finder, mut ui_texts, /*...*/): Self::SystemData) {
        if self.ui_entity.is_none() {
            self.ui_entity = ui_finder.find("my_text_display");
        }
        if let Some(ui_text) = self.ui_entity.and_then(|entity| ui_texts.get_mut(entity)) {
            // update the text
        }
    }
}

And now I have another dependency in SystemData... this may not be so bad, but these things stack. Also, I remain with this cumbersome and weird code that needs to check ui_entity every single time.

I also did some logging, and it turns out that on the first iteration run couldn't find the UI entity! Only from the second iteration could it start working.

This had me thinking - what if we could delay the registration of a System until after all it's resources are available? An API like this:

fn create_update_text(res: &mut Resources) -> Result<UpdateText, SystemCreationError<()>> {
    let ui_finder = UiFinder::fetch(res);
    Ok(UpdateText {
        ui_entity: ui_finder
            .find("my_text_display")
            .ok_or(SystemCreationError::RetryLater)?,
    })
}

struct UpdateText {
    ui_entity: Entity,
}

impl<'s> System<'s> for UpdateText {
    type SystemData = (
        WriteStorage<'s, UiText>,
        // ...
    );

    fn run(&mut self, (mut ui_texts, /*...*/): Self::SystemData) {
        if let Some(ui_text) = ui_texts.get_mut(self.ui_entity) {
            // update the text
        }
    }
}

Where `SystemCreationError` is:
```rust
pub enum SystemCreationError<E> {
    RetryLater,
    Failure(E),
}

Instead of registring a UpdateText object, we register the create_update_text function (or a closure that calls it). It'll go into a list, and before we run the systems on each frame we check that list and run every Fn in it. If it returns Ok(system), we add system to the dispatcher and remove the creator from the list. If it returns Err(SystemCreationError::RetryLater) nothing happens - and the same creator will be attempted again on the next iteration. Err(SystemCreationError::Failure(error)) will fail everything with Err(error). Or we could just give up on that part, let the creator return an Option, and actual setup failures will panic! instead.

This delayed setup phase can be a bit expensive, since it can't be parallelized, but it should only happen at the first few frames - after that the queue will be empty and the other systems will run a little bit faster because they won't have to check their Options - which will also make their code cleaner.

[torkleyy]

Hm, I think you're missing something here. The setup method is only for the purpose of adding missing resources to the world and eventually subscribing to them, but it's not some kind of "start" phase. And in the example you gave there's an asset that needs to be loaded first; that could happen many frames after the game started (or maybe it's done at a later point in the game, we don't know); my point being that this doesn't belong into System::setup. Maybe the system should be working different (although I don't see anything wrong with what is essentially a null pointer check).

Okay, just looked at the link you posted, the code doesn't look very nice, but I still don't think this is the way to solve it.

cc @Moxinilian @Xaeroxe @Jojolepro

[Moxinilian]

I’ve always wanted a way to get rid of the optional wrappers and all that stuff from Systems so I’m glad this is something we can talk about. I think a good approach inspired by what you suggest would be to have system have a `fn generate(res: Resource) -> Self` method that could be used by the Dispatcher to build the system after all the resources have been properly set up. That would be much cleaner in my opinion and would save some useless option checking (which is more of a UX clutter than an actual performance issue). Maybe the generate method could also take a one-time struct of metadata to save flexibility. Or the generator could also be a closure that would have moved ownership over the metadata in its context. 

[idanarye]

When I'm talking about performance, I'm not talking about the extra None checks - these are surely insignificant. My main concern is the extra resources in SystemData that are only used once (or twice, in my example) but still need to be locked on every frame. True, this is a read lock so the damage is limited, but it can still block some parallelization opportunities if something else needs a write lock on the same resource...

[AnneKitsune]

I'm already working on a thing that automatically updates ui texts based on the id. Both the UiText and the UiTextUpdated will be on the same component, so I will just have a join over both of those. In this specific case the current api design will look like this:

Components

• UiText

• AutoFpsText(component): UiAutoText(trait)

• AutoFpsSystem (FPSCounter resource -> updates AutoFpsText

• AutoTextSystem<T: UiAutoText> (UiAutoText -> updated UiText)

AutoFpsSystem would always run before AutoTextSystem
and both would be in the fps counter bundle.

[Moxinilian]

Of course Options are free. I just think they are cumbersome, so this issue might provide a solution to that in the process. The suggestion I made also solves the issue you raise, as now you could do everything in the generating method where all things would be ready.

[AnneKitsune]

@Moxinilian its not about resources, in this case its about doing an entity search. Its rare to have a system that needs to wait for a resource to be created. Also, in most of the cases where you do need to wait for that resource, you would be better off using a second dispatcher instead of managing the option in the system.

[Moxinilian]

Resource in the inclusive meaning, that is anything that requires setup by the World.

[torkleyy]

@Moxinilian I think we're discussing two different things here. Making setup create the system instance is one (posible) way to go, the example that's discussed here though does not rely only on a resource, but an IO operation and other stuff to run first. Please don't mix these things here.

[torkleyy]

Let's split this issue:

1. one issue for this specific example (in the Amethyst repo)
2. another one for possible improvements for System::setup

[AnneKitsune]

As I said, the amethyst one is going to be fixed soon. There are multiple ui related issues that cover this.

[torkleyy]

Okay, great. As for setup suggestions please open dedicated issues and I'll reject them one by one xD

Nah seriously I want the discussions to be on topic, so Mox if you want something to change, please create another issue here or in the shred repo. @idanarye As for the delayed setup phase I think already answered that that is beyond the scope of setup, besides not being very helpful for this case. Should something be unclear or for any other suggestions, please open new, separate issues.

[torkleyy]

Also related: #437