bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Make RenderPlugin emit only a soft error when there's no gpu, instead of panicking

Open anchpop opened this issue 1 year ago • 9 comments

  • Depends on #5932

Objective

  • Fixes #5931

Solution

  • In initialize_renderer, I just replaced the .expect with a error!, and made the function return an Option
  • In bevy_render/src/lib.rs I wrapped some code in an if let. (The diff is huge, but it's just indentation changes.)
  • Now that RenderPlugin will work in gpu-less environments like CI, I added it to TestPlugins.

Changelog

Fixed

  • RenderPlugin no longer panics when run on a device without a GPU.

anchpop avatar Sep 10 '22 02:09 anchpop

What is the advantage of this? If there is no GPU but the render plugin is used, the app is in a broken state. It doesn't make sense to needlessly consume resources (it will likely saturate at least a single core due to no vsync) as opposed to exit.

bjorn3 avatar Sep 10 '22 07:09 bjorn3

@bjorn3 The rationale is explained in the linked issue.

irate-devil avatar Sep 10 '22 08:09 irate-devil

What is the advantage of this? If there is no GPU but the render plugin is used, the app is in a broken state. It doesn't make sense to needlessly consume resources (it will likely saturate at least a single core due to no vsync) as opposed to exit.

After testing on my mac (by causing initialize_renderer to always return None), a window will still be created that can be closed by the user. So at least it probably won't silently consume CPU in way that has to be killed via task manager.

anchpop avatar Sep 10 '22 16:09 anchpop

In my opinion, this would be better as a cargo feature disabling rendering when not needed.

mockersf avatar Sep 10 '22 16:09 mockersf

In my opinion, this would be better as a cargo feature disabling rendering when not needed.

Can you selectively disable cargo features for specific tests? If so, how?

alice-i-cecile avatar Sep 10 '22 16:09 alice-i-cecile

Alternatively we could add a method to DefaultPlugins which disables the render and window plugins.

bjorn3 avatar Sep 10 '22 16:09 bjorn3

Can you selectively disable cargo features for specific tests? If so, how?

You can select the tests and the features to use when running the command cargo test. But why would you want some tests with rendering and some without?

mockersf avatar Sep 10 '22 16:09 mockersf

Can you selectively disable cargo features for specific tests? If so, how?

You can select the tests and the features to use when running the command cargo test. But why would you want some tests with rendering and some without?

I suspect it's simpler from the user's perspective to do what @bjorn3 said (so that's what I did!). But I'm happy to do it with conditional compilation, if you think it's better.

anchpop avatar Sep 10 '22 21:09 anchpop

I like the idea, but should we move it to a separated bevy_tests crate? So users would use as [dev-dependencies]. We may also add other useful tests utilities in the future.

I don't like the ideia of for_testing being available in non-testing builds.

That's an interesting idea. Would it live in a separate github repo, or would it still be in this one? I worry that it adds a lot of complexity (for users, for docs, for examples, etc) for not that great a benefit. A user using the testing plugins would not get a usable game.

But on the other hand, I love the idea of having a separate crate with additional utilities for testing. Another option would be to add an optional feature which exposes a bevy::tests module, and direct users to configure cargo to enable that feature when running cargo test.

All that said, I'd like to get this merged and it feels a bit preemptive to make a whole crate or module to just export one struct. If there's interest, I'll write an automated testing RFC with an outline for more features and improvements to the organization. Would you be willing to approve the merge without this change?

anchpop avatar Sep 11 '22 08:09 anchpop

Closing this out; https://github.com/bevyengine/bevy/blob/main/examples/app/no_renderer.rs should just work.

alice-i-cecile avatar Sep 18 '22 15:09 alice-i-cecile