Optimize spawn_batch + relations

[gwafotapa]

Objective

Fixes #19891
Follow-up of #19519
The on_insert hook of Relationship is deferring an EntityCommand that could fail if the entity has been despawned and leave us in an invalid state. That command also uses EntityEntryCommands::or_insert_with which may perform an unnecessary allocation because it always runs its closure at the time of this writing.
See #19891 for details.

Solution

Defer the whole operation of the hook. This postpones the lookup, making sure the entity has not been despawned. It also uses Entry::or_insert_with instead of EntityEntryCommands::or_insert_with.

Testing

No additional testing.

[gwafotapa]

Not sure we want this branch but in the case where the original entity doesn't exist either, I thought that information could be useful to our user.

[chescock]

My first thought was that warning here was consistent with the old code, but now I'm leaning towards not logging anything.

If we hit this branch, then the entity was despawned in between the observer and the command. And the despawn hook would have tried to remove entity from the Self::RelationshipTarget when that happened. So it's okay that we can't add entity to Self::RelationshipTarget, because we want it removed anyway.

Looking at it another way: The warnings here are when you try to add a relation and the hook prevents it from being added. In this case, the hook didn't prevent it; some other hook or observer did by despawning the entity.

But mostly I don't expect that branch to ever be hit, so I don't think it matters much either way!

[gwafotapa]

I agree that branch is rarely ever hit and it hurts readability a bit. On the other hand, When it is hit, it means our user got some debugging to do! And even though the culprit is elsewhere, it helps to know that the hook failed instead of leaving our user in the dark.

[alice-i-cecile]

@gwafotapa, is there anything you need to do before pulling this out of draft?

[gwafotapa]

@alice-i-cecile Nope. Just forgot!

[chescock]

Looks good!

[chescock]

I think I was expecting to pull in the code starting from let target_entity = ... into the closure, as well, since it also ends with a command. But I can't convince myself it matters one way or the other, so keeping the change small seems better!

[gwafotapa]

I preferred having the target_entity locked in before deferring. If we get the target_entity from the command, our Relationship component could have been modified and we could end up applying the command to the wrong target. With that, it made sense to run the next block right away as well.

[chescock]

My first thought was that warning here was consistent with the old code, but now I'm leaning towards not logging anything.

If we hit this branch, then the entity was despawned in between the observer and the command. And the despawn hook would have tried to remove entity from the Self::RelationshipTarget when that happened. So it's okay that we can't add entity to Self::RelationshipTarget, because we want it removed anyway.

Looking at it another way: The warnings here are when you try to add a relation and the hook prevents it from being added. In this case, the hook didn't prevent it; some other hook or observer did by despawning the entity.

But mostly I don't expect that branch to ever be hit, so I don't think it matters much either way!

[chescock]

This comment is a little unclear without the context of the original PR. If I understand the problem, maybe something like

Suggested change

	// Deferring is necessary for batch mode
	// Defer in case we need to insert a `Self::RelationshipTarget` component.
	// It may be added by a hook for a different entity during batch insert,
	// so also defer checking whether it exists.

?

[gwafotapa]

You're right. This is unclear for posterity. I'll complete it.