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

Plugins should be more customizable and work with states #2160

Open
alice-i-cecile opened this issue May 14, 2021 · 13 comments
Open

Plugins should be more customizable and work with states #2160

alice-i-cecile opened this issue May 14, 2021 · 13 comments
Labels
A-Core Common functionality for all bevy apps C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR

Comments

@alice-i-cecile
Copy link
Member

Bevy version

0.5

Problem description

  1. Many plugins add interesting and valuable gameplay systems.
  2. States are very commonly used to control whether a game's core systems operate, such as creating a Paused or Loading state.
  3. Systems added in plugins are inserted into the stage and state that the plugin author set, with no way to change this.
  4. System labels cannot be added to systems added via plugins.

The State problem here is side effect of the fact that we cannot add run criteria to systems added in plugins.

Potential solutions

Solution 1: Everything is pub

Rather than trying to solve this directly, the Bevy community forces plugin authors to make all of their systems (and supporting types) pub.

Users then reconstitute the plugins from scratch by adding the systems etc. on their own terms in their own code to get the interoperability they need.

This is bad because it creates cluttered, confusing APIs and exposes details that should be internal carelessly. This then causes every non-trivial plugin crate version to have breaking changes.

It is also very heavy on boilerplate.

Solution 2: Just fork it

As Solution 1, but users fork all plugins (and the engine) and forcibly make the things they need pub / make the changes they need.

Like Solution 1, but shifts the maintenance burden and headaches to the end users.

Very bad for user experience and ecosystem fragmentation.

Solution 3: Post-hoc configurability

Allow users to set the stages / states / run criteria of all systems in a plugin at once.

This doesn't involve any changes to our logic, and patches the most pressing issues.

However, it starts to break down pretty badly if you need to have plugins with systems that cannot coexist in the same stage or state: startup systems and those that must be separated by commands are particularly common and painful here.

Solution 4: Plugins contain only one system set

As solution 3, but each plugin can only have a single system set.

Solves the problems above, but makes their common use more onerous.

Solution 5: App as a structured object

Completely rework how App works by replacing the "mutate the entire AppBuilder" approach with one where the AppBuilder (and App) have carefully structured fields that you can mutate regardless of the visibility of the field.

To solve this particular issue:

  1. All systems belong to a system set, and the app can have any number of system sets.
  2. Each system set must have a label (although it might be automatically generated).
  3. All system sets can be modified using their label after their insertion into the AppBuilder.
  4. Each plugin contains their own app in this way, and their labels are exposed to the larger app that they make up.
  5. The App is not constructed until .run is called, at which point it is constituted in a deterministic order that is independent of insertion order.

Solves #1255, which would be great for clarity and ease of refactoring. This would be a serious endeavor though, and might limit users ability to throw new ad-hoc behavior onto the AppBuilder (not that I've ever seen anyone try?).

This is a sensible compromise on the level of granularity and visibility that plugins expose, grouping their functionality into configurable units.

Conclusions

Solution 1 is the de-facto standard right now for the Bevy engine, as virtually all of our systems are pub. Solution 2 is what will happen if we do nothing (or advanced users instead develop severe NIH syndrome). Both of these are pretty terrible.

Thus, we need a thoughtful approach to configuration. Solutions 3 and 4 are somewhat better than what we have now as they allow more than 0 configurability, but are almost certain to fall short in real use.

I am in favor of Solution 5, but this needs some serious thought. If you want to make an RFC, go for it! Otherwise, I'll probably get around to it after 0.6 launches due to other priorities.

Of course, all of these problems get dramatically worse once we have large games and an editor, and want to examine and manipulate systems visually.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention A-Core Common functionality for all bevy apps C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged and removed C-Bug An unexpected or incorrect behavior labels May 14, 2021
@Nilirad
Copy link
Contributor

Nilirad commented May 14, 2021

A bit of context about AppBuilder and App

AppBuilder wraps an App.

The App struct contains a World, a Schedule, and a runner function.

Bevy plugins (first or third party), implementing the Plugin trait, expose the build method, that takes &mut app: AppBuilder as a parameter. Therefore, plugins are able to edit any field of the underlying App.

The problem

The property of a plugin to be added with no configurability (at least by default), can have its pros and cons.

The major concern that comes to my mind is relevance of builder method order. This comes from the fact that some methods overlap and end up overwriting previous data or other kinds of stuff. For example, you can have two plugins, A and B, that both add a SystemStage (i.e. a stage) after Bevy's Update stage. Based on order of plugin addition we get different stage order results:

With A then B:

'Update' -> 'B' -> 'A'

With B then A:

'Update' -> 'A' -> 'B'

A possible solution

Solution 5 seems the best decision when compared against the others. It will require the most work, but it will give us the most benefits in the long run.

About system reordering, it's ok to give the user the chance of moving them around, but there should be constraints. I'll explain.

Once, I had to fix an obscure bug on bevy_prototype_lyon (the API consists in you creating a stub object with a description that gets completed later) where meshes for the shapes weren't generated. Then I discovered that moving the mesh-generating system to a new stage after 'Update' solved the problem.

Therefore I think that it's ok to give the user the ability to reorder system sets within a stage, but they should not be able to move them to other stages, since stages are kind of hard boundaries. Also, stage reordering should be limited to only across plugins. If we take the previous example, you shouldn't be able to have a stage configuration like the following:

'A' -> 'Update' -> 'B'

since that would break the 'Update' -> 'A' stage configuration requirement that the A plugin author originally intended. In poor words, configuration is only allowed where plugins don't know each other (A doesn't know B and vice versa).

The solution that I see is one where a Plugin is made of smaller building blocks that the client can (not must) move around (with some well thought limitations) to their needs.

@alice-i-cecile
Copy link
Member Author

For others who may encounter this before we fix it properly: I hand-rolled a solution for allowing the end user to control which state if any a plugin's system runs.

You can examine it here: it's reasonably nice, but must be added to each plugin ahead of time and adds significant boilerplate.

@pjankiewicz
Copy link

Just want to add from the other issue (#4362) that I think you should also analyze in what circumstances people what to change the plugin behaviour. In my case I just wanted to disable the plugin under some conditions and something like:

app.add_plugin_with_criterium(PluginA, some_criterium_system)

or inheriting System Set

app.add_plugin(PluginA.with_run_criteria(some_criterium_system))

Would solve the problem.

I'm a new Bevy user and I found it a little bit confusing that plugins and System Sets have different functionalities. In my opinion when someone writes a plugin that is just a single System Set then it shouldn't be in fact a Plugin but a System Set. This makes it possible to use every functionality of a System Set in this case. So a solution in this case would be not to allow creating objects which behave in the same way but are different type. I think it makes me vote for 4th solution. Maybe creating SystemSetPlugin could be an option.

@minecrawler
Copy link

minecrawler commented Mar 31, 2022

I really like that I can add a plugin and mostly don't have to care how it is implemented inside. Just imagine a plugin which is one system set today, but suddenly needs more complex logic after an update - which means that I have to switch out the system set and make it a plugin again.

I really love the idea of focusing things around APIs. Plugin handling should have a rich, enjoyable API from Bevy's side, so that common things can be done with "standard" Bevy. Maybe adding run criteria for a plugin is one of those things.

However, I would also love to have plugins which expose configuration themselves. It is hard to cover every plugin use case from Bevy's side after all. So, plugins should expose a way to be configured. I am not talking about code, necessarily. Plugins may be forced to implement traits to get some sort of uniformity. What I would expect, though, is a best practice guide on how to create a plugin for Bevy, which covers how to expose a configuration object, how to access custom plugin APIs, etc.

Imagine someone wrote a 2D RPG plugin, which already implements a lot of common logic for a 2D game - and works like a sandbox if just slapped into Bevy with a single line of code (think RPG Maker sandbox). Game Devs could use that plugin as a base to build their own game, but would need to configure a ton of things and call APIs to interact with the sandbox, which Bevy would never be able to foresee. No game dev wants to care about how the plugin works under the hood, and even after many version updates and internal changes, it should ideally have the same API and usage as before.

To sum it up, I'd propose another solution to the above problem, which would mainly be: Add common QoL API around plugin handling and write a good guide on how to create plugins, which covers passing labels into the plugin, calling functionality of plugins inside systems, exposing components as API, which can be queried by users, etc.

@andho
Copy link

andho commented Jun 19, 2022

However, I would also love to have plugins which expose configuration themselves. It is hard to cover every plugin use case from Bevy's side after all. So, plugins should expose a way to be configured. I am not talking about code, necessarily. Plugins may be forced to implement traits to get some sort of uniformity. What I would expect, though, is a best practice guide on how to create a plugin for Bevy, which covers how to expose a configuration object, how to access custom plugin APIs, etc.

I really like this idea and bevy providing a trait like ConfigurablePlugin might be best way to allow some uniformity. At the same time if a plugin really wants something different, that's possible even now. Or just the Plugin trait could expose some extra optional methods that take some configuration primitives and applies it during build.

@alice-i-cecile
Copy link
Member Author

I'd like to split Plugin::build into two methods to encourage plugin authors not to overconfigure things:

fn build_minimal(&self, app: &mut App);

fn build_default(&self, app: &mut App);

build_minimal is required, and build_default defaults to calling build_minimal. The ideas is that which one is used is controlled based on whether you call App::add_plugin or App::add_minimal_plugin.

This encourages plugin authors (including Bevy itself!) to expose both a "works out of the box" and a "minimal set of constraints required" version of their plugin, greatly improving end-user customizability without breaking our "constraints cannot be removed once added" privacy guarantee.

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 28, 2023
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed P-High This is particularly urgent, and deserves immediate attention S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged labels Feb 28, 2023
@alice-i-cecile
Copy link
Member Author

[8:40 AM]Alice 🌹: @cart expressed skepticism that there's a clean minimal / default split, which is fair (it's been cut as a suggestion from the release notes)
[8:41 AM]Alice 🌹: One of his ideas there was a split between system config, based on whether or not it can be removed
[8:41 AM]Alice 🌹: Which is quite neat!
[8:42 AM]Alice 🌹: I like that it enforces that the "default" setup must be a strict superset of the "minimal" one
[8:43 AM]Alice 🌹: Syntax and discoverability definitely feel like the hardest parts of that idea though.
[8:46 AM]Alice 🌹: You need to be able to:

  1. Declare system config (and maybe other parts of plugins) as optional
  2. Let users and tools quickly determine which parts were optionally added, ideally without having to read the source
  3. Let users refer to specific constraints to wipe them out in a granular fashion that's stable across releases
    [8:49 AM]Alice 🌹:
    app.reversibly_configure_set(InputSystem::ReadInputs.in_base_set(CoreSet::PreUpdate), InputSystemRule::BaseSets)
    [8:50 AM]Alice 🌹: I think that means that you a) have to explicitly configure these rules separately and b) name each rule in some way

Recording my thoughts from Discord here: I think that this is probably a clearer and more powerful approach, if a bit heavier.

I want to keep an eye on user stories here, to try and more exactly figure out the needs of both plugin authors or users.

@alice-i-cecile alice-i-cecile modified the milestones: 0.11, 0.12 Jun 19, 2023
@66OJ66
Copy link
Contributor

66OJ66 commented Sep 4, 2023

For some simple plugins, the extent of the configuration needed might just be passing a State into it.
e.g. a plugin might just load some assets then insert some resources - all it needs to know is which State to do that in.

Example code:

// 3rd party plugin
struct TestPlugin(State<dyn States>)

impl Plugin for TestPlugin{
   fn build(&self, app: &mut App) {
      app
         .add_systems(OnEnter(self.0), start_asset_load)
         .add_systems(Update, check_asset_load.run_if(in_state(self.0)));
   }
}


// Consumer might then do this:

#[derive(States, PartialEq, Eq, Debug, Default, Hash, Copy, Clone)]
pub enum GameState {
   Startup,
   MainMenu,
   Loading,
   InGame,
   // etc...
}

fn main() {
   App::new()
      .add_plugins((
         DefaultPlugins,
         TestPlugin(GameState::Startup),
      ))
      .run();
}

I've been trying to get the above working, but I'm getting this error:

error[E0038]: the trait `bevy_ecs::schedule::States` cannot be made into an object
   --> src/lib.rs:22:34
    |
22  | struct TestPlugin(pub State<dyn States>);
    |                                  ^^^^^^^^^^ `bevy_ecs::schedule::States` cannot be made into an object
    |

    
39  | pub trait States: 'static + Send + Sync + Clone + PartialEq + Eq + Hash + Debug + Default {
    |                                                   ^^^^^^^^^ the trait cannot be made into an object because it uses `Self` as a type parameter

Is there a way to work around this, or is this approach incompatible with the current State design?

@66OJ66
Copy link
Contributor

66OJ66 commented Sep 4, 2023

Tried a few more permutations, this seems to work:

pub struct TestPlugin<S: States + Copy>(pub S);

impl<S: States + Copy> Plugin for TestPlugin<S> {
   fn build(&self, app: &mut App) {
      app
         .add_systems(OnEnter(self.0), start_asset_load)
         .add_systems(Update, check_asset_load.run_if(in_state(self.0)));
   }
}

Very little boilerplate needed!

@alice-i-cecile
Copy link
Member Author

Yep, this is a great approach for plugin authors to provide these days. It would be nice if there was a standard though, so then users know what to expect.

This is particularly painful for update vs fixed-update for physics and gameplay plugins.

@feelingsonice
Copy link

Personally, I'd be happy if there is a way to define plugins under a state so that the systems added there are only run when the app is in that particular state. My intuition, overall, is that it makes sense to merge the concept of state and plugins. Somehow get rid of the concept of plugins and have everything be a state.

@Fidius-jko
Copy link

I want to catch errors from Plugin, becouse Plugin can broke my game (I want to implement mods to my game as Plugins. And disable if mod doesn't work)

@68317fa2
Copy link

68317fa2 commented Feb 15, 2024

Personally, I'd be happy if there is a way to define plugins under a state so that the systems added there are only run when the app is in that particular state. My intuition, overall, is that it makes sense to merge the concept of state and plugins. Somehow get rid of the concept of plugins and have everything be a state.

I also thought about this idea and I really liked it. So the only real purpose of Plugins is the registration of multiple systems at once. Even if you would have to open a UdpSocket or something similar, you could easily move that into a system, where you would have a whole lot more control over when to execute the system ( e.g. PostStartup or Startup). Every advantage of SystemSets, like the order and run criteria, you would want to have in Plugins too. So wouldn't it be possible to drop Plugins completely and implement sorts of predefined SystemSets, where you are able to add run criteria, etc? The only downsides would be, that a lot more functionalities of the app had to be brought into Systems or had to be implemented somewhere else, e.g. the initialization and implementation of States or other logic that happens in the Plugins build- or clean- functions.

I think the build-, ready-, clean-, etc. functions of Pluginss are generally something to reconsider with the idea above. Even though the idea of predefining SystemSets may break initial ideas of Plugins or SystemSets themself, I think it is really promising and provides a lot of customisability but also simplicity, like mentioned by @minecrawler. I hope I am not missing something about the philosophy and the ideas behind Plugins and SystemSets.

And could we maybe rename the issue to something like "Missing customisability of plugins", so that it is more clear, what the core problem is about?

So I thought about some code examples and came up with the following (I must admit, that I've used SystemSets in a wrong context in my above explanation):

fn main() {
   let test_plugin = (system_a, system_b.after(system_a).run_if(run_criteria))

   App::new()
      .add_systems(Update, test_plugin)
      .run();
}

fn system_a() {
   // ...
}
fn system_b() {
   // ...
}

So basically you have the systems predefined, as in the Plugins, but with the possibility for the developer to modify their run_criteria.

#[derive(SystemSet, Debug, Clone, PartialEq, Eq, Hash)]
struct Gameplay;

fn main() {
   let network_plugin = (ping_server, send_packets.after(ping_server).run_if(run_if_internet_connection_available))

   app.configure_sets(Update,
      Gameplay
         .run_if(run_if_player_alive)
   );

   App::new()
      .add_systems(Update, network_plugin.in_set(Gameplay))
      .run();
}

fn ping_server() {
   // ...
}
fn send_packets() {
   // ...
}

The problem here is, that it is not always clear how the run_criteria and SystemSets should be merged. Another problem is working with Scheduless, because it is not always clear, in which Schedule the developer intended the Plugin to be in. But maybe this is good, because sometimes you want to run Plugins in a custom Schedule. Also, a boilerplate-less definition of these predefined Plugins/Systems is probably only possible with a macro:

// plugin.rs

plugin!(test_plugin, (system_a, system_b.after(system_a).run_if(run_criteria)))

fn system_a() {
   // ...
}
fn system_b() {
   // ...
}

// main.rs

fn main() {
   App::new()
      .add_systems(Update, test_plugin.run_if(run_if_test_condition))
      .run();
}

I don't know how easy it is, to implement this, but I think it would be a breaking change, especially for the DefaultPlugins, so I'd like to hear your opinion about that @alice-i-cecile.

@alice-i-cecile alice-i-cecile changed the title Gameplay plugins cannot be configured to fit into users' games Plugins should be more customizable and work with states Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Needs Design
Development

No branches or pull requests

9 participants