Panic rendering sprite

[timbess]

Bevy version

main#e018ac838d164ce00a8c8fec3748787aaddb0e05

Operating system & version

Linux 5.10.83-1-MANJARO #1 SMP PREEMPT Wed Dec 1 14:46:38 UTC 2021 x86_64 GNU/Linux

What you did

It happens extremely itermittently, seems to be a timing issue.

Occasionally while spawning sprites this line will panic:
https://github.com/bevyengine/bevy/blob/main/crates/bevy_sprite/src/render/mod.rs#L526

What you expected to happen

It should probably log a warning rather than killing the app.

What actually happened

The app panicked.

Additional information

Seems to happen consistently on my debug builds and not on my release build. I expect the optimizations change the timing and fix the bug.

Maybe this should be another issue, but I noticed there are ~150 unwraps in the bevy codebase.

Might be useful to address each of these scenarios and revisit whether it's worth killing the app, and if so, attach an appropriate error message indicating what happened.

[DJMcNab]

Permalink to the relevant line:

bevy/crates/bevy_sprite/src/render/mod.rs

Line 526 in e018ac8

let gpu_image = gpu_images.get(&batch.handle).unwrap();

[DJMcNab]

I think the path for that unwrap to fail is due to the assets event system being delayed, i.e. when a new image is created, and added to a sprite, but the asset event hasn't reached the asset events system yet.

I'm not sure of the stage order to know how wide that opportunity is. I wonder if it's worth making the Assets<T> the source of truth for AssetEvent<T>s?

[timbess]

Is there not a way to "await" a handle? Or would you just ignore it for a few frames and do if let Some(foo) = gpu_images.get(thing)?

[DJMcNab]

I think we need more context to what your code is doing; when extracting sprites, we confirm whether their image exists before extracting them, and the GPUImages are created based on the AssetEvents<Image>, which is synced up to Assets<T> at the end of each frame. Therefore, as far as I can see, the path to this happening is from adding an asset after the AssetStage::AssetEvents stage, or during one of the render app stages. Those are both extremely strange things to do.

That being said, we should find a way to remove that possible window.

Can you please create a minimal reproduction?

[timbess]

I'll see if I can make a reproduction, it's tough with timing bug like that though. I expect it's related to this:
https://github.com/StarArawn/bevy_ecs_tilemap/blob/d7c4f8c8aa7775061f38aaf22bf2040610012f31/examples/tiled/tiled.rs#L61

Either way though, the unwraps need to be addressed. Unrecoverable errors like panics should be reserved for truly exceptional cases, and even then, should come with a descriptive scenario like you described in your comment. If it's the sort of thing that can be skipped until a few frames later when the asset loads... maybe it should trigger an error log and move on rather than blowing everything up.

[cart]

I agree that its worth doing a pass over Bevy's unwraps to see what can be removed, but there is a class of unwrap that should definitely not be removed: a code-author-validated assertion about the behavior of code (thing X must always exist because Y) that other parts of that code scope rely on to behave properly. These types of errors often should not be bubbled up to users, nor should they be silently ignored or logged to the console (because the code that follows makes additional assumptions building on top of the asserted behavior) . Most of them should be converted to expect("BEHAVIOR_AND_RATIONALE_HERE") to properly encode the intent of the author / why this unwrap is "safe".

A good portion of unwraps that dont fall into that category are either from tests, examples, or docs. And most of those shouldn't be removed either.

So yeah, I'm on board for a pass to identify unwraps that shouldn't be there (and a pass to encode rationale in unwraps that should be there). But "code should never unwrap" isn't something I can get behind. Counting unwraps as a measure of code quality isn't a particularly useful metric in mind.

[cart]

That being said, we should find a way to remove that possible window.

Yeah the separation is there for UX reasons (asset events are consumed the same way as other events). But the separation does introduce UX issues in cases like the one mentioned above, so its a tradeoff. I'm open to proposals that improve UX, as long as they take "event consumption consistency" into account (at least as part of the argument, even if we trade consistency in the interest of resolving sync issues).

[timbess]

Agreed on all points. Though if you take it very literally, "code should never unwrap" is true in that you should encode your invariant into the "expect" rather than calling "unwrap" like you pointed out (excluding tests and other things irrelevant to the output). Good point on the tests and stuff, so probably much less than 150 then.