TaffyTree::set_children: Remove from previous parent

[SpecificProtagonist]

Objective

TaffyTree::set_children did not remove the children from their previous parent, resulting in an inconsistent hierarchy. Fix this.

Context

Causes a crash in bevyengine/bevy#16639.

This bug was present in previous taffy releases too; backporting it should be simple.

[nicoburns]

Hmm... I'm sympathetic to the idea that this ought to work like this. But this is a breaking change (of the kind where people might actually have been relying on the current behaviour) so I don't think we can release it as a 0.6 version. And the current behaviour isn't technically wrong, just unexpected.

Summoning the Unofficial Taffy Committee @alice-i-cecile @TimJentzsch @Weibye thoughts?

• Do we think that "move semantics" (fix up tree to remove from previous parent) are the correct default?
• Do we think there is any place for "raw" functions that don't attempt any smart behaviour?

[SpecificProtagonist]

And the current behaviour isn't technically wrong, just unexpected.

Hmm, I assumed it was unintentional because the type is called TaffyTree, and a node's parent is stored in a Option<NodeId>, not a Vec<NodeId>. As is, the current behavior means that set_children can result in nodes that are not their child's parent. TaffyTree::remove (and maybe other methods?) assumes that isn't the case and panics.

[TimJentzsch]

Hmm... I'm sympathetic to the idea that this ought to work like this. But this is a breaking change (of the kind where people might actually have been relying on the current behaviour) so I don't think we can release it as a 0.6 version. And the current behaviour isn't technically wrong, just unexpected.

Summoning the Unofficial Taffy Committee @alice-i-cecile @TimJentzsch @Weibye thoughts?

• Do we think that "move semantics" (fix up tree to remove from previous parent) are the correct default?
• Do we think there is any place for "raw" functions that don't attempt any smart behaviour?

First thoughts:

• I'm always a big fan of having the default behavior being as "safe" as possible. While I don't think we explicitly document it, having an invariant that the node structure is a tree and keeping this consistent makes sense to me. Then the "move" behavior would be a sensible way to uphold that invariant.
• For a low-level library like Taffy, offering "raw" functions that don't uphold internal invariants might make sense. However, I think we should only add them once someone asks us for it and then add a pre or postfix to the name to encourage using the "safe" API instead. E.g. "set_children_unchecked"

[alice-i-cecile]

Fully agree with @TimJentzsch's analysis here. I like the move semantics. I'm fine with carefully documented raw methods, but only when we actually have users asking for them.

[nicoburns]

Ok, sounds like there's consensus on this change :) Out of an abundance of caution (and a lack of consequences to bumping version numbers) I think I will consider this a breaking change and make the next release 0.7.0.

[nicoburns]

@SpecificProtagonist Released in Taffy 0.7. Bevy upgrade PR here: bevyengine/bevy#16780