Don't panic when duplicate plugins are added

[alice-i-cecile]

Objective

Adding duplicate plugins is generally a mistake in user code, resulting in overridden settings, duplicate systems, doubled-up entities and more.

As a result, we plugin authors can set the is_unique attribute on their plugin, preventing it from being added multiple times.

This was added in #6411, and by default this flag is true. This is a good default! However, in that PR, we chose to make this failure mode panic. This is frustrating to users because:

1. Plugins are not always added directly by the user.
2. We do not report where the duplicate plugin was added very well.
3. The standard is_plugin_added workaround doesn't work for plugin groups.

This was encountered in #21111, but has been widely complained about in #18909 and #15802.

Ultimately, this is not a critical failure: we can simply log a warning and not insert the second copy.

Fixes #21111, closes #18909.

Solution

In the spirit of #2337 and the blessed #14275, downgrade this failure mode to a warning and do the right thing.

Note that I cannot use the standard logging tools here, since bevy_log may not be initialized yet due to #1255.

Note to reviewers

Would I prefer to have #69 instead? Yes! But this patch still moves us in the correct direction and alleviates a major user pain point.

Question for reviewers

This behavior is completely harmless in the case of non-configurable plugins, but fairly concerning in the case of configurable plugins. Would you prefer a silently ignored / panic split based on the size of the plugin type?

[alice-i-cecile]

This behavior is completely harmless in the case of non-configurable plugins, but fairly concerning in the case of configurable plugins. Would you prefer a silently ignored / panic split based on the size of the plugin type?

After chewing on this a bit while writing this PR description, this is my preferred solution. I'll implement this tomorrow unless someone convinces me not to.

[hukasu]

how would this deal with plugins that can take values?

app.add_plugin(
  MyPlugin {
    flag: true,
  }
);
app.add_plugin(
  MyPlugin {
    flag: false,
  }
);

[alice-i-cecile]

how would this deal with plugins that can take values?

app.add_plugin(
  MyPlugin {
    flag: true,
  }
);
app.add_plugin(
  MyPlugin {
    flag: false,
  }
);

Yeah, this is why I'm leaning towards a branch on whether or not the plugin is a ZST: panicking in those cases is quite reasonable!

[janhohenheim]

allowing ZSTs and keeping the old behavior otherwise sounds completely reasonable to me :)

[mockersf]

This behavior is completely harmless in the case of non-configurable plugins, but fairly concerning in the case of configurable plugins.

I don't think this is true. Systems are added twice, pipelines are created twice, ...

If we want to remove the panic, we should make the api return a result

[janhohenheim]

Can't we just skip the duplicate ZST plugin? I.e. not run its lifecycle?

Edit: yeah, that's what this PR is doing, isn't it? Just scope that to ZSTs and nothing should be added / registered twice

[cart]

Rather than all of this "pomp and circumstance", can't we just name the bound field _plugin_name?

[janhohenheim]

or add a let _ = plugin_name; to the end of the scope :)

[janhohenheim]

Or even better, learned from @janis-bhm: #[allow(clippy::allow_attributes, unused_variables, reason = "...")]

[cart]

I think changing behavior based on the number of fields / size of the typee is confusing / hard to document / hard to understand. Additionally, nothing is stopping a ZST plugin from behaving differently at different times, or feeding on data that might change.

I think we either need to embrace "unique plugins registered more than once ignore the configuration from the second plugin and warns" or "double-registering unique plugins aborts app execution".

I do think the warning makes sense, as plugins shouldn't need to coordinate / anticipate what other plugins might also register. Of course, the true fix is deferred plugin init, which would embrace the "only one instance, with shared config" approach.

[janhohenheim]

Imo that's fine in this case, since it's not behavior you have to actively think about. It's just an instance of one less panic in a specialized case.

[hukasu]

Panic at startup is very harmless compared to some during runtime, it is not like you are gonna miss it during development, it is annoying if you have to go to a library maintainer to ask them not to add external plugins, I won't deny, but this is something that is best dealt with the required plugins logic than with what is being proposed here

[janhohenheim]

Why bother though? Asking the other plugin to not add a ZST plugin has exactly the same effect in practice as what is proposed here. Imo we have a sensible and safe way to automate ecosystem tedium upstream with a tiny PR.

[hukasu]

this is primarily so that libraries can add the plugins that they require without the need for testing if it is added previously, correct?
i propose something like this

app.require_plugin::<RenderPlugin>();
app.on_build_plugin_hook::<RenderPlugin>(run_post_render_plugin_build, true);
app.on_finish_plugin_hook::<RenderPlugin>(run_post_render_plugin_finish, false);

where

fn require_plugin<T: Plugin>();
fn on_[build|finish]_plugin_hook<T: Plugin>(system: fn(&mut App), required: bool);

Marking a plugin as required will cause the app to fail due to it missing.
Creating hooks that have required = true will also fail if the plugin is missing. The on build hook will run after the build or finish methods, if the on build hook is added to a plugin that is already added, then run it immediatly

[hukasu]

a visual example of what cart said

struct MyPlugin;
impl Plugin for MyPlugin {
  fn build(app: &mut App) {
    if app.is_plugin_added::<SomeOtherPlugin>() {
      <do something else>
    }
  }
}

fn main() {
  let mut app = App::new();
  app.add_plugins(MyPlugin);
  app.add_plugins(SomeOtherPlugin);
  app.add_plugins(MyPlugin);
}

the first time the MyPlugin is added the if is false, but the second time it would've been true

[janhohenheim]

ZST clearly can have divergent behavior, but is that a meaningful distinction? Is that something that actually happens in the wild?

[hukasu]

Is that something that actually happens in the wild?

It only needs to happen once for people to get really mad, because this divergent behavior would be a pain to debug