Filter UI traversal to only Node and GhostNode

[villor]

Objective

With the warning removed in #15736, the rules for the UI tree changes.
We no longer need to traverse non Node/GhostNode entities.

Solution

• Added a filter Or<(With<Node>, With<GhostNode>)> to the child traversal query so we don't unnecessarily traverse nodes that are not part of the UI tree (like text nodes).
• Also moved the warning for NoUI->UI entities so it is actually triggered (see comments)

Testing

• Ran unit tests (still passing)
• Ran the ghost_nodes and ui examples, still works and looks fine 👍
• Tested the warning by spawning a Node under an empty entity.

---

[UkoeHB]

This means the warning in UiSurface::get_layout won't fire for nodes with non-node parents (see #15736). I support this PR but we need a way to detect and warn for nodes with non-node parents (preferably one that isn't incidental like the current reliance on get_layout).

[villor]

This means the warning in UiSurface::get_layout won't fire for nodes with non-node parents (see #15736). I support this PR but we need a way to detect and warn for nodes with non-node parents (preferably one that isn't incidental like the current reliance on get_layout).

Right! Any ideas on how we could do that? A system just for the warning? 🤔

[cart]

This means the warning in UiSurface::get_layout won't fire for nodes with non-node parents

Ah yeah we definitely want to retain that warning. The wins here aren't worth dropping that.

[UkoeHB]

Looks like the warning actually never prints, since update_uinode_geometry_recursive early-outs if it hits an entity without Node.

One solution would be for the UiChildren iterator to return Has<Node> || Has<GhostNode> then in the for child_uinode in ui_children.iter_ui_children(entity) block in update_uinode_geometry_recursive, inspect any non-node entities to see if they have any descendants with Node or GhostNode and issue a warning. The warning in UiChildren::get_layout should be relocated since that function already returns an error.

[villor]

Looks like the warning actually never prints, since update_uinode_geometry_recursive early-outs if it hits an entity without Node.

One solution would be for the UiChildren iterator to return Has<Node> || Has<GhostNode> then in the for child_uinode in ui_children.iter_ui_children(entity) block in update_uinode_geometry_recursive, inspect any non-node entities to see if they have any descendants with Node or GhostNode and issue a warning. The warning in UiChildren::get_layout should be relocated since that function already returns an error.

Wouldn't that basically mean we still traverse all non-UI entities? To check the descendants.

Also the warning would still only show up for Nodes under non-UI entities somewhere below a valid UI tree. We would not catch Nodes added somewhere else, right?

I think for this warning to really work anywhere, it needs to be decoupled from the layout system completely. How about a system like this?

fn warn_ui_entity_in_non_ui_hierarchy(
    nodes_with_changed_parent: Query<(Entity, &Parent), (With<Node>, Changed<Parent>)>,
    ui_entities: Query<Entity, Or<(With<Node>, With<GhostNode>)>>,
) {
    for (entity, parent) in nodes_with_changed_parent.iter() {
        if !ui_entities.contains(parent.get()) {
            warn!(
                "Styled child ({entity}) in a non-UI entity hierarchy. You are using an entity \
with UI components as a child of an entity without UI components, your UI layout may be broken."
            );
        }
    }
}

[UkoeHB]

I think for this warning to really work anywhere, it needs to be decoupled from the layout system completely. How about a system like this?

Yeah this would catch orphaned sub-trees (non-UI -> UI without any preceding UI). Main downside is Changed<Parent> means we iterate all nodes every tick. It also doesn't catch the edge case of Node being removed from a parent.

[villor]

I think for this warning to really work anywhere, it needs to be decoupled from the layout system completely. How about a system like this?

Yeah this would catch orphaned sub-trees (non-UI -> UI without any preceding UI). Main downside is Changed<Parent> means we iterate all nodes every tick. It also doesn't catch the edge case of Node being removed from a parent.

Huh, I had no idea that’s how Changed worked. You learn something new every day 😅 could we use HierarchyEvent::ChildAdded/ChildMoved to avoid that?

Since a Node without parent is a valid root node, we shouldn’t really need to care about removals for this warning system.

[UkoeHB]

could we use HierarchyEvent::ChildAdded/ChildMoved to avoid that?

This seems viable, although now the perf issue is frames where you spawn a big bunch of entities in a hierarchy (like a scene, or a bunch of UI) will become even more expensive.

[villor]

Maybe we can include the warning system conditionally using #[cfg(debug_assertions)]. Then at least it won’t affect production perf.

Just have to decide between Changed or HierarchyEvent, or something else…

@alice-i-cecile @cart do you have any preference here?

[cart]

Maybe we can include the warning system conditionally using #[cfg(debug_assertions)]. Then at least it won’t affect production perf.

I like this idea.

Just have to decide between Changed or HierarchyEvent, or something else…

HierarchyEvent is a blunt-force tool, given that it is unfiltered. I personally think we should reconsider its existence entirely. Consuming that event is committing to iterating every hierarchy change across the entire world (and then checking if each change matches your criteria). Pretty much nobody should want to that. Producing those events feels likes a fixed cost that we shouldn't need to pay for most things.

Given that we're already iterating every UI node entity here, it feels like we might as well just insert our logic there (check if parent has changed, and if so, check if the parent has Node or GhostNode)

[villor]

Given that we're already iterating every UI node entity here, it feels like we might as well just insert our logic there (check if parent has changed, and if so, check if the parent has Node or GhostNode)

Done!

Tested with the following:

commands.spawn(()).with_children(|parent| {
    parent.spawn(NodeBundle::default());
});

Triggered just fine 👍

Decided not to do any #[cfg(debug_assertions)]-check since I figured there might be cases in the future where UI:s are created by someone running a release-mode binary (like an editor), and the warning might be helpful to them. We are iterating all the nodes either way so why not 🤷‍♂️

I think this is ready for reviews now.

[UkoeHB]

Just a new comment