Fix panic in update_viewport_render_target_size when despawning

[mgi388]

Objective

• Fix a panic when you despawn a camera linked to a ViewportNode.
• In Discord I was asking about why adding Msaa::Off to my camera causes a gray screen and it turns out it's related to my inspector egui camera. While trying to work out the cause, I was going through the egui inspector and despawning camera to see if it helped. While despawning one of my cameras which is referenced in a ViewportNode, I got this panic due to unwrap() being used.

Solution

• Don't unwrap but just ignore the camera if it no longer exists. This can easily happen if the camera entity is despawned, e.g., using DespawnOnExit state, or manually despawned in an editor/inspector.
• This means that the ViewportNode is left with a dangling/invalid reference. I'm not sure how to solve that though?
• Also update the doc to mention what happens.
• Note: There's another unwrap in the system but I don't want to change it in this PR. I didn't experience that panic so I'd rather leave it out of this PR (it may be a useful panic so should be considered separately).

Testing

• Tested in my Bevy 0.17 fork that it no longer panics.

[alice-i-cecile]

Panicking is not great, and I would like to avoid that!

This is a good non-breaking fix for 0.18.1, but I would be interested in a PR that swaps to an Option<Entity> for 0.19 in order to avoid this weird dangling state.

[mgi388]

Panicking is not great, and I would like to avoid that!

This is a good non-breaking fix for 0.18.1, but I would be interested in a PR that swaps to an Option<Entity> for 0.19 in order to avoid this weird dangling state.

@alice-i-cecile correct me if I'm wrong but a pub camera: Option<Entity>, wouldn't help in this case would it? The way I caused this bug was to despawn the linked camera in my inspector. If this field was an Option and I despawned the camera, prior to this PR, the code would still panic. That is to say, Option<Entity> still doesn't say the Entity within the option is going to exist, right?

Do relationships help here? I know that you can use them to auto-despawn linked entities, but in this case if the camera is despawned, does that mean we want to auto-despawn the entity with the ViewportNode on it? Looking at my own where I use ViewportNode, we see (as expected) that they are just arbitrary UI nodes/entities, and I'd find it weird if that part of the UI disappeared just because my camera was despawned.

Or, maybe this is just up to users to make sure they keep in sync themselves (if they care about dangling references here).

[alice-i-cecile]

So, changing this to an Option won't allow us to avoid the panic. The early return here would still be required.

Instead, changing it to an Option would make it clear that this behavior was supported: allowing you to keep around a ViewportNode without a render target. This is actually pretty nice, since it lets you keep around the UI elements while you swap scenes or the like.

My thought was that when the camera query fails, we set the value to None, so then the ViewportNode knows that the linked camera has been despawned (or is no longer a camera). Then, the user has an easy indication that something has gone wrong, and the state is relatively clear.

We could instead swap ViewportNode to be a relation, and it would good automatically cleaned up when the linked entity is despawned. But a) that wouldn't work if the Camera component was just removed, leading to a panic still and b) given the use case outlined above, I think that allowing a temporarily empty ViewportNode is actually useful.

[chompaa]

lgtm! i also agree with Alice's comments about switching to an Option for 0.19

[mgi388]

@alice-i-cecile ah got it, thanks for clearing that up for me.

Great, thanks @chompaa.

I made an issue to track the enhancement: #22641

I don't plan on working on it at this stage (just FYI), for anyone casual observers who want to pick it up.