Optimize spawn_batch + relations

[alice-i-cecile]

@chescock in #19519 (comment)_ says:

With this change, every branch in this hook (after the RelationshipHookMode check) is guaranteed to run a command, right? It might make sense as a follow-up PR to wrap the entire method in world.commands.queue(|world| {...}). That would avoid having to look up target_entity twice, which would also avoid the possibility of it getting despawned in between.

Huh, and it also looks like EntityEntryCommands::or_insert_with just invokes the closure immediately. If we work with world instead of commands then I think we can avoid allocating a new RelationshipTarget if it already exists. Also, I'm going to create an issue for or_insert_with, because that behavior is surprising ... #19884.

[gwafotapa]

@chescock

It might make sense as a follow-up PR to wrap the entire method in world.commands.queue(|world| {...}). That would avoid having to look up target_entity twice, which would also avoid the possibility of it getting despawned in between.

In that case, should we also be concerned that the calls at lines 126 and 148 may fail ?

Huh, and it also looks like EntityEntryCommands::or_insert_with just invokes the closure immediately. If we work with world instead of commands then I think we can avoid allocating a new RelationshipTarget if it already exists. Also, I'm going to create an issue for or_insert_with, because that behavior is surprising ... #19884.

If I understand you correctly, this issue is with EntityEntryCommands::or_insert_with and should resolve itself when #19884 is fixed, but deferring the whole thing would bypass that issue as a bonus as Entry::or_insert_with works as expected ?

[chescock]

In that case, should we also be concerned that the calls at lines 126 and 148 may fail ?

Oh, I hadn't thought about those! Looking at them now, I think those are probably fine either way because they want the component to be removed, so if something else removes it first then everything is still in the right state. (In contrast, if something despawned target_entity before the command executed then we wouldn't remove Self and would have a relation with no target.)

If I understand you correctly, this issue is with EntityEntryCommands::or_insert_with and should resolve itself when #19884 is fixed, but deferring the whole thing would bypass that issue as a bonus as Entry::or_insert_with works as expected ?

Exactly! Although I think a proper fix might be tricky, and if we "fix" it by removing the method then we'd still be doing the extra allocation here.

[gwafotapa]

Ok got it! That seems straightforward enough. I'll open the PR.