Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order independence of App construction #1255

Open
alice-i-cecile opened this issue Jan 17, 2021 · 22 comments
Open

Order independence of App construction #1255

alice-i-cecile opened this issue Jan 17, 2021 · 22 comments
Labels
A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 17, 2021

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
Copy link
Contributor

Ratysz commented Jan 17, 2021

You probably meant #1144.

@tigregalis
Copy link
Contributor

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
Copy link
Contributor

Ratysz commented Jan 17, 2021

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
Copy link
Contributor

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
Copy link
Contributor

Ratysz commented Jan 18, 2021

[...] 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
Copy link
Contributor

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
Copy link
Member Author

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.

@Moxinilian Moxinilian added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible labels Jan 25, 2021
@alice-i-cecile
Copy link
Member Author

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
Copy link
Contributor

tigregalis commented Feb 2, 2021

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:

  • There is the AppBuilder order independence issue, of course.
  • This concept of pre-start configuration really should belong to the plugin, rather than the app. How do we expose an API to "configure" a plugin? Next, how do you expose an API to configure plugin groups? It looks like there is an attempt at this here: Allow reconfiguring a Plugin inside a PluginGroup #1382
  • WindowDescriptor (if that's the resource used) ideally should be an "active" resource, in the same way as ClearColor. This is nice, because it is declarative. A user simply updates a resource, and things just work. This could be as simple as a system added to WindowPlugin that monitors changes to Res<WindowDescriptor>, and automatically applies the appropriate window.set_* methods.
  • WindowDescriptor only applies to a single window. Having said that, as does ClearColor. How do we expose an API that covers multiple windows? One idea was to make the normal case more explicit (PrimaryWindowDescriptor). One idea was to have Windows as entities, similar to how Cameras are,
  • There is a rather more general question about how Bevy interacts with systems external to it.

@alice-i-cecile
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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

@alice-i-cecile
Copy link
Member Author

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
Copy link
Contributor

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

@alice-i-cecile
Copy link
Member Author

Most relevant links: #2160, bevyengine/rfcs#33

@colepoirier
Copy link
Contributor

Perfect, thank you!

@alice-i-cecile
Copy link
Member Author

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.

@alice-i-cecile alice-i-cecile modified the milestones: 0.11, 0.12 Jun 19, 2023
@mockersf mockersf modified the milestones: 0.12, 0.13 Oct 20, 2023
@alice-i-cecile alice-i-cecile modified the milestones: 0.13, 0.14 Jan 24, 2024
@JMS55 JMS55 removed this from the 0.14 milestone Apr 25, 2024
@cBournhonesque
Copy link
Contributor

What about building a 'PluginGraph' (similar to schedule graph) with before/after constraints and then calling plugin::build in topological order of the graph?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

No branches or pull requests