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

Add error when extract resource build fails #4964

Merged
merged 6 commits into from
Apr 28, 2024

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Jun 7, 2022

Objective

  • Provide feedback when an extraction plugin fails to add its system.

I had some troubleshooting pain when this happened to me, as the panic only tells you a resource is missing. This PR adds an error when the ExtractResource plugin is added before the render world exists, instead of silently failing.

image

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jun 7, 2022
@alice-i-cecile
Copy link
Member

Another one for #1255...

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a .expect() is appropriate here. It should be a fatal error imo as the code is wrong. Also, why did the render app not exist yet? This is a plugin. Did you add it before DefaultPlugins or something? @alice-i-cecile @cart what do you think? Fatal or not?

@alice-i-cecile
Copy link
Member

Definitely fatal; I don't think there's any way to recover from this.

The fix is always just "order your plugins better", as much as I hate that.

@aevyrie
Copy link
Member Author

aevyrie commented Jun 8, 2022

Good point. expect is more appropriate.

@cart
Copy link
Member

cart commented Jun 11, 2022

Iirc we adopted the if let Ok(render_app) pattern to ensure we dont panic when "headless" mode is enabled. We should make sure that switching to expect doesn't break headless.

@aevyrie aevyrie requested a review from superdump April 28, 2024 02:19
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@superdump superdump added this pull request to the merge queue Apr 28, 2024
Merged via the queue into bevyengine:main with commit 4b446c0 Apr 28, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants