rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Generic UI navigation system

Open nicopap opened this issue 2 years ago • 26 comments

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

  • [X] Create a MVP that acts as a bevy plugin
  • [X] Design doc of latest implementation
  • [ ] Solve open questions
    • [X] Naming: NavFenceNavMenu?
    • [x] Game-orientation and potential conflict with editor design
    • [X] NavRequest and input handling
    • [ ] Any other that were not asked in this RFC?
  • [X] Implement downward navigation optimization & "deep focus memory"
  • [X] Mouse support
  • [ ] Find a name for the bevy crate, since bevy_ui_navigation is already taken (my bad :cry:)
  • [ ] https://github.com/bevyengine/bevy/pull/5378
  • [ ] After merge, open issues for identified shortcomings and possible improvements:
    • [ ] Multicursor
    • [ ] tab-navigation
    • [ ] make public the TreeMenu systems (insert_tree_menus and set_first_focused)
    • [ ] Higher level wrapper for menu switching
    • [ ] Handling of Display::None (if it doesn't work already)
    • [ ] Robustness from deleting parent menus while inside of them
    • [ ] Optimizations, reduce number of iterated focusables in various places

nicopap avatar Nov 06 '21 13:11 nicopap

Implementation going on here: https://github.com/nicopap/ui-navigation

nicopap avatar Nov 06 '21 13:11 nicopap

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.

TheRawMeatball avatar Nov 06 '21 13:11 TheRawMeatball

The user only has to add Navigable components to relevant nodes. With this, the system should be able to construct a flat navigation graph based solely on the Transform (or GlobalTransform maybe?) of the nodes.

They only need to add Container components if they want special behaviors for different menus (like the tabbed menu in the example). Or hierarchical menu navigation (like going option > graphical options > Resolution)

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.

nicopap avatar Nov 06 '21 16:11 nicopap

The user only has to add Navigable components to relevant nodes. ... They only need to add Container components if they want special behaviors for different menus. ...

While I agree with Navigable, I'm not quite as sure on what Container gets us. Wouldn't a simple depth-first iteration over the ui tree where we only look at Navigables get us something basically equivalent?

TheRawMeatball avatar Nov 06 '21 17:11 TheRawMeatball

I thought a bit more about the design. A few things:

  1. I want to get rid of the concept of Plane. Beside 2D movements with arrows, the only real use case for other "dimensions" is just going through a tabbed menu and going up/down the hierarchy. The idea of a "select" plane doesn't make sense and doesn't really work.
  2. The "one container within other containers" is too limiting and I feel it's not too much of a burden to have a tree with multiple branches.
  3. NavigableFocusable. Hopefully this clarifies the difference with Containers.

Tomorrow I'll add a roadmap for the implementation in the ui-navigation repo

nicopap avatar Nov 06 '21 23:11 nicopap

The user only has to add Navigable components to relevant nodes. ... They only need to add Container components if they want special behaviors for different menus. ...

While I agree with Navigable, I'm not quite as sure on what Container gets us. Wouldn't a simple depth-first iteration over the ui tree where we only look at Navigables get us something basically equivalent?

The difference between Container and Navigable is that you can focus on a navigable, while the container cannot be focused. In the example I gave, it wouldn't make sense to be able to focus on the entire submenu, including the panels and the "ABC" menu. Only the specific panels and the individual circles should be able to be focused. The Container is still necessary because it separates the "ABC" menu from the "soul" menu. You would have to press A and B to go from one to another. The other thing is that the "ABC" menu is not the same kind of thing as the panels on the left side, and when cycling through the panels, you shouldn't be able to end up in the large menu.

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 Container also act as hard boundary when searching siblings in the UI hierarchy, reducing the depths of searches, and specifying exactly where to look for focusable elements, rather than going through dead ends.

nicopap avatar Nov 06 '21 23:11 nicopap

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.

nicopap avatar Nov 07 '21 15:11 nicopap

The other thing is that the "ABC" menu is not the same kind of thing as the panels on the left side, and when cycling through the panels, you shouldn't be able to end up in the large menu. 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.

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.

TheRawMeatball avatar Nov 07 '21 16:11 TheRawMeatball

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. Container (that I renamed for now into NavNode) is entirely optional in this example NavNode is not even imported.

In this example we explicitly use NavNode as a way to isolate some parts of the UI from other. You'll notice that we can still move from one container to the other, only the one explicitly marked with a NavNode is closed-off.

Maybe naming it NavigationBarrier would be more explicit?

nicopap avatar Nov 07 '21 17:11 nicopap

Maybe naming it NavigationBarrier would be more explicit?

Yes please, although probably NavBarrier for consistency.

alice-i-cecile avatar Nov 07 '21 18:11 alice-i-cecile

@alice-i-cecile told me to check this out. I got to say, I really like this design, and especially appreciate how NavCommands are sent by the app and not the library. If you add mouse support, I think you should make it another NavCommand that stores a Vec2 or something, and let people create the bindings themselves (unlike the current UI focus system). It requires more work on their side, but I think if bevy wants to work for bigger and more diverse titles, it needs flexibility.

JonahPlusPlus avatar Nov 09 '21 04:11 JonahPlusPlus

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.

nicopap avatar Nov 10 '21 07:11 nicopap

This is the final version of the RFC. It is very different from the first and second versions. I guess I'm requesting for comments :)

Nothing urgent. I implemented everything I need for now and don't expect to start work on the bevy PR before a while. I just need confirmation for the NavRequest open question.

This might even need a new RFC! How bevy handles inputs is important to discuss and this feature quite obviously deals with inputs.

nicopap avatar Nov 14 '21 20:11 nicopap

So mouse input would be handled with a NavRequest::FocusOn, correct? All you would have to do is check which entity the mouse is over when you click and send the Entity. I can get behind that. Gives more options for plugins to work with. As for naming, maybe NavRequest::ChangeMenu and NavEvent::Pass or NavEvent::Through or just NavEvent::NoChange?

JonahPlusPlus avatar Nov 14 '21 21:11 JonahPlusPlus

So mouse input would be handled with a NavRequest::FocusOn, correct? All you would have to do is check which entity the mouse is over when you click and send the Entity. I can get behind that. Gives more options for plugins to work with. As for naming, maybe NavRequest::ChangeMenu and NavEvent::Pass or NavEvent::Through or just NavEvent::NoChange?

I still think your suggestion of having a NavRequest::MousePosition or similar is, if not the best, at least a good solution. I guess it's still an open question, how does mouse hover state and click interact with the navigation system? I completely skipped that part. Do you have a suggestion?

  • Hover ≠ Focus: example: hovering over a tab menu shouldn't modify focus (and change the submenu)
  • Left click = NavRequest::Action?

There might very well be two systems that simply must deal with each other. For example a MouseFocusPlugin that manages its own state independently from the current proposal and emits NavRequest::FocusOn when relevant. Ahah, so exactly what you described.

nicopap avatar Nov 15 '21 08:11 nicopap

Hmmm... A MouseFocusPlugin could always just send both events on a click: FocusOn and Action and then you could add a HoverOn request that could be sent for movement that takes an entity as well. By handling FocusOn first, then Action, it would effectively click on whatever it is hovering over.

Something like MousePosition would make it simple, but is inflexible the more I think about it. For instance, mapping space. Users may want to do complex stuff with ring menus, and perhaps they'll want to switch menu items even when not hovering exactly over them (sort of snapping the mouse to an item). Or perhaps they are making a sort of trolling puzzle game that makes you click on something else to click the button.

Who knows, but it would be redundant to map the Vec2 only for it to calculate which one needs to be clicked when you know which one needs to be clicked. I think it's better to keep it complex and let plugins hide that complexity.

JonahPlusPlus avatar Nov 15 '21 20:11 JonahPlusPlus

However, if the FocusOn technique is what gets implemented, in order to make it easier to work with, there should be a method in the UI to find what Entity is at a location. It wouldn't be necessary for people who store the Entity in some Component but for a lot of use cases, it would be annoying to implement something so basic.

JonahPlusPlus avatar Nov 15 '21 20:11 JonahPlusPlus

While dogfooding the ui-navigation library with a large and straining usecase, I could identify limitation and improvement leads for the library. Developments in the design of the ui-navigation library:

Cancel focusables

Adding a cancel mod to focusables gives a natural way to specify going backward in the menu hierarchy, especially with a mouse.

https://github.com/nicopap/ui-navigation/issues/2

Lock focusables

Adding a lock mod to focusables could facilitate implementation of widgets with complex encapsulated controls. It could also simplify transition between gameplay and inventory.

https://github.com/nicopap/ui-navigation/issues/7

Improvement to menu management

I often needed a way to check in which menu I'm currently in, there is no way to do that with the current implementation.

The way I "worked around" the limitation is by adding a component that specify the current menu to each focusable and query that component in the system responsible to manage NavEvent responses.

I explore designs in https://github.com/nicopap/ui-navigation/issues/4

This is still an open question.

nicopap avatar Jan 23 '22 09:01 nicopap

This looks great. My only note is that mouse input will need to be abstracted to any type of Pointer, including gamepad controlled software cursors. This is something I'm working on, and can integrate on top of this when I upstream. Glad this adds focus, I was missing this with pointer interactions.

Re: mouse input discussion I think this is entirely out of scope of this rfc. The navigation and focus primitives are the important part here. A pointer abstraction is a huge area on its own.

I'd prefer if the implementation of the rfc is completely independent from any ui logic to prevent this abstraction from getting leaky. My use case involves pointers interactions with scene geometry which need to use the focus manager.

aevyrie avatar Jul 16 '22 18:07 aevyrie

This is very out of date. The design changed in key parts since the last draft (remember, this is basically a specification of https://github.com/nicopap/ui-navigation). I actually started drafting an update last week, but got caught up in things. I'll prioritize a little bit.

nicopap avatar Jul 16 '22 18:07 nicopap

I decided to copy parts of the bevy-ui-navigation API docs for this. Since those are well thought out.

nicopap avatar Jul 22 '22 11:07 nicopap

Imagine this skill tree. I'm suspecting that just using NavEvent::Move(direction) would allow the user to freely navigate between nodes as if they were not connected by this hierarchy. Would this design allow me to define navigation so that the user is forced to navigate along the white path?

image

Source: Star Wars Jedi: Fallen Order

Weibye avatar Jul 29 '22 09:07 Weibye

I do think there should be a NavRequest::Next and NavRequest::Previous (or something similar) to facilitate behaviours like tab-cycling through a menu

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.

Imagine this skill tree. I'm suspecting that just using NavEvent::Move(direction) would allow the user to freely navigate between nodes as if they were not connected by this hierarchy. Would this design allow me to define navigation so that the user is forced to navigate along the white path?

Yes. The default navigation option is very rudimentary and only cares about position, but you can disable it to replace with your own strategy (MenuNavigationStrategy trait)

nicopap avatar Jul 29 '22 10:07 nicopap

@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?

nicopap avatar Jul 30 '22 09:07 nicopap

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.

(I especially want the bus factor on ui-navigation to increase over 1 if possible)

Could you elaborate what you mean by this?

Weibye avatar Jul 30 '22 09:07 Weibye

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.

nicopap avatar Jul 30 '22 09:07 nicopap

This needs to be split into three different changes

  1. Event-based interactions
  2. simple gamepad-based UI controls
  3. menu navigation.

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.

nicopap avatar Jan 19 '23 11:01 nicopap