Allow multiple root UI Node

[Davier]

All the UI entities with Node that are at the root of their hierarchy (i.e. have no Parent component) will stretch on the whole window instead of sharing it, allowing them to act as layers.

[cart]

I like this idea. I think before merging we'd want to have an answer to the z-layer problem. We can't use "entity iteration order" to determine layering here because that is undefined (it starts as being entity insertion order, but as soon as archetypes change or entities are removed this breaks down).

It should also be possible to change z-ordering at runtime.

[Davier]

I agree that a z ordering is needed, that's the reason I changed the PR back to a draft.
I was thinking about the design for that:

Do we want to specify z order for any node or only root nodes?
Ordering all non root node with the current system seems fine to me, but might restrict some use cases.

If we can specify z order for any node, do we want to use offsets or layers? (relative or absolute z)

• layers would be better for complex scene with multple roots
• offsets would be better to order relative to siblings
• both are equivalent for root nodes, which would be the main use for it

Should the offset be a new field of Style or a separate component?

• if we can specify z order for any node, it could belong in Style in an Option (but would be unused for most entities)
• if we only specify order for root nodes, a separate component seems better. Besides, I would like to allow ui roots a with Parent that is not a Node (e.g. inside a loaded scene), which would probably be implemented with a component that is automatically managed. That component could hold a non optional offset.

My current opinion is to only have an offset for root nodes. The new commit implements that with a UiRoot component (and addresses your other comments), but I am not very satisfied by that name .

[cart]

Do we want to specify z order for any node or only root nodes?
Ordering all non root node with the current system seems fine to me, but might restrict some use cases.

We need to ensure that layers never "intersect" with each other. Ex: if layer1 has a "stacked z-size" of 10, layer2 should start at 11 (10 and 11 use arbitrary units ... in practice it would be a floating point value)

If we can specify z order for any node, do we want to use offsets or layers? (relative or absolute z)

I think it probably makes sense to make it relative. Maybe we can do something similar to CSS: https://developer.mozilla.org/en-US/docs/Web/CSS/z-index.

Should the offset be a new field of Style or a separate component?

I pretty strongly feel that it should be a part of Style (defaulting to a None or Auto value). I don't like requiring a separate component for roots because that is "redundant information". The fact that something is a root is already encoded by the hierarchy itself. Additionally, it allows the "root component" to be added to non-root nodes (or even non-root non-node entities), which is a bit weird imo. Additionally, it requires additional manual action from the user to define a root when constructing the hierarchy.

[Davier]

We need to ensure that layers never "intersect" with each other. Ex: if layer1 has a "stacked z-size" of 10, layer2 should start at 11 (10 and 11 use arbitrary units ... in practice it would be a floating point value)

I think it probably makes sense to make it relative. Maybe we can do something similar to CSS: https://developer.mozilla.org/en-US/docs/Web/CSS/z-index.

Good idea!

I pretty strongly feel that it should be a part of Style (defaulting to a None or Auto value).

I agree with you now.

I don't like requiring a separate component for roots because that is "redundant information". The fact that something is a root is already encoded by the hierarchy itself.

In my WIP rewrite, I store a WindowId in it, so it does have extra information. We would need to store that information somewhere anyway to support multiple windows, and it makes sense to me to have it there.

Additionally, it requires additional manual action from the user to define a root when constructing the hierarchy.

I automatically add that component when necessary (with the primary window's id), so users only need to add or modify it when they deal with several windows.
The only breaking change of this PR should be that all root UI entities do not share the screen.

Additionally, it allows the "root component" to be added to non-root nodes (or even non-root non-node entities), which is a bit weird imo.

Allowing "UI root" entities that are not a global root is IMO an attractive feature. I would like for instance to embed UI trees under the root entity of a scene, and automatically load/store them. I'm not completely sure about that, but I'd like to test it first, even if you want me to remove it later.
As for non-root non-node entities, I agree that it's weird. For now I could either error! when it happens or just accept it as a separate UI tree in a weird place. If the user explicitly adds that component in that position, it's probably what they want.

I expect to push something by tomorrow for feedback, but I suspect it will need extensive testing. Do you have any suggestion of things to test in addition to the ui examples?

[cart]

In my WIP rewrite, I store a WindowId in it, so it does have extra information. We would need to store that information somewhere anyway to support multiple windows, and it makes sense to me to have it there.

Hmm yeah the "where should we put WindowId" discussion should also be had. That is root-specific information. We could consider just putting it in Style for convenience (even though it would only ever apply to root nodes). Or we could consider adding a new NodeWindowId component (probably optional and defaulting to the primary window when it isn't defined)

I automatically add that component when necessary (with the primary window's id), so users only need to add or modify it when they deal with several windows.

I want to avoid automatically adding components on the user's behalf whenever possible:

1. Components should generally be a users assertion of what the World should look like. Adding components on a users' behalf is "implicit behavior".
2. Adding a component at runtime changes an entity's archetype, which has a non-zero cost (its not terrible, but its also worth keeping the number of archetype changes low).

Some cases are basically unavoidable (such as Parent/Child components), but in this case I think just defaulting to the primary window is the right call.

Allowing "UI root" entities that are not a global root is IMO an attractive feature

I think I agree with that, but I still think that should be "behavior implicit to the hierarchy". If we want to support non-global roots, I think it should work by nature of a Node being added beneath some non-Node root. Adding the component is still redundant information / boilerplate.

Do you have any suggestion of things to test in addition to the ui examples?

We we regrettably don't have any built in examples/tests more complex than the ui.rs and button.rs example. It would be nice to eventually have a few "example apps" with non-trivial ui to test on. The Bevy Editor will eventually fill that niche nicely. It might be worth asking around on discord for examples of complex ui / browsing awesome-bevy.

[Davier]

Most steps of the layout system are now incremental.
The exceptions are the calculation of the layout by stretch (but it has its own cache), and the linear iteration of all nodes to update their x & y translation.
We could add a "dirty" flag to know if these steps should be skipped, but I don't know if it's useful.
I pretty much rewrote the entire file, I hope it's OK with you. Let me know if the style/naming is alright.
This commit works for me, but I still need to extensively test it.

There are some other new features:

• allow mixed hierarchies of entities with and without Node component
• add NodeWindowId component for root UI entities to select their window
• add z_index property to Style component
• remove dead stretch nodes (but it's deactivated for now, Stretch::remove seems to have a bug)

Additional thoughts:

• we could change NodeWindowId to be an Option, None indicating that the root node should not be resized. That would be needed to output the UI to textures (?)
• we could separate z-index in its own component instead of being in Style, allowing alternative/external UI systems to hook in (and also allowing us to filter separately when updating). This was suggested by @TheRawMeatball .
• parent_update_system is run in POST_UPDATE, which is after UI; that has already caused issues in discord.
• when the new scheduler is merged, flex::layout_system should have a hard dependency on widget::image_node_system & widget::text_system
• the z_index property is implemented by modifying the local transform's scale.z. Is that OK?

[TheRawMeatball]

I think I agree with that, but I still think that should be "behavior implicit to the hierarchy". If we want to support non-global roots, I think it should work by nature of a Node being added beneath some non-Node root. Adding the component is still redundant information / boilerplate.

Not sure if this is a case we want to support, but a node might want to assert that it is a root node regardless of its position in the hierarchy. For example, a "popup button" component might be getting spawned under a UI node, but it might spawn a popup as a root node.

[cart]

In general I'm loving these changes. Much more flexible and much easier to read.

[cart]

This does a ton of redunant work in terms of allocations, traversals, sorting, etc (especially on startup). I think it makes sense to add some tracking here to cut down on the combinatorics.

Ex: On startup every single node:

1. walks up the tree to (worst case) the root O(logn)
2. Allocates/populates a stacking context with every node (worst case the whole tree, which is O(n))
3. Sorts the stacking context O(nlogn)
4. Sets the transform z value of each node in the stacking context (which is O(n))

By my calculations thats worst case O(n^2logn) compute AND allocations, which is not fantastic

[Davier]

Ex: On startup every single node:

1. walks up the tree to (worst case) the root O(logn)

This is a real concern but I'd like to point out that it only does that work for their stacking context, which by default and in most cases (ZIndex::None or ZIndex::Some(i)) means their children. The worst case happens if the user explicitly selects ZIndex::Auto in every single node. In practice I only expect that feature to be used inside the small hierarchy of a widget for instance.

3. Sorts the stacking context O(nlogn)

In most cases (ZIndex::None and ZIndex::Auto), the stacking context should already be sorted at creation (I need to test that). The sorting only has to move nodes with ZIndex::Some(i).

Here is a list of potential improvements:

• use a smallvec so that most cases without ZIndex::Auto do not allocate
• skip sorting if there is no ZIndex::Some(i)
• cache the stacking context (maybe only when ZIndex::Auto is involved?)
• use an iterator to add all children at once instead of pushing one by one
• as discussed, making ZIndex a separate component so that changes to other Style properties do not trigger this whole process. (Alternatively, ZIndex could have its own mutation flag?)

The part that I'm unsatisfied with is that the process is duplicated when several non leaf entities from the same stacking context are added/updated at once. However I'm being cautious because it would be unfortunate if an optimisation for that adds overhead for the default/common cases. I'll think about what kind of tracking can be done there.

Besides, I think there is a bug in this implementation for negative z-index, I'll have to check.

Last thought: is the ZIndex::Auto feature worth adding all that complexity?

[Davier]

The new commit only allocates stacking contexts once, and makes sure they are updated at most once every frame, when required.
However the z transforms are now all updated if any stacking context changed, since using the transforms's scale for incremental update was causing issues with f32 precision.

[cart]

This impl breaks scale factor support. We'll need to fix that before merging.

[Davier]

I forgot to mention it, sorry.
I'm not familiar with this topic, is it required to round the translations at all?
If it is we would need to cache the scale factor for every node.
If it's not we could just disable the final rounding in stretch and do all layout calculations in logical pixels, which is very simple.

[cart]

Yeah I think ideally we would use logical pixels to keep the code simple, but I do think stretch rounds on our behalf, which would break things when we try to scale back up via the scale factor. We could either try to fix that in stretch or revert to the "physical pixel" approach used on "bevy master".

[Davier]

I'd like to fix it in stretch, but it seems the maintainers are not responsive. How would you like to handle that?

[cart]

we could change NodeWindowId to be an Option, None indicating that the root node should not be resized. That would be needed to output the UI to textures (?)

Yeah thats a good idea. Lets hold off for now and when the "UI to Texture" feature is built we can re-visit. No sense in building a feature when we aren't sure its needed yet.

we could separate z-index in its own component instead of being in Style, allowing alternative/external UI systems to hook in (and also allowing us to filter separately when updating). This was suggested by @TheRawMeatball .

Hmm thats worth considering, but I think I'd prefer to break it out if/when theres a demand for it. Taking on the "interoperable ui" problem introduces a whole new set of problems that I don't think we've discussed enough to understand the scope of.

parent_update_system is run in POST_UPDATE, which is after UI; that has already caused issues in discord.

Yeah lets revisit this after the hierarchy rework.

the z_index property is implemented by modifying the local transform's scale.z. Is that OK?

I think this is probably ok, but we should probably make sure it doesn't produce precision issues for deeply nested ui.

[Davier]

I just realized that the current z-index implementation don't actually solve the layering of root nodes, which is what I wanted from it in the first place. I need to put them in their own stacking context.

the z_index property is implemented by modifying the local transform's scale.z. Is that OK?

I think this is probably ok, but we should probably make sure it doesn't produce precision issues for deeply nested ui.

The minimum precision between 0..1 is about 5.96e-8 near 1. If we take the simple case of each node having one child, we would hit that limit in -ln(5.96e-8)/ln(2) = 24 nested scopes. That's not as deep as I expected, but makes sense in hindsight since that's the size of the mantissa of f32, and each scope will be half the size of its parent.
Having N nodes in a scope increases the size as log2(N), so one layer with 128 nodes takes "as much space" as 7 nested one-child-scopes.
We can probably use a logarithmic scale instead of a linear one to improve the precision, but I'll have to think about that, and the theoretical limit would still be 32 nested scopes.
However, if anyone has that issue, they can fix it by using ZIndex::Auto in a few places to reduce the depth and number of scopes.

If that's not good enough, we would have to switch back to updating all components at every change. It will be worth checking the performance difference between the two in large applications when they exist.

[cart]

Yeah if 32 is the limit, I have a feeling someone will hit that eventually. We should support "near infinite" nesting (in theory, if not in practice, for perf reasons).

[Davier]

After testing it, I ran into that issue even earlier than that.
I'm re-implementing z indices with an algorithm closer to the original one (always recalculate the transform of every node, all nodes are stacked at a fixed step).
It works but is more complex and a bit ugly. I will clean it up and push soon.

[Davier]

Here is the reimplementation of the z-index property without using the transforms' scale. The z-indices for window nodes and negative z indices are fixed, and I added a lot of tracking to avoid duplicating work. I added a Not<> QueryFilter for that, but it's a bit out of scope of this PR, tell me if you want me to split it out.

The window nodes' layering is undefined if their z-index is not set to Some(_). They will be on different layers, but which one is in front can change. That should probably be documented somewhere.

Not sure if this is a case we want to support, but a node might want to assert that it is a root node regardless of its position in the hierarchy. For example, a "popup button" component might be getting spawned under a UI node, but it might spawn a popup as a root node.

Since NodeWindowId is already specific to window nodes, it now allows users to explicitly mark window nodes (in the previous commit that component was just ignored on normal nodes, which people could find surprising).

I had to add a new stage for window_nodes_transform_system, since it must run after transform_propagate_system and before drawing. It should be possible to clean that up when the new scheduler lands? More generally, it might be better if the UI entities' transforms were not updated by transform_propagate_system, but I'm not sure.

[nicopap]

Mutliple UI roots is especially useful for 3rd party plugin providing UI functionalities like https://github.com/nicopap/bevy-debug-text-overlay/ . I guess the user could specify a node to which to parent the plugin's UI, but it will mess with the user's UI, which we do not want.

[Davier]

Mutliple UI roots is especially useful for 3rd party plugin providing UI functionalities like https://github.com/nicopap/bevy-debug-text-overlay/ . I guess the user could specify a node to which to parent the plugin's UI, but it will mess with the user's UI, which we do not want.

Agreed, my original use case was also a debug overlay using bevy_ui.
It's possible to use absolute positioning so that the rest of the UI is not messed, however the issue of z layers remains.

[oceantume]

@Davier @alice-i-cecile What's the status on this. Is there something actively blocking it? I imagine it may be out of sync with latest Bevy now, but is it worth updating or should a different solution be adopted?

I was thinking about this problem yesterday, before finding this PR, and had an idea for a flexible but not too complex solution. I'm wondering if I should try implementing it and make a different PR instead.

The idea is to allow setting either "local" and "global" z-index via an enum component, where local affects the render order of siblings and global affects render order of all UI nodes (e.g. to allow absolute nodes that are deep in the tree to render on top or under other things).

// This component should be optional. Either way, the component being absent or using
// ZIndex::Local(0) or ZIndex::Global(0) should mean the same thing and result in the same ordering.

/// Indicates that this node has special requirements for the order in which it should appear
/// in the UI instead of relying on where it appears in the nodes hierarchy tree.
[#derive(Component)]
enum ZIndex {
  /// Indicates the order in which this node should be rendered compared to its siblings.
  Local(i32),
  /// Indicates the order in which this node should be rendered -compared with all other
  /// nodes that also have a `ZIndex::Global(i32)` value.
  Global(i32),
}

[alice-i-cecile]

What's the status on this. Is there something actively blocking it? I imagine it may be out of sync with latest Bevy now, but is it worth updating or should a different solution be adopted?

This needs to be adopted; we have consensus that this work needs to be done. I'm not immediately sure on whether or not this specific approach is optimal.

[alice-i-cecile]

Closing in favor of #5892.