Add UI GhostNode
#15341
Add UI GhostNode
#15341
Conversation
alice-i-cecile
added
C-Feature
|
I'm going to patch this into bevy_reactor and see if I can get it working. |
|
OK, I managed to come up with a bare bones version of bevy_reactor that uses this, but I had to disable a ton of stuff because it's incompatible with changes in 0.15-dev (which has nothing to do with this PR). What I am seeing is that everything looks good on the initial render; however if I replace the ghost node's children, I no longer see those children. Unfortunately, I can't 100% guarantee that this is not a problem in my code; I can't run the EGUI world inspector in Bevy 0.15, so I have to make do with printf debugging of the entity tree. From what I can tell, If you want to try it out yourself, the code is here: https://github.com/viridia/bevy_reactor - note that only the 'simple2' example compiles, everything else is broken. |
|
Overall, the code looks pretty good. I see the TODO comment about the change detection, which aligns with my intuitions about the root cause of the problem I am seeing. I suspect that when my conditional node, which is a ghost node, deletes and re-creates its children, it's not triggering a layout pass on the parent of the ghost node because the change detection isn't propagating upward. |
Thank you for setting this up! Very good to have a real-world scenario to test with 👍
This is to be expected currently, as the layout system implementation does not yet do change detection on nested children. |
|
OK, looks like everything in my simple example is working now - this demonstrates |
alice-i-cecile
added
X-Blessed
|
I'll re-post some thoughts here that I wrote on Discord:
|
villor
force-pushed
the
ui-ghost-nodes
branch
from
179c2e1 to
a0bc2ef
Compare
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
|
@UkoeHB Thank you for the review! 😍 |
alice-i-cecile
added
S-Ready-For-Final-Review
|
Late to the party again, but I would like to point out that the majority of users would not want to pay the performance hit (over 2x!) and complexity hit (because now everybody needs to work around the possible ghost nodes) for a feature they will never use. It also splits hierarchy traversal in two distinct versions for "normal" and UI use. Bevy UI is difficult enough as-is for users. This should be feature gated at the very least, though it still requires all UI crate makers to rewrite all their systems that rely on tree traversal. Which will be a lot. On another note, I think mixing the "ghost" and regular UI nodes is a terrible approach as it mixes "virtual" configs with actual configs for UI, increasing complexity in both the management, extraction, and introspection side. Normally reactive DOMs are entirely separate from their render versions, much like how we have a main world and a render world in Bevy. It would make more sense to put them in a separate subapp and sync their diff to the main world (maybe even two world so that a world-world diff pre-filter could be implemented). To reason is that the "logical" view might have no hierarchical correspondence with the rendered one. Think about re-using fragments: The logical tree would have only the fragment link in the hierarchy, while the template is elsewhere, perhaps synced on it's own. Another reason would be that the ghost approach is not resilient against external modifications, while having only the logical tree is. While this may not seem a worry, all library makers will have a ton of issues opened 'cus someone wanted to "just update the text" or something. |
I agree that the performance hit is a bit much at its current state. But I'm confident this can be optimized so that there is no significant performance hit.
For someone who doesn't use ghost nodes this PR will not change anything. If you don't care about ghost nodes, you can continue using However, I do agree that this is not the most optimal approach. There is still design work to be done to reach a nice UI hierarchy abstraction that fits all use cases while also being easy to grasp and use. We have had some discussions about this on Discord. But a GitHub discussion for this would be good so we can all reach a consensus on the right path.
To me ghost nodes are part of the interface here (just like Mesh and Material are interfaces to the separate render hierarchy), something to use not just for reactive libraries, but other use cases as well. While I could see a separate retained hierarchy for the UI nodes kept as an optimization (along with a well documented interface to query it), I think the main ECS hierarchy should allow mixing UI and non UI entities within the hierarchy, as well as starting new UI hierarchies and terminating them, allowing more ergonomic world space UI and UI space 2D/3D entities. |
But this is the problem. All library authors must make sure they support the ghost nodes since they cannot be sure users won't use their widgets in ghost contexts, while all users must make sure they consider them since libraries may use it beyond just supporting it. And I am saying this as a library author, spending weeks on night shift refactoring just to support 3 ppl makes me want to stop entirely.
Except that they are not components that sit on their target and do something in a different world, which makes them no longer be "configs". Anyway I am just a user not a maintainer so I leave it to them, but I doubt this will bring any benefit for the masses (but will bring a lot of headaches). |
I have implemented both approaches now (reactive nodes as a separate tree, and combined in the same tree). I can report that using a single tree resulted in a huge reduction in overall complexity of the ui library code. This doc describes in detail my experiences migrating my UI framework to ghost nodes. Normal Bevy users may not ever use ghost nodes, but I suspect that a number of popular crates will incorporate them. There is a very good chance that BSN is going to incorporate reactivity using ghost nodes, so putting it behind a feature flag would require putting BSN behind a feature flag, which I don't think is going to happen. |
|
I'm in the process of reviewing @viridia's reactive system (based on ghost nodes). I don't want to make a final call on this until I know my feelings about that impl and how it uses ghost nodes. Storing this data "in the main world" and attaching it directly to entities has really driven down complexity in bevy_reactor. I've been really stoked on what I've seen so far (and how nicely it appears to fit into the BSN model). I want to give this a fair shake. If we find pain points in the impl that feel like deal breakers, or the value doesn't seem to outweigh the added complexity, we can revert this. Given that 3rd party library authors can choose to ignore supporting ghost nodes (ex: to my knowledge Sickle UI would have continued working as-is without ghost nodes, other than potentially behaving unexpectedly if a user tries to use them), I don't see the big deal here, especially in the short term while we're evaluating. I do consider this to still be an experiment. Doing it in-tree made @viridia's life easier. I think in general 3rd party UI authors should just ignore it for now while we evaluate, unless they also want to experiment. If supporting GhostNodes isn't currently opt-in at the moment for 3rd party devs, please say so and we can try to sort something out. |
But I do. I am not going to claim the crate works with vanilla Bevy if I know full well the moment someone tries to use a BSN it will blow up. And why wouldn't they want to use a BSN the moment it lands? Do you expect me to patch it for the next release knowing it will be obsolete in a month? I am eager to see how you would implement docking zones, tabs that can be torn out to floating panels, floating panels with docking zones that you can dock in another docking zone, docking zone splits, etc. I am also eager to see how you would support arbitrary adding, removing, or reordering children across an unknown number of ghost nodes. Hint: You won't. Ghost nodes implicitly remove support for manipulating regular nodes directly. As such everyone is forced to wrap every tiny thing in a ghost node and manipulate the ghost tree as is, going the full circle just with compound complexity. You also loose direct access to measurements on these ghost nodes, so every time you change the ghost tree, a regular parent lookup becomes necessary if you care about scaling things down the tree (like you would if you add / remove a bunch of children that are dynamically scaled). A frame delayed at that. Text and 3D will have the same issues once people start trying to use it. There are more reasons listed in the discord but I understand the community agrees with this so I will stop at this post. As for As a closing thought: I doubt you'll get feedback from 3rd party crate authors since Quill is already on board even though it worked without, and sickle is out since it cannot work within. There are no others, or at least not deep enough to mind. |
|
I'm not ready to respond to each of your individual points, although I am interested in the answers and the objections you raise. However, I do want to clarify some things, just to insure that we're not arguing past one another: First, while Quill does not currently use ghost nodes, but it might in the future - Quill currently has a number of subtle footguns which ghost nodes may or may not solve. Right now ghost nodes are used in Fine-grained systems like bevy_reactor tend to have a large number of micro-reactions, and managing these as a separate tree creates a lot of complexity as well as issues around closure lifetimes and variable capture. (Credit where credit is due: it was the comment by Sander on the BSN proposal that got me interested in working towards a single-tree solution.) The BSN documents have stated from the beginning that reactivity would be a part of the solution, but not a lot of details were given. Part of the motivation for bevy_reactor was to try and do experiments to fill in those missing gaps. But these were always intended to be experiments, I never pushed a crate and never tried to get a user base or persuade people to use it. What this means is that frameworks that want to interoperate with BSN were always going to have to accommodate reactivity in some form; the only alternative would be to lobby to drop the reactivity requirement, which means coming up with a convincing and viable alternative strategy. The good news (for you) is that you are one of the few people who can make such an argument and be taken seriously, because you have a framework that operates at scale, which most UI frameworks don't. (Vanilla bevy_ui doesn't operate at scale: while it's easy to spawn a complex UI, keeping it up to date and dynamic quickly bogs down into a morass of complexity. But a lot of people seem to think that building a better UI toolkit is about making spawning easier, when spawning is not the problem.) I always thought that there would be room for multiple UI frameworks with different APIs and different authors, because the choice of UI framework is a personal decision that involves aesthetic as well as technical considerations. But I think that it's important that these frameworks are able to co-exist at some level. At the very least, one can imagine an app that is based on sickle_ui, but uses a plugin world inspector popup that is implemented in BSN. Or perhaps the other way around. The other thing I would say is that so far, I have found working with ghost nodes to be pretty painless, however that may be because I rely entirely on bevy_ui for all layout calculations (well, except for floating popups), and ghost nodes only impact layout - in every other respect they are like normal nodes. I suppose someone who was doing their own layout traversal might have problems; but I think that can be solved. |
I expect nothing from you and I would like to support what you're doing in any way that I can, while still making forward progress on upstream Bevy UI / Scene improvements.
From my perspective we are still in the process of exploring and evaluating the bounds of what is possible with ghost nodes. I'm just this week diving back in to this space after sadly being sidetracked for awhile working on other things. It is very possible that your assessment of the situation is correct. I just sadly haven't fully wrapped my head around the space yet enough to convince myself that these scenarios aren't possible. I own (and apologize for) the fact that this probably should not have been merged in its current form, especially without more messaging around the state of the effort, our expectations around it, and strong urgings for people to not adopt it yet. I'm sorry that this came out of nowhere, I'm sorry this has derailed your work, and I'm sorry you feel pressured to drop everything and support it.
I'm not sure we have consensus on "ghost nodes" as a foundational UI piece yet. Needs more investigation / proof-of-viability first.
If anything we build erases the work you've done over the past year, thats worth discussing. I can't promise we won't break you (or other 3rd party Bevy UI frameworks), but as much as possible I would like to ensure there are easy paths to bring things forward.
First: the value and desirability of your work is clear. That being said, given the-in-flux state of "upstream Bevy UI" and the promise (for years) that things would change, I hope it isn't too surprising that the things being built on the framework as it exists today may need to rethink some of their assumptions if they want to work in the new system.
Yup this makes sense to me. |
|
The main alternative to The over-arching use cases for this hierarchy complexity are:
A single complex hierarchy is one solution to the above problems but another approach may also work. |
|
To be clear: I suggested using Owner/Owned only for cases where the ghost nodes are leaves, not in cases where they are interior nodes. Owner/Owned does not solve the latter use case. One might ask: why use ghost nodes on leaves in the first place? First, some context: a significant overall trend in Bevy development can be summed up as "...as entities". That is, we are increasingly moving towards a world in which visible "nodes" have a halo of associated, invisible satellite entities, for a variety of housekeeping purposes: observation, callbacks, state management and many others. Unfortunately, the But ghost nodes aren't the only way to do this, and not necessarily the most efficient. The layout system still has to iterate over these entities, even if they are ignored. And unfortunately, ghost nodes also incur the overhead of traversing into an (empty) child tree. Using a separate relationship for satellite leaf nodes, such as Owner/Owned, in parallel with Parent/Child, would solve the use case in a more efficient way, because the layout system would never see the satellite entities at all. Ghost nodes still have value for their primary use case, which is acting as a proxy/placeholder for dynamically-generated children. (In fact, I realized just now that "ProxyNode" would also have been a valid name). As stated above, I have explored many permutations of this, including parallel trees, storing the data in components, and so on. The ghost node solution is the simplest and cleanest approach I have been able to come up with. That doesn't mean it's the best solution possible :) In particular, one advantage offered by ghost nodes that none of the other solutions I've looked at do is that it allows and even encourages non-monolithic designs. Here's what I mean: when children are dynamically generated in something like a ForEach loop or an If-condition, they need to be properly inserted into their parent node, and ordered properly with respect to their non-dynamic siblings. This has to work even if the dynamically-generated children are the empty set, or if the generated children of the previous generation were the empty set. This requires that the parent be aware of what's going on dynamically with its children, to be notified when changes take place, and to have metadata that allows it to order things correctly. This gets further complicated by the fact that the hierarchy levels of the display tree and the hierarchy levels of template execution might not be a 1:1 mapping: it's common in the React world, for example, to have templates that do nothing but call other templates, sometimes for several levels (these intermediate templates do, however, add value in the form of additional state, effects, callbacks and so on). So you might have a case where the "parent" of a display node doesn't directly correspond to the "parent" of a template node or VDOM node. This means that the repositioning of dynamic children has to search upward in the hierarchy until they can find some parent responsible for deciding where the children should go. With ghost nodes, all of that complexity lives in the layout extraction traversal, hidden from the average user. This means that each ghost node and its children are context-free - they don't care about their parent and never need to communicate with it. This decoupling allows for a non-monolithic approach to UI construction, such that the methods used to construct a parent node doesn't have to match or coordinate the methods used to construct its children - you can mix and match. |
|
@viridia nothing can justify breaking the guarantee that the node tree is the single source of truth that will end up being rendered. Especially not ghost nodes that do nothing. Your obsession with java script is commendable but bevy already rendered anything you changed in the tree and there are observers now to achieve data binding outside of systems. The reasons react and reactive systems exist do not apply to Bevy. Never did. It is simply insane to litter the tree with unknown-purpose gaps. Except for the sole purpose of supporting a fully authoritative top down management framework... where have I seen that I wonder? In any case, there is no reason to continue this discussion, I won't stand in the way of bevy's evolution, good or bad. There is always a way forward regardless of current state and it isn't in my authority to influence how much users need to accept or adopt. Nor am I one to provide a supporting framework anymore. I have already pulled Also, I've already left the discord once because nobody can really say anything over you shouting wall of texts and only popped back to announce the deprecation. I have left again and have no intention (or reason) to return after this. I would be glad you are on your way to achieve your dream game if it didn't involve destroying others'. I offer no apology for the personal outburst, nor for the content of it. I held it as long as humanely possible, tried to find workarounds, but push came to shove. Cheers. |
|
@cart, I won't accept your apology since you have nothing to apologize for. I was prepared to rewrite the entire bevy_ui & text for my own purposes should the need arise (upset about it, yes, but prepared). In fact I made a PoC for the text over a month ago. |
# Objective - Fixes bevyengine#14826 - For context, see bevyengine#15238 ## Solution Add a `GhostNode` component to `bevy_ui` and update all the relevant systems to use it to traverse for UI children. - [x] `ghost_hierarchy` module - [x] Add `GhostNode` - [x] Add `UiRootNodes` system param for iterating (ghost-aware) UI root nodes - [x] Add `UiChildren` system param for iterating (ghost-aware) UI children - [x] Update `layout::ui_layout_system` - [x] Use ghost-aware root nodes for camera updates - [x] Update and remove children in taffy - [x] Initial spawn - [x] Detect changes on nested UI children - [x] Use ghost-aware children traversal in `update_uinode_geometry_recursive` - [x] Update the rest of the UI systems to use the ghost hierarchy - [x] `stack::ui_stack_system` - [x] `update::` - [x] `update_clipping_system` - [x] `update_target_camera_system` - [x] `accessibility::calc_name` ## Testing - [x] Added a new example `ghost_nodes` that can be used as a testbed. - [x] Added unit tests for _some_ of the traversal utilities in `ghost_hierarchy` - [x] Ensure this fulfills the needs for currently known use cases - [x] Reactivity libraries (test with `bevy_reactor`) - [ ] Text spans (mentioned by koe [on discord](https://discord.com/channels/691052431525675048/1285371432460881991/1285377442998915246)) --- ## Performance [See comment below](bevyengine#15341 (comment)) ## Migration guide Any code that previously relied on `Parent`/`Children` to iterate UI children may now want to use `bevy_ui::UiChildren` to ensure ghost nodes are skipped, and their first descendant Nodes included. UI root nodes may now be children of ghost nodes, which means `Without<Parent>` might not query all root nodes. Use `bevy_ui::UiRootNodes` where needed to iterate root nodes instead. ## Potential future work - Benchmarking/optimizations of hierarchies containing lots of ghost nodes - Further exploration of UI hierarchies and markers for root nodes/leaf nodes to create better ergonomics for things like `UiLayer` (world-space ui) --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
|
@eidloi All I can say is that I am sad to see |
|
I'm locking this thread as it has become too contentious. On the topic of Ghost Nodes, the final state of things is "they are still experimental and we are not yet committed to adopting them". The maintainers and I are discussing whether to revert or to adjust how we expose them to avoid people taking dependencies on them (or feeling pressure to adapt to them) before we're actually committed. @eidloi, you have violated our code of conduct (as I am sure you are aware). I'm giving you a temporary week long block to let you cool off. Please don't let it happen again. |
|
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1710 if you'd like to help out. |
alice-i-cecile
removed
the
M-Needs-Release-Note
Objective
Solution
Add a
GhostNodecomponent tobevy_uiand update all the relevant systems to use it to traverse for UI children.ghost_hierarchymoduleGhostNodeUiRootNodessystem param for iterating (ghost-aware) UI root nodesUiChildrensystem param for iterating (ghost-aware) UI childrenlayout::ui_layout_systemupdate_uinode_geometry_recursivestack::ui_stack_systemupdate::update_clipping_systemupdate_target_camera_systemaccessibility::calc_nameTesting
ghost_nodesthat can be used as a testbed.ghost_hierarchybevy_reactor)Performance
See comment below
Migration guide
Any code that previously relied on
Parent/Childrento iterate UI children may now want to usebevy_ui::UiChildrento ensure ghost nodes are skipped, and their first descendant Nodes included.UI root nodes may now be children of ghost nodes, which means
Without<Parent>might not query all root nodes. Usebevy_ui::UiRootNodeswhere needed to iterate root nodes instead.Potential future work
UiLayer(world-space ui)