Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI Node's Style changes are ignored for unparented nodes as long as the parent exists #11385

Closed
eidloi opened this issue Jan 17, 2024 · 1 comment · Fixed by #14490
Closed
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@eidloi
Copy link
Contributor

eidloi commented Jan 17, 2024

Bevy version

0.12.1

What you did

I made a floating panel (based on NodeBundle) that needs to be absolutely positioned to the whole window. The panel is first spawned as a child of another entity (to have access to the ChildBuilder needed to populate it's content), then I call remove_parent() on the panel (via commands) with a system in Update.

What went wrong

  • Changes to the Style component aren't reflected on the panel, i.e. left/top positioning and display properties are ignored. Except when the Window is resized.

Additional information

The Interaction component is updated as if the Style didn't change, so it seems the issue is around the calculation of the transforms or updates to the Node props.

The Style is correctly processed if the original parent node is despawned, so this works as expected:

let mut floating_panel_id = Entity::PLACEHOLDER;
commands
  .spawn_empty()
  .with_children(|parent| {
    // The FloatingPanel has a component that tells an `Update` system to remove the parent
    let (panel_id, mut container) = FloatingPanel::build(/*args*/);

    floating_panel_id = panel_id;
    container.with_children(|parent| {
        // populate content
    });
})
.despawn();

While this does not work:

let mut floating_panel_id = Entity::PLACEHOLDER;
// entity is a NodeBundle and a part of the working UI
commands.entity(entity).with_children(|parent| {
  let (panel_id, mut container) = FloatingPanel::build(/*args*/);

  floating_panel_id = panel_id;
  container.with_children(|parent| {
      // populate content
  });
});

Removing the parent:

// Necessary, as you cannot remove the parent immediately after spawning due to how the hierarchy systems work
fn remove_node_parent(
    q_to_remove: Query<Entity, Added<RemoveParent>>,
    mut commands: Commands,
) {
    for entity in &q_to_remove {
        commands
            .entity(entity)
            .remove_parent()
            .remove::<RemoveParent>();
    }
}

EDIT: After more trial and error, it seems that even if the panel is spawn directly without a parent the loss of updates can occur. Randomly, I would say. I had a working version, but then it stopped working after a few builds even though I did not touch this part. The key difference between when it is working and when it's not seems to be about when the panel is spawned. During Startup always seem to work. During Update, usually not.

@eidloi eidloi added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jan 17, 2024
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Jan 18, 2024
@eidloi
Copy link
Contributor Author

eidloi commented Jul 26, 2024

After more and more investigations, this is due to taffy storing "phantom" children that were removed during the layout update. The phantom children exist because the node children need to be updated before node removal to avoid a panic (Invalid SlotMap key). The solution is to sync the children again after the nodes were cleaned up.

github-merge-queue bot pushed a commit that referenced this issue Jul 30, 2024
# Objective

The `ui_layout_system` relies on change detection to sync parent-child
relation to taffy. The children need to by synced before node removal to
avoid trying to set deleted nodes as children (due to how the different
queries collect entities). This however may leave nodes that were
removed set as children to other nodes in special cases.

Fixes #11385

## Solution

The solution is simply to re-sync the changed children after the nodes
are removed.

## Testing

Tested with `sickle_ui` where docking zone highlights would end up
glitched when docking was done in a certain manner:
- run the `docking_zone_splits` example
- pop out a tab from the top
- dock the floating panel in the center right
- grab another tab and try to hover the original static docking zone:
the highlight is semi-stuck
- (NOTE: sometimes it worked even without the fix due to scheduling
order not producing the bugged query results)

After the fix, the issue is no longer present.

NOTE: The performance impact should be minimal, as the child sync relies
on change detection. The change detection was also the reason the parent
nodes remained "stuck" with the phantom children if no other update were
done to them.
mockersf pushed a commit that referenced this issue Aug 2, 2024
# Objective

The `ui_layout_system` relies on change detection to sync parent-child
relation to taffy. The children need to by synced before node removal to
avoid trying to set deleted nodes as children (due to how the different
queries collect entities). This however may leave nodes that were
removed set as children to other nodes in special cases.

Fixes #11385

## Solution

The solution is simply to re-sync the changed children after the nodes
are removed.

## Testing

Tested with `sickle_ui` where docking zone highlights would end up
glitched when docking was done in a certain manner:
- run the `docking_zone_splits` example
- pop out a tab from the top
- dock the floating panel in the center right
- grab another tab and try to hover the original static docking zone:
the highlight is semi-stuck
- (NOTE: sometimes it worked even without the fix due to scheduling
order not producing the bugged query results)

After the fix, the issue is no longer present.

NOTE: The performance impact should be minimal, as the child sync relies
on change detection. The change detection was also the reason the parent
nodes remained "stuck" with the phantom children if no other update were
done to them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants