aevyrie / bevy_mod_picking

Picking and pointer events for Bevy.
https://crates.io/crates/bevy_mod_picking
Apache License 2.0
764 stars 170 forks source link

Use `try_insert` to prevent `panic`s #299

Closed janriemer closed 6 months ago

janriemer commented 8 months ago

When using insert instead of try_insert on EntityCommand, it can happen that the system panics, when the entity does not exist, even when the EntityCommand has been accessed with get_entity(...). The following example demonstrates the problem:

if let Some(mut entity_commands) = commands.get_entity(entity) {
    // at this point some other system despawns `entity` concurrently
    entity_commands.insert(bundle);
    //              ^^^^^^ this will panic
}

This is also documented in the official bevy docs: https://docs.rs/bevy/0.12.1/bevy/ecs/system/struct.Commands.html#method.get_entity

Other occurrences of .insert (where probably no change is necessary)

I've also looked at other occurrences of .insert on EntityCommand in the code, but I don't think we will have problems there, when .insert is used, because it is directly retrieved from the Query of the same system, but I'm not sure. What do you think?

Here are other locations of .insert: https://github.com/aevyrie/bevy_mod_picking/blob/1471a9f83435f3fdd43829b90391e6ab965e0e73/backends/bevy_picking_egui/src/lib.rs#L64-L71

https://github.com/aevyrie/bevy_mod_picking/blob/1471a9f83435f3fdd43829b90391e6ab965e0e73/crates/bevy_picking_highlight/src/lib.rs#L287-L296

aevyrie commented 6 months ago

Looks good. I've also included your other suggested changes. Thanks!

janriemer commented 6 months ago

Thank you for merging and also adding it to the other call sites!❤