YGNodeFree does not mark parent as dirty #1659

New Issue

2024-05-30T03:52:17-07:00

robbert-ef commented

2024-05-30 03:52:17 -07:00

(Migrated from github.com)

Report

I have searched existing issues and this is not a duplicate

Issues and Steps to Reproduce

YGNodeRef root = YGNodeNew();

YGNodeRef child0 = YGNodeNew();
YGNodeInsertChild(root, child0, 0);

YGNodeRef child1 = YGNodeNew();
YGNodeInsertChild(root, child1, 1);

YGNodeRef child2 = YGNodeNew();
YGNodeInsertChild(root, child2, 2);

YGNodeRef child3 = YGNodeNew();
YGNodeInsertChild(child2, child3, 0);

// Calculate so all nodes are clean
YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);

bool dirty_before = YGNodeIsDirty(root); // == false

// YGNodeFree does not mark root as dirty
YGNodeFree(child0);
bool dirty_after_free_child0 = YGNodeIsDirty(root); // == false

// YGNodeFreeRecursive does not mark as dirty when node has no children
YGNodeFreeRecursive(child1);
bool dirty_after_free_child1 = YGNodeIsDirty(root);  // == false

// YGNodeFreeRecursive marks as dirty because it calls YGNodeRemoveChild which marks dirty
YGNodeFreeRecursive(child2);
bool dirty_after_free_child2 = YGNodeIsDirty(root); // == true

Expected Behavior

I would expect YGNodeFree and YGNodeFreeRecursive to always mark its parents as dirty as it affects the positioning and sizing of other nodes.

Actual Behavior

YGNodeFree never marks its parent as dirty
YGNodeFreeRecursive only marks its parents as dirty when it has children, this is because it calls YGNodeRemoveChild which marks the parent as dirty.

# Report - [x] I have searched [existing issues](https://github.com/facebook/yoga/issues) and this is not a duplicate # Issues and Steps to Reproduce ```cpp YGNodeRef root = YGNodeNew(); YGNodeRef child0 = YGNodeNew(); YGNodeInsertChild(root, child0, 0); YGNodeRef child1 = YGNodeNew(); YGNodeInsertChild(root, child1, 1); YGNodeRef child2 = YGNodeNew(); YGNodeInsertChild(root, child2, 2); YGNodeRef child3 = YGNodeNew(); YGNodeInsertChild(child2, child3, 0); // Calculate so all nodes are clean YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); bool dirty_before = YGNodeIsDirty(root); // == false // YGNodeFree does not mark root as dirty YGNodeFree(child0); bool dirty_after_free_child0 = YGNodeIsDirty(root); // == false // YGNodeFreeRecursive does not mark as dirty when node has no children YGNodeFreeRecursive(child1); bool dirty_after_free_child1 = YGNodeIsDirty(root); // == false // YGNodeFreeRecursive marks as dirty because it calls YGNodeRemoveChild which marks dirty YGNodeFreeRecursive(child2); bool dirty_after_free_child2 = YGNodeIsDirty(root); // == true ``` # Expected Behavior I would expect `YGNodeFree` and `YGNodeFreeRecursive` to always mark its parents as dirty as it affects the positioning and sizing of other nodes. # Actual Behavior `YGNodeFree` never marks its parent as dirty `YGNodeFreeRecursive` only marks its parents as dirty when it has children, this is because it calls `YGNodeRemoveChild` which marks the parent as dirty.

👍 1

NickGerleman commented

2024-06-22 22:04:30 -07:00

(Migrated from github.com)

This seems like a sane enough behavior to change, with the current model where YGNodeFree will mutate the tree around it to disconnect the node first.

An aside, I don't like that YGNodeFree manipulates the tree around it, instead of just freeing the node. E.g., we needed to add a separate YGNodeFinalize which doesn't do this, because GC'ing Yoga nodes could happen in parallel in Java bindings. It's probably not worth breaking the existing folks relying on this behavior, but I would consider doing any sort of unhooking of nodes from your Yoga tree, before freeing parts of it.

Probably not going to happen any time soon, but I have had some desire to add an intrinsic ref-count to the nodes, and deprecate YGNodeFree for something like YGNodeRelease. I think it could be done in a backwards-compatible way.

This seems like a sane enough behavior to change, with the current model where `YGNodeFree` will mutate the tree around it to disconnect the node first. An aside, I don't like that `YGNodeFree` manipulates the tree around it, instead of just freeing the node. E.g., we needed to add a separate `YGNodeFinalize` which doesn't do this, because GC'ing Yoga nodes could happen in parallel in Java bindings. It's probably not worth breaking the existing folks relying on this behavior, but I would consider doing any sort of unhooking of nodes from your Yoga tree, before freeing parts of it. Probably not going to happen any time soon, but I have had some desire to add an intrinsic ref-count to the nodes, and deprecate `YGNodeFree` for something like `YGNodeRelease`. I think it could be done in a backwards-compatible way.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1659