Quit Application on any RenderErrors by default#24131
Quit Application on any RenderErrors by default#24131kfc35 wants to merge 7 commits intobevyengine:mainfrom
RenderErrors by default#24131Conversation
| // do nothing | ||
| } | ||
| RenderErrorPolicy::QuitApplication => { | ||
| main_world.write_message(AppExit::error()); |
There was a problem hiding this comment.
Ugh, we only get a u8 to embed error information.
alice-i-cecile
left a comment
There was a problem hiding this comment.
I think this is the right compromise, but the docs absolutely need to be clearer.
Strong, strong preference to sending AppExit over panicking: this is something you can 100% attempt an auto-save on.
|
Would it be an option to by default only exit when there are multiple render errors within a small timespan? Say 3 errors within 5 min or 10 within an hour. |
I think this is a good idea, thank you! But I think this can be a follow up. I think optimizing the default on how to better exit can come after the decision to default exit on any error (this PR). (I think there is still the potential for flashing if you accept x number of errors within some minutes, if the errors come in rapid succession. In that case, you’d probably want to pause the renderer before attempting to render again, and that just seems like a more complicated render error policy that I do not feel like figuring out at the current moment.) IIUC other apps will be able to implement that policy independently and replace the default handler, so this won’t block them from doing so. |
|
Note that bevy (and wgpu) have historically had tons of validation errors all the time, for me there hasn't been a release that runs 3d_scene without console spamming validation errors on vulkan since like 0.11 or so. This very likely would regress example UX. I investigated the render recovery paths a while ago around when the strobe issue was first reported and found no logic errors, the only divergence seems to be having device error handlers at all. This may be a wgpu bug with how it behaves when a handler is present, I'll investigate once I have more time on my hands |
tychedelia
left a comment
There was a problem hiding this comment.
I'm a bit confused about the behavior here. We should definitely not exit on regular vulkan validation errors.
I can put an if-block there if we can trust that, specifically, vulkan validation errors won’t cause light strobing. I’ll do so in a bit. My reasoning behind making all validation errors exit is because I’ve personally seen it cause strobing on I’m not experienced enough in rendering to know which validation errors could or could not cause strobing, but if Vulkan validation ones are not going to strobe, I can put that exception in. |
|
Context: On Mac/Metal, I’m able to see a strobing pink screen when I checkout this commit 6fcf9a6 and I run the volumetric_fog example: Under @atlv24 ’s suggestion, I commented out the internals of these wgpu handlers defined in device.set_device_lost_callback(move |reason, str| {
// bevy_log::error!("Caught DeviceLost error: {reason:?} {str}");
// assert!(device_lost.lock().unwrap().replace((reason, str)).is_none());
});
device.on_uncaptured_error(Arc::new(move |e| {
// bevy_log::error!("Caught rendering error: {e}");
// uncaptured
// .lock()
// .unwrap()
// .get_or_insert(WgpuWrapper::new(e));
}));The app still strobes. If I comment even the setting of the handlers out, it panics (which is what I would expect from my previous comment regarding wgpu behavior). It was suggested that this means the bug is in wgpu. However, I think the app strobes because the app has not changed For what it's worth, I’m unable to reproduce the pink screen flashing in the Ultimately, I still remain convinced this PR should be considered. However, since strobing was not able to be reproduced on Vulkan (tested in Discord), and the pink screen effect seems to be a Mac specific thing (#20318), I think I can agree to ignore validation errors for non Metal adapters only at this time (I assume this is just a matter of checking |
|
This change is correct in spirit: the slip-up earlier was the belief that wgpu validation error == vulkan validation error. Vulkan validation errors are a different layer outside wgpu, but look very similar to the point of being easy to confuse: I think char and I were both operating under the assumption that your PR would make vulkan validation errors stop the renderer. This is not the case. the handler is only for wgpu validation errors. |
Just do it in the default handler Co-authored-by: atlv <email@atlasdostal.com>
809d8cd to
2c0706c
Compare
|
The generated |
Objective
Solution
Instead of ignoring any errors and continuing to render, this PR changes the default error policy to quit the application upon any error. OOM and Validation error induced strobing will not happen cause the app should just quit. I’m not aware of strobing that could happen on DeviceLost / Internal errors, but I’m leaning towards safety here. You can correct me (within reason) if there are some obvious errors we should handle with a different error policy, but I think this is a better starting point than
Ignorefor all.Note: At times, even with this code, I’ve seen the app flash to a magenta/pink color before exiting (not a strobing pink that would happen if the app ignored validation errors, just a single abrupt switch before app exit), so I’m wondering if it’s better to throw a
panic!rather than try to quit gracefully.Testing
pccmexample andlight_probe_blendingexamples are still broken onmain.cargo r --example light_probe_blending --features free_camera,httpscorrectly closes the app upon Validation errorcargo run --example pccm --features="free_camera https”does the same.Example console logs
There is some
WARN bevy_ecs::world::command_queue: CommandQueue has un-applied commands being dropped. Did you forget to call SystemState::apply?spam (like at least 20+ lines of it) after quitting the application; if anyone knows if that could be prevented somehow, I’m keen to learn.