Add `try_add_plugins` for apps a la `try_insert` for entities

[mgi388]

Discussed in #15802

^{Originally posted by mgi388 October 10, 2024}
We have try_insert for entities. Doc:

Unlike [Self::insert], this will not panic if the associated entity does not exist.

x.try_insert(CombatBundle {
  health: Health(100),
  strength: Strength(40),
});

Could we have try_add_plugins for apps? Doc:

Unlike [Self::add_plugins], this will not panic if any of the plugins are already added.

Instead of this:

if !app.is_plugin_added::<DebugUiPlugin>() {
  app.add_plugins(DebugUiPlugin);
}
if !app.is_plugin_added::<WireframePlugin>() {
  app.add_plugins(WireframePlugin);
}

Which gates against panics like this:

Error adding plugin bevy_pbr::wireframe::WireframePlugin: : plugin was already added in application

The new function would let us do this:

app.try_add_plugins((WireframePlugin, DebugUiPlugin));

Motivation: I have 10s of lines of code of plugins being added only if they aren't already added and it would save lines but also possible bugs (when you copy paste and forget to change both instances of the plugin name).

I know there are lots of floating ideas around plugin dependencies / requirements, etc. but I wonder if this is a small enough API additional that it doesn't hurt in the long run even if/when a new API arrives.

[alice-i-cecile]

I think I'm on board with this: we use this pattern repeatedly. I agree that this is a stopgap, but it'll be easy to migrate and improve things now.

[mockersf]

Unlike entities which are dynamic and happens during playtime, plugins are added at application setup time and shouldn't be dynamic, so it's easy for the person coding the app to notice the issues and fix them.

try_insert is for avoiding panics encountered by players, this doesn't map to try_add_plugins.

We also don't want to normalise on "plugin" dependencies as that would reduce modularity and make it harder to swap between plugins

[mgi388]

We also don't want to normalise on "plugin" dependencies as that would reduce modularity and make it harder to swap between plugins

As far as the current version of plugins goes, I suppose the ship has set sail on this though, right? As in, we have already normalized on it, it's just that the code looks like this and is 3 lines:

if !app.is_plugin_added::<DebugUiPlugin>() {
  app.add_plugins(DebugUiPlugin);
}

When it could look like this:

app.add_plugins_if_not_added(DebugUiPlugin);

I used a different name this time to stay away from try_, just to move my comment forward, but I'm not saying I like the name.

With this new code, we still haven't normalized any more or less than what was there before, right? It's just that it's only 1 line of code and has no bugs.

I may not understand what you're getting at with your comment and how adding a helper like this would make it harder to swap between plugins anymore than the alternative 3 line version.

---

Just to make it clear, I don't feel dogmatic about this at all. I like the convenience this would offer when we're already doing it anyway, but if this is clearly controversial, I'm OK with that.

[cBournhonesque]

I would love this feature. There's dozens of places in my code where I have to check if the plugin was already added before adding it to avoid panics

[alice-i-cecile]

Could we make this a plugin-level setting / behavior? "Silently ignore duplicate copies with no effects" seems like a sensible behavior, and makes much more robust than adding a method that users need to remember to call.

[cBournhonesque]

I guess we already have Plugin::is_unique; we could expand this to:

pub enum ReAddBehaviour {
  // error if the plugin is added again to an App (similar to what happens if Plugin::is_unique = True)
  ErrorIfReAdd,
  // don't do anything if the plugin tries to be added again (similar to calling `if !app.is_plugin_added() { app.insert(plugin) } `
  SkipIfReAdd,
  / re-insert the plugin again (similar to what happens if Plugin::is_unique = False`
  ReinsertIfReAdd
}