Generic UI navigation system #41
Conversation
|
Implementation going on here: https://github.com/nicopap/ui-navigation |
|
This is super exciting, though I have concerns about having people manually build the navigation tree. I think all navigation info should be derived from the existing widget tree, at no extra cost to the developer. The way webpages handle this stuff should probably also be considered under prior art. Also btw, if you have a discord account it'd be great if came in and said hi at #ui-dev. I don't think Github is ideal for the fast-paced brainstorming etc. that happens early on in design. |
|
The user only has to add They only need to add I think it's necessary to specify the navigable nodes, because there is a lot of irrelevant-to-navigation nodes in the UI tree. Some nodes only exist to specify the layout, others are just pictures etc. |
rfcs/41-ui-navigation.md
Outdated
| `NavEvent::FocusChanged` or `NavEvent::Caught`. | ||
|
|
||
| The navigation system doesn't handle itself the state of the UI, but instead | ||
| emits the `FocusChanged` etc. events that the developer will need to read to, |
There was a problem hiding this comment.
IMO, this shouldn't be an event. Instead, toggle the value of Focused enum (like we have for buttons), and then let the developers use change detection for this.
There was a problem hiding this comment.
While for the current system this'd be the best option, in the context of a possibly larger reworking of UI adding a separate concept of "UI events" with propagation and using said events for this would probably be better. Out of this RFC s scope though.
There was a problem hiding this comment.
There is two reasons behind this:
- It's a way for me to completely hand-wave away everything but exactly the navigation algorithm
- It responds to that specific request from ndarilek
rfcs/41-ui-navigation.md
Outdated
| Let's go back to the menu. In the example code, I refer to `:container` and | ||
| `:navigable`. I've not explained yet what those are. Let's clear things up. | ||
|
|
||
| A _navigable_ is an element that can be focused. A _container_ is a node entity |
There was a problem hiding this comment.
I suspect we can just reuse the existing Interaction component, probably reflavoring to Focus. The venn diagram is a circle. so we can use the same component for both marking and storing the state.
While I agree with |
|
I thought a bit more about the design. A few things:
Tomorrow I'll add a roadmap for the implementation in the ui-navigation repo |
The difference between A case can be made that you should. This was part of the initial design, but as I came up with a more concrete proposal, it became less practical. In the implementation, the |
|
Updated plugin demo with reference and bevy-specific implementations. There is an example usage at flat_2d_nav.rs I'm surprised how the ECS version is basically just a series of function call. Although the flipside is that it's way more error-prone than the "generic" version which enforces the invariants at the API level. Next step is clarify this RFC. Afterward, I'll add Action/Cancel to the implementation which seems trivial. |
I'm very not sure whether we should block traversing out of a container node. A counter example might be a webpage or most other UI, where you can visit every focusable UI node by hitting tab. I don't think we should have hard boundaries. |
Nothing to worry about. In this example we explicitly use Maybe naming it |
Yes please, although probably |
tldr: * Get rid of concept of `Plane` * A lot of renaming: Navigable => Focusable, Container => NavFence * Put more emphasis on "single navigation path" limitations * Add a `Terminology` section * Improve illustrations to highlight the concept of `Active` * Elude UI specification from example * Use rust code for the algorithm * Remove plain-text English explanation of the algorithm
|
@alice-i-cecile told me to check this out. I got to say, I really like this design, and especially appreciate how |
|
I cooked up a new design! It solves basically all the unresolved questions. It's a relatively minor implementation change, but it's a major change in the way to think about the navigation graph, and it requires changing important parts of this RFC. I'm quite fed up editing graphs in Inkscape, but I think this is the last stretch and having a clear documentation on top of the implementation would be golden. Hopefully a good formal source of inspiration for implementing ui navigation in other UI projects too. |
|
I decided to copy parts of the bevy-ui-navigation API docs for this. Since those are well thought out. |
rfcs/41-ui-navigation.md
Outdated
| That proxy component is `MenuSeed`, | ||
| you can initialize it either with the `Name` or `Entity` of the parent focusable of the menu. | ||
| A system will take all `MenuSeed` components and replace them with a `TreeMenu`. |
There was a problem hiding this comment.
Note that the current implementation has simplified this. MenuSetting is now a component, and we have another MenuBuilder component that still gets converted by the navigation plugin into a TreeMenu.
Issue is I don't like "MenuBuilder" and wait to have a better name before updating the RFC.
| - _transitive_: If `A` is _foo_ of `B` (swap _foo_ with anything such as | ||
| _child_, _left_ etc.) | ||
| Then `A` is _transitive foo_ of `B` if there is any chain of `C`, `D`, | ||
| `E` etc. such as `A` is _foo_ `C`, `C` is _foo_ `D`, ... , `E` is _foo_ `B`. |
There was a problem hiding this comment.
I'm so sorry. I hope this is a good enough explanation for everyone :D
There was a problem hiding this comment.
This is a good and well thought out design! I like the decoupling of everything and I think that is important for a robust and flexible design.
I do think there should be a NavRequest::Next and NavRequest::Previous (or something similar) to facilitate behaviours like tab-cycling through a menu
- A user should be able to press next (tab) or previous (shift + tab) to bring focus to the different focusable elements within the menu
- A developer should be able to define the order the elements should be cycled through
- This could be completely unrelated to the geometric layout of the elements
- I'm pretty sure this is an important feature for general accessibility and screen-reader support
|
|
||
| `Focusable`s have a `prioritized` state that is set when they go from | ||
| active/focused to not active. | ||
| This is a form of memory that allows focus to go back to the last focused |
There was a problem hiding this comment.
I think we need the ability for the developer to define:
"When coming back to this menu, should we focus on the last-focused element or should we focus on the initial default element"
There are use cases in apps (filling in a new form) or games where we want to treat a menu as "first-time-user-visits" even though the user has visited before. The FFX battle system also springs to mind where you can choose between these two modes, which has a profound effect on muscle-memorisation.
There was a problem hiding this comment.
That's something I overlooked. Current design doesn't allow the user to modify the prioritized focusable. I think this can be enabled by providing a custom SystemParam.
rfcs/41-ui-navigation.md
Outdated
| Something that automates that could be a natural extension of the exposed API. | ||
| It may also help end users go with the best design first. | ||
|
|
||
| ### User upheld invariants |
There was a problem hiding this comment.
IMO there are much move severe invariants already in the engine, which to a some degree is needing to be upheld by the user (GlobalTransform + Transform for hierarchy to work comes to mind).
That in itself is not a good argument for adding more, but we are going to need a good solution for Archetype Invariants engine-wide anyway which this design would benefit from so I'm less nervous about this.
That being said, there is plenty of value is exploring alternatives but I don't think this should be that hard of a blocker in the name of getting some velocity on core UI features.
| The "tabs menu" is defined as a "scope menu", which means that | ||
| by default, the `LT` and `RT` gamepad buttons will navigate | ||
| the "tabs menu" regardless of the current focus position. |
There was a problem hiding this comment.
I'm guessing this is just for illustrative purposes, but we should aim to separate button inputs from action events, which would be out of scope for this RFC (and I suspect @alice-i-cecile's input manager is making a good job of that) but could this be example be rewritten to use something like NavRequest::Next and NavRequest::Previous (as suggested above)?
|
Imagine this skill tree. I'm suspecting that just using
Source: Star Wars Jedi: Fallen Order |
Strongly agree, will add that to the design. Though I need to think what's the best way to let users specify ordering without being a burden.
Yes. The default navigation option is very rudimentary and only cares about position, but you can disable it to replace with your own strategy ( |
|
@Weibye On second though, I think the tab cycling feature should wait. I'll add it to the "future" section. The PR is massive as it is, and I don't want to pile on functionalities. It's already going to be difficult to review as-is. There is a few features that I decided to delegate to future endeavor. What I will do is that if the ui navigation PR is merged, I'll immediately open an issue for each functionalities I left behind (and a tracking issue on top). This also allows more piecemeal PRs that are better for people to familiarize themselves with the ui-navigation code (I especially want the bus factor on ui-navigation to increase over 1 if possible) Is this fine for you? |
Yup, perfectly fine :) And this should probably be our preferred approach when dealing with large features like this.
Could you elaborate what you mean by this? |
|
I currently am the only person who really understands the code for ui navigation. I hope I wrote enough (internal) docs + this RFC for other people to be able to understand and contribute, but at lest for a short period I'll probably the only one confident enough to modify the navigation code. I want this to be as short a period as possible. |
| The design of this RFC _hinges on_ the whole tree been loaded in ECS while | ||
| executing navigation requests, and would work poorly if menus were spawned and | ||
| despawned dynamically. |
There was a problem hiding this comment.
My mind keeps coming back to this statement. It is an extremely common use-case to not know the data ahead of time: I want to select a villager from a list. The game queries the list of villagers currently available, and spawns buttons for them so that I can click on one. Or just a search bar in an application.
Does this mean that we cannot support those use cases at all?
There was a problem hiding this comment.
Agreed, now that you highlight this I share your concerns.
There was a problem hiding this comment.
So. To be precise, this section is really meant to contrast with the pattern of spawning and despawning each menu as they are created.
The limitation here (I've not tested it) I think is only when despawning the parent menu (for example despawning the main menu when entering the options menu), not when creating dynamically new menus you can enter, or despawning menus that do not have focus.
When you go back one level of menus (pressing backspace or clicking a "back" button), the navigation algo reads the Entity of the button the menu is "reachable from" (aka the button that activated the menu), and if that Entity is despawned, probably bad things happen (panic! most likely), as we now sort of "dereference a dangling Entity".
However, the reverse case of creating new menus at runtime is widely supported, the MenuBuilder::ParentEntity enum variant should help with runtime spawning of new menus, since you can just give it whatever Entity with a Focusable component you have available. It does mean however that the submenus must be created before the button is activated (for example when it gets focused/hovered).
It is not necessary for the whole tree to be loaded at once, but rather than the path back to the root menu is. For support for giving back focus to the last focused element (prioritized) you would also need to keep the Focusables and menu tree of already-explored menus loaded. But typically, for a dropdown, this isn't something people expect.
This is why I mentioned an "infinitely deep recursive menu" as an edge case ui-navigation couldn't support.
I'm currently making a sort of "infinite weapon upgrade menu" as a cool example to demonstrate how easy it is to use your own navigation methods (using Sprite and Text2d) and generating new menus on the fly.
There was a problem hiding this comment.
When it comes to creating new Focusables in an existing menu, it should be alright. But it's always going to be difficult when removing them (beside the mandatory minimum 1 Focusable in a menu). TreeMenu has a active_child field for avoiding iterating all children, but it completely violates the single-source-of-truth principle which makes despawning Focusables dangerous (you must not despawn a focused or active focusable)
I'll probably have to rework access to the active_child to fallback to the ECS hierarchy when it doesn't exist anymore.
There was a problem hiding this comment.
That is good to know, and still impressive work with all of this. Is the last remaining hurdle then to find a good way to remove leaf nodes in the navigation tree dynamically?
The requirement that the path back to root must exist I think is acceptable as long as we can dynamically remove leaf nodes
|
This needs to be split into three different changes
It's simply not feasible to integrate such a huge chunk of changes in bevy without more fine grained reviews. I'll close this RFC. I've no short term plans on opening the 3 RFCs the split would require. |
RENDERED
Two new components and a new system to define navigation nodes for the ECS UI.
On top of making it easy to implement controller navigation of in-game UIs, it enables keyboard navigation and answers accessibility concerns such as the ones raised in this issue.
Roadmap
NavFence⇒NavMenu?NavRequestand input handlingbevy_ui_navigationis already taken (my bad 😢)TreeMenusystems (insert_tree_menusandset_first_focused)Display::None(if it doesn't work already)