bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
33.55k stars 3.26k forks source link

Order independence of App construction #1255

Open alice-i-cecile opened 3 years ago

alice-i-cecile commented 3 years ago

What problem does this solve or what need does it fill?

As with our previous discussion on implicit ordering of systems (see #1144), insertion order mattering is frustrating to debug at scale and can make refactoring more painful.

In addition, this avoids a common beginner pain point, where they'll insert a resource after it's used and then get an error about the resource not existing.

What solution would you like?

Collect all of the App methods, then process them more holistically (such that order doesn't matter) when AppBuilder.run() is called.

With system insertion order no longer mattering once #1144 is stabilized, there are only a few places that rely on ordering (as best as I can tell).

  1. Resource and state insertion. These will typically be able to be added in parallel before any other steps. Special FromResources resources are the only wrinkle that comes to mind. We can also catch duplicate insertion of the same type, which is a nice correctness-checking feature.
  2. Adding stages. Stage ordering is typically defined relative to other stages. We'll need a little tree resolution mechanism to make sure that dependencies are added first, but solving this will be a major win for refactorability (since you won't need to think about this manually).
  3. Working with State. These will want to be handled after resources and stages are added.
  4. set_runner. We should probably just panic if two of these are set at once, rather than overwriting.

What alternative(s) have you considered?

Through clean code architecture, you can often define resources in the corresponding plugin, before any other systems are added. This reduces the pain substantially, but relies on consistent code hygiene, and won't work well for resources that are used by multiple plugins.

Additional context

Down the road, this approach opens us up to more sophisticated optimizations and analyses, as we'll have access to all of the parts of the App before they're processed, rather than only getting a backwards-looking view at best.

Ratysz commented 3 years ago

You probably meant #1144.

tigregalis commented 3 years ago

I don't know if this is the right place for this, but I've been reading the discussion (and trying to follow), but is it accurate to say that mostly what a Stage does is just provide a point at which the queued commands get executed? If so, would it be feasible to have a stage-less API that allows someone to simply insert a "run_commands" system and depend on it?

Ratysz commented 3 years ago

That's not all a stage does, and is not its most important function: a stage encapsulates a group of systhems and guarantees that they will run before or after another group of systems (encapsulated by a different stage). Everything else just follows from that definition - for example, applying commands issued during the stage at the end of the stage becomes an obvious idea.

If you were to try and encode this encapsulation via dependencies, you would have to make the "apply commands" system depend on all systems that come before it and be a dependency for all systems that come after it. This is obviously much less ergonomic, and a lot more error-prone: a system that applies commands can modify archetypes, among other things, and so, with the upcoming non-deterministic executor, can easily produce heisebugs should the user fail to create this synchronisation point.

This is not the first time I see "let's get rid of stages" and I still don't fully understand where it's coming from. As I've said before, the schedules and stages hierarchy is strictly organisational; the fact that a stage can be managing several systems doesn't mean that all stages have to be big and slow. A larger, performance-minded project's schedule will most likely consist of a single large heavily-parallelized stage and a handful of small stages performing synchronisation.

tigregalis commented 3 years ago

Is it correct that if you want to spawn entities, despawn entities, or modify components, you can only do this at the end of a stage (or rather, you queue them up as commands, and it takes effect then)?

You might wish to do these things in the "middle" of a stage. You would have to insert a stage, which might come at the cost of how parallel your systems can run.

In some cases you can't, though. A plugin you're using might not have split their systems into stages sufficiently to meet your needs, as you might want to insert a system between some of the plugin's systems within a stage. e.g. plugin_system_a -> my_system -> plugin_system_b

The systems you want to run may be mutually exclusive. e.g. you have a set of AI systems, a set of physics systems, and a set of UI layout/update systems; it's likely that each of these can and should run in parallel but (sequential) stages prevent that. Your UI systems might run every frame. On the other hand, your physics system needs to run on a fixed timestep (let's say every 10ms), so it may run several times each frame; your physics system might (will) need to spawn objects between steps in the simulation. Your AI systems may be long-running across multiple frames.

It could just be because this is the current convention of using stages to define system execution order which I've seen and are more familiar with, and perhaps in future the default set of stages don't get changed much and we would maybe use SystemSets within the Update stage, but what does this future API properly look like?

Ratysz commented 3 years ago

[...] but what does this future API properly look like?

We're still working on that; I'm in the middle of another API pass (not pushed it yet), so keep an eye on #1144. Most of your questions and/or wishes about this will also be more at home there.


More on topic; I think the solution for exclusive system ordering in #1144 can be reused for inserting stages, too. It's simple: instead of inserting and sorting stages on the go, just store them, and resolve labels and all the "befores" and "afters" on build, or first run. (Re-reading it now, that's pretty much what you suggest too?)

Resources are handled by initialization, I think, which is done on a per-system basis; the burden of that will most likely be shifted completely onto Stage implementors. So resources used by say the startup stage are all initialized right before it runs its systems for the first time.

Regarding states... I'm not certain what the plan is, but system sets allow us to reimagine most of StateStage's functionality. States could be just a resource that run criteria read.

jakobhellermann commented 3 years ago

Another thing that's probably relevant here is how to deal with repeated addition of Plugins.

To give an example: The bevy-inspector-egui crate depends on bevy-egui, and because I'd like it to be usable without adding it yourself I do app.add_plugin(EguiPlugin) in the InspectablePlugin. This works fine, until another crate tries to add the EguiPlugin.

If there was a way to only add a plugin if it's not already added that would solve my problem I think.

alice-i-cecile commented 3 years ago

When / if we implement this, we should also be mindful of how it interacts with State, and especially the obvious pattern of putting game startup logic within a reusable StateStage, rather than sticking it all in startup systems.

Currently, you should be able to register a StateStage with logic that triggers .on_state_enter, then set the State to an initial value after it in the AppBuilder, allowing you to immediately trigger that logic.

If we register all resources before all systems, this won't be possible. I don't think it's a serious issue, but I would like to see an example of how to do this properly made as part of this change.

alice-i-cecile commented 3 years ago

Another issue along these lines: the order in which StateStages that rely on the same state are inserted seems to control which one gets to handle the transitions.

tigregalis commented 3 years ago

A related issue is the following:

// this doesn't work
    App::build()
        .add_plugins(DefaultPlugins)
        .add_resource(WindowDescriptor {
            title: "My game".into(),
            mode: WindowMode::BorderlessFullscreen,
            ..Default::default()
        });

// this works
    App::build()
        .add_resource(WindowDescriptor {
            title: "My game".into(),
            mode: WindowMode::BorderlessFullscreen,
            ..Default::default()
        })
        .add_plugins(DefaultPlugins);

In this case, this is because WindowDescriptor is really a "pre-start configuration object", rather than a true resource. There was discussion on discord about this: https://discord.com/channels/691052431525675048/742569353878437978/805752533959835668

There are a few separate issues with their own questions, but the gist is that this particular case can be "solved" without necessarily addressing the AppBuilder order independence issue, or perhaps that the AppBuilder order independence issue is a collection of several separate issues that each need addressing:

alice-i-cecile commented 3 years ago

If insert ScheduleRunnerSettings after adding MinimalPlugins, it's ignored Perhaps it can be solved by moving lines 50-53 inside the closure: https://github.com/bevyengine/bevy/blob/main/crates/bevy_app/src/schedule_runner.rs

\@broom, on Discord

alice-i-cecile commented 2 years ago

Another good example of the problem: inserting WgpuOptions after DefaultPlugins causes validation issues when attempting to use wireframe rendering. The workaround is to insert WgpuOptions first, then add DefaultPlugins.

wgpu error: Validation Error

Caused by: In Device::create_render_pipeline missing required device features NON_FILL_POLYGON_MODE

colepoirier commented 2 years ago

Oof that is exceedingly suboptimal. Are there any ideas yet for how we might solve this @alice-i-cecile?

alice-i-cecile commented 2 years ago

Yes; this is half the reason behind why I want to move to a structured approach to plugin definition.

That should get us most of the way there.

colepoirier commented 2 years ago

@alice-i-cecile that's great! Can you please link to the 'structured approach to plugin definition' issue or discussion?

alice-i-cecile commented 2 years ago

Most relevant links: https://github.com/bevyengine/bevy/issues/2160, https://github.com/bevyengine/rfcs/pull/33

colepoirier commented 2 years ago

Perfect, thank you!

Weibye commented 2 years ago

We have runtime system and startup_system. Would formalizing a concept of build_system that runs during app / plugin construction solve part of this problem? I have no clue how possible or viable that is.

Re-reading: That's just an implementation of what alice is proposing:

Collect all of the App methods, then process them more holistically (such that order doesn't matter) when AppBuilder.run() is called.

alice-i-cecile commented 2 years ago

It's an interesting idea for how to do so though. IMO in order to get that working well, we'd want to allow systems to access &mut App entirely. Which would be an exciting design, but might play nicely with multiple worlds.

I like this quite a bit, because it reuses existing machinery.

Weibye commented 2 years ago

Maybe you could even formalize a concept of "build-time-resource" that stops existing at the end of the build-stage so that you get rid of dead and dangling resources (relevant discord comment)

cart commented 2 years ago

2988

alice-i-cecile commented 1 year ago

I finally finished first (compile) part of migrating my game from 0.9 to .10 (which took me whole day today) and now getting runtime errors. The only first one strike me so that I don't have any idea how to fix it: thread 'main' panicked at 'Schedule OnEnter(Init) does not exist.', /home/set/.cargo/git/checkouts/bevy-f7ffde730c324c74/43ea6f2/crates/bevy_app/src/app.rs:393:17

I have few places where it might probably be causing:

.add_system(init_configs.in_schedule(OnEnter(GameStates::Init)));

and

.add_system(bullet_init.in_schedule(OnEnter(GameStates::Init)))

which both looks similar. Is there something wrong with how I use in_schedule and OnEnter here? What might be the cause of this error?

From a Discord #help thread.

This problem also applies to schedule initialization, and thus states.