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

Restrict available methods on App from Plugin::Build() #4231

Closed
Vrixyz opened this issue Mar 16, 2022 · 4 comments
Closed

Restrict available methods on App from Plugin::Build() #4231

Vrixyz opened this issue Mar 16, 2022 · 4 comments
Labels
A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@Vrixyz
Copy link
Member

Vrixyz commented Mar 16, 2022

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

When implementing a Plugin, a user has some footgun available:

What solution would you like?

Ordered from my personal preferred to less preferred:

If there are no uses to run an App from a Plugin::Build, a user shouldn't be able to, we could either:

  • pass ownership to App::run(self), the same way run_once works, so we couldn't call that from Plugin::Build because we have a &mut App.
  • Leverage the type system to avoid having App::run() and other footguns being available within the Plugin::Build() function body.

If there exists reasons (I'm curious to hear about them) to indeed run an app from a plugin, we could either:

  • Do the above, but provide another function like build_advanced allowing such rare behaviour ?
  • Add a warning when starting the app from a non-conventional place, offering a way to suppress those ?
  • Use tooling akin to ReportExecutionOrderAmbiguities to offer advanced diagnostics.
@Vrixyz Vrixyz added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 16, 2022
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 16, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 16, 2022

Related to #1255, #2160 and bevyengine/rfcs#33.

My preference is to remove the build paradigm entirely, and instead provide a structured approach for plugins to hook into.

There are a fixed number of things plugins can do (add systems, initialize types, add resources, set the runner, ...) and we could have defaulted trait methods to allow end users to only specify the properties that they wish to set.

That approach eliminates this footgun, enables order-independence, and makes it clearer to users how plugins are meant to be used.

@alice-i-cecile
Copy link
Member

That said, I think that simply passing ownership to App::run is a good simple solution for now, and should be implemented ASAP.

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 16, 2022

pass ownership to App::run(self), the same way run_once works, so we couldn't call that from Plugin::Build because we have a &mut App.

This would make it impossible to do either App::build().add_plugin(Foo).run() or let mut app = App::build(); app.add_plugin(Foo); app.run(); as either add_plugin accepts and returns the app by value in which case app.add_plugin(Foo); doesn't work, or it accepts and returns app by mutable reference in which case .run() doesn't work.

Maybe App could have a private field indicating if it is in the middle of an .add_plugin() operation and panic on .run() in that case?

@Vrixyz
Copy link
Member Author

Vrixyz commented Mar 17, 2022

Indeed we would lose function chaining, see diff on 3d_scene example:
image

Your comment might be interpreted as that solution would NOT work, but I'm positive that the code I linked works with App::run() taking ownership of app.

That said, I fully agree that losing function chaining is very suboptimal so I'll try to implement your suggestion 👍 .

Vrixyz added a commit to Vrixyz/bevy that referenced this issue Dec 21, 2022
@bors bors bot closed this as completed in ca87830 Dec 25, 2022
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
…bevyengine#4241)

# Objective

Fixes bevyengine#4231.

## Solution

This PR implements the solution suggested by @bjorn3 : Use an internal property within `App` to detect `App::run()` calls from `Plugin::build()`.

---

## Changelog

- panic when App::run() is called from Plugin::build()
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…bevyengine#4241)

# Objective

Fixes bevyengine#4231.

## Solution

This PR implements the solution suggested by @bjorn3 : Use an internal property within `App` to detect `App::run()` calls from `Plugin::build()`.

---

## Changelog

- panic when App::run() is called from Plugin::build()
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants