Prevent setting parent as itself

[lewiszlw]

Objective

• Fixes Stack overflow error when bevy_ui node has circular Parent Children dependency #8979.

Solution

• Panic when parent and child are same entity.
• Add checks into EntityCommands and EntityMut methods

[farend-east]

The fix must also apply to InsertChildren PushChildren and ReplaceChildren

[lewiszlw]

The fix must also apply to InsertChildren PushChildren and ReplaceChildren

Added!

[nicopap]

This doesn't solve the problem for the EntityMut methods. Those methods are called in the functions you modified, so you can just move the check to the EntityMut methods.

In this impl block:

impl<'w> BuildWorldChildren for EntityMut<'w> {

With regard to EntityCommand implementations, Adding a panic in the commands methods would be ideal, so that the panic occurs, the backtrace shows exactly what code caused the issue.

Add the panic in the relevant functions in this impl block:

impl<'w, 's, 'a> BuildChildren for EntityCommands<'w, 's, 'a> {

The current change only solves the issue for parents inserted through commands, and has poor backtraces.

[lewiszlw]

This doesn't solve the problem for the EntityMut methods. Those methods are called in the functions you modified, so you can just move the check to the EntityMut methods.

Yeah, putting checks into EntityMut methods should be better.

With regard to EntityCommand implementations, Adding a panic in the commands methods would be ideal, so that the panic occurs, the backtrace shows exactly what code caused the issue.

I think the panic message might be enough. Waiting for more opinions.

[farend-east]

We still need the same logic for EntityCommands as this is where the user will mainly call from commands.
It can be the same logic as the one added in EntityMut.

The function set_parent for both impl will also needs this check .

[nicopap]

We still need the same logic for EntityCommands as this is where the user will mainly call from commands.

Technically, the commands then later will call the EntityMut methods with the same parameters, so it will panic. But I think they should be added to EntityCommands regardless.

I would strongly advise to add the check in the EntityCommands extension methods. Have you ever hit the "couldn't despawn entity, it is already despawned" error? It's so annoying, and it's basically impossible to know what code caused it.

This is because the panic is located in a different system than the one that triggered the despawn.

Pushing the panic in the EntityCommands extension methods will make the panic run within the system that tries to add the parent as child, as a result, the system will be included in the backtrace. With this, it's trivial for the bevy user to find where the error happens and fix it. Without it, it can become a struggle to figure out how to avoid the panic.

[lewiszlw]

I would strongly advise to add the check in the EntityCommands extension methods. Have you ever hit the "couldn't despawn entity, it is already despawned" error? It's so annoying, and it's basically impossible to know what code caused it.

make sense.

[alice-i-cecile]

I don't love adding more ways for commands to panic, but I think that surfacing the error early when the command is initialized is ultimately a nicer UX.

[mockersf]

Some of those panics are added not in the commands execution, but in the command creation, so while we could still return a result.

If we want to panic in those methods, a panic section should be added to their docs

[lewiszlw]

Added panics doc