Unrequire ComputedNode

[ickshonpe]

Objective

UI nodes with Display::None set are meant to be removed from the layout and ignored but because all we do to signal removal is set the size to zero the inactive nodes get caught up in all the UI queries which is catastrophically terrible for performance and causes a few bugs.

Fixes #16904

Solution

Remove ComputedNode from Node's required components and instead add or remove it automatically in ui_stack_system during the walk to update the stack indices depending on the state of the Node::display field.

I bashed this out really quickly and it's a bit rough. I expect there to be some bugs and some things need a clean up. I'm convinced the idea is sound though and it's ready enough to test and review.

Testing

cargo run --example many_buttons --release --features trace-tracy -- --display-none

---

Migration Guide

ComputedNode is no longer a required component for Node, instead it's added or removed automatically by ui_stack_system.

[alice-i-cecile]

Oh good catch. @pcwalton this might help explain your weird perf issues with UI layout.

After some chewing, I like this approach better than adding some disabling marker: it doesn't make sense for us to keep the data around due to how long-lived Display::None states are. It also prevents weird footguns where you're checking stale derived data.

[ickshonpe]

Oh good catch. @pcwalton this might help explain your weird perf issues with UI layout.

I think his issue is because the stack indices, clipping rects and border values are alway recomputed every frame regardless of changes to the layout. It used to not be a problem but recent changes, especially ghost nodes, have made those updates a lot more expensive.

After some chewing, I like this approach better than adding some disabling marker: it doesn't make sense for us to keep the data around due to how long-lived Display::None states are. It also prevents weird footguns where you're checking stale derived data.

Yeah lots of query filters can be confusing, removal is definitely better imo.

[UkoeHB]

UiSystem::Prepare was explicitly added to be the first ui system set. That way you can reliably do .before(UiSystem::Prepare) and your systems will never mess up ui calculations.

It looks like you're using the results of UiSystem::Stack in systems in Prepare now. I think you need to do a larger rearranging of systems to accommodate this so Prepare remains the first set.

[cart]

Imo the alternative to consider is something like ComputedNode::Enabled { /* computed state */ } and ComputedNode::Disabled, or a field like ComputedNode::enabled. This has the benefit of not moving things around in the ECS as "normal" part of the flow, and allows us to spawn node trees without immediately doing an archetype move. The downside is you then need to check the state in the internals, but this feels pretty reasonable to me (and making it an enum variant would force that). I dont think ComputedNode is so big that we need to optimize it out whenever something isn't being displayed, especially when we (1) factor in the cost of archetype moves when switching and (2) consider that disabled nodes will essentially always be enabled at some point.

Ultimately I think adding enabled/disabled "runtime" states to components in Bevy ECS would be a nice generic solve to this problem. Lets us keep things in memory while also "hiding" components from systems. But in lieu of that feature, I'm kind of biased toward leaving ComputedNode as required and eating the complexity of checking the enabled flags in the few internal places where that matters.

[UkoeHB]

Suggested change

	if node.display == Display::None {
	commands.entity(id).remove::<ComputedNode>();
	}
	if visited_root_nodes.contains(&id) {
	continue;
	}
	if visited_root_nodes.contains(&id) {
	continue;
	}
	if node.display == Display::None {
	commands.entity(id).remove::<ComputedNode>();
	}

[UkoeHB]

This behavior is not the same as the zindex_global_node_query case below (which looks correct)

[UkoeHB]

Suggested change

	if node_query.get(node_entity).unwrap().display == Display::None {
	remove_computed_nodes_recursive(node_entity, commands, ui_children);
	}
	if node_query.get(node_entity).unwrap().display == Display::None {
	remove_computed_nodes_recursive(node_entity, commands, ui_children);
	return;
	}