Reduce panics in rendering code to facilitate render recovery#23349
Reduce panics in rendering code to facilitate render recovery#23349alice-i-cecile merged 4 commits intobevyengine:mainfrom
Conversation
There was a problem hiding this comment.
First review on this repo, sorry if my comments do not reflect an understanding of the error cases of the code.
Some of these will silently fail now instead of crashing the application, I think we should replace the unwrap removals on Result's with error tracing of some sort.
This is a first pass, so I plan to pull the changes down and familiarize myself with them little more before revisiting my own review.
Co-authored-by: Brennan Paciorek <50780403+BrennanPaciorek@users.noreply.github.com>
|
I like keeping panics for things that are Bevy responsibility to enforce. If it panics it's a Bevy bug. Panics make it a lot easier to catch issues in our tests. Should we keep panics in debug builds? Or should we make CI fail if there are error logs? |
|
Also please rename the PR to make it easier to read in the git log. The first objective would be better :) |
|
The generated |
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
|
I like keeping them as debug_asserts, imo. |
|
The issue is that these get hit in the recovery path. So we'd be panicking instead of recovering. I don't agree with making them error!, but okay. |
|
Yeah, we shouldn't treat these errors as panics any more than users should. If we want to panic on those errors IMO it should be the job of the render recovery handler. If devs want it to panic then set the handler to panic. |
# Objective - Completes goal and closes #23029 - Culmination of #22761, #23350, #23349, #23433, #23458, #23444, #23459, #23461, #23463, #22714, #22759, #16481 ## Solution - Add a release note. - Re-export a wgpu type that you need to match on to handle errors. ## Testing - cargo run --example render_recovery with all the other PRs merged in. Press 5 and then V, the app will not crash. Note that D for "destroy device" will still crash: this is a WGPU problem resolved by gfx-rs/wgpu#9281. # Note I opted not to change the default recovery behavior yet. I believe we need testing in user projects and just general trodding of this code path before committing to a new default. It works in a simple example, it might not work in a complex project. We need to field test this and likely iterate to really call this ready IMO.
…gine#23349) # Objective - Reduce panics in bevy renderer - Continuation of Render Recovery efforts bevyengine#22761 (can't reinitialize if we panic. I hit every single one of these on the device lost path.) ## Solution - Use early outs and error!s instead of hard panics. - If the panics are needed to maintain some kind of invariant, that should be documented. I don't see any comment saying they do, so I am assuming they do not. Correct me if I am wrong. ## Testing - CI --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Brennan Paciorek <50780403+BrennanPaciorek@users.noreply.github.com> Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
# Objective - Completes goal and closes bevyengine#23029 - Culmination of bevyengine#22761, bevyengine#23350, bevyengine#23349, bevyengine#23433, bevyengine#23458, bevyengine#23444, bevyengine#23459, bevyengine#23461, bevyengine#23463, bevyengine#22714, bevyengine#22759, bevyengine#16481 ## Solution - Add a release note. - Re-export a wgpu type that you need to match on to handle errors. ## Testing - cargo run --example render_recovery with all the other PRs merged in. Press 5 and then V, the app will not crash. Note that D for "destroy device" will still crash: this is a WGPU problem resolved by gfx-rs/wgpu#9281. # Note I opted not to change the default recovery behavior yet. I believe we need testing in user projects and just general trodding of this code path before committing to a new default. It works in a simple example, it might not work in a complex project. We need to field test this and likely iterate to really call this ready IMO.
Objective
Solution
Testing