use get_entity_mut in insert commands to avoid a panic

[expenses]

At the moment, the insert and insert_bundle commands issued through a Commands struct in a system can cause a panic when applied if the entity the command is used for no longer exists. This fixes that problem via the use of get_entity_mut, and brings them inline with the other commands.

[alice-i-cecile]

bors try

[alice-i-cecile]

This seems like a sensible change and is well motivated. The only possible thought is that it might hurt performance in noticeable ways.

Are you alright trying to run some benchmarks? We can walk you through it.

[expenses]

I can't possibly see it having a performance impact, as entity_mut just calls get_entity_mut and panics if None: https://github.com/expenses/bevy/blob/653c10371e546176059bb779844c0569c0190b6b/crates/bevy_ecs/src/world/mod.rs#L213

[alice-i-cecile]

Great! I'm going to toss on the ready-for-cart label then.

[bors]

try

Build succeeded:

• check-markdown-links
• markdownlint
• build-wasm (stable, ubuntu-latest)
• build-wasm (nightly, ubuntu-latest)
• check-doc
• build (stable, macos-latest)
• build (stable, windows-latest)
• run-examples
• build (nightly, ubuntu-latest)
• build (stable, ubuntu-latest)
• build-android

[NathanSWard]

A few observations:

1. I wouldn't really call this a bug and just rather a design decision. If the default behavior is to panic if an entity does not exist, that merely a choice by the implementor.

2. with this change, if someone tries to insert a Bundle or Component onto an entity and that entity is invalid, this just silently succeeds. This is not good imo. We need to at least add an error! message.

3. this doesn't really address the primary problem, which is error handling with Commands. Ideally we want a user to configure what the expected behavior is when it comes to fallible commands. (Either panic, log, silently fail, handle the error)

I actually have a PR incoming (most likely today) that's adds custom error handling for fallible commands.

And the good news is that the new default is to not panic! and error! 😊😊

[mockersf]

related to #2004 and #2219

[expenses]

I agree that the error should probably be propagated. In the case of applying commands at the end of a stage, silently ignoring an error is probably the right behaviour.

[NathanSWard]

@expenses are we ok to close this PR in favor of #2241 ?

[expenses]

@expenses are we ok to close this PR in favor of #2241 ?

Yep, that branch fixes the panic for me.