-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic directional (gamepad) navigation for UI (and non-UI) #17102
Add basic directional (gamepad) navigation for UI (and non-UI) #17102
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments about the implementation, but it looks great!
It's worth pointing out that I don't think this will work well for highly dynamic or (diegetic 3d) UIs. You'd need either (a) a very fast auto-generation framework or (b) a lot of manual management. So for stuff that moves around on screen in complex nonlinear ways, we'll need something else.
That said, I like this a lot. Most of the UIs we have seen people make are really static (especially the types we tend to see in game jams). Having something purpose-built for the static case that seems great.
/// | ||
/// Pass in the current focus as a key, and get back a collection of up to 8 neighbors, | ||
/// each keyed by a [`CompassOctant`]. | ||
pub neighbors: EntityHashMap<NavNeighbors>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, could it make sense for NavNeighbors
to be a component? Possibly an immutable one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I chewed on this a fair bit. IMO until we have relations we shouldn't try to encode these graphs via components.
self.neighbors.remove(&entity); | ||
|
||
for node in self.neighbors.values_mut() { | ||
for neighbor in node.neighbors.iter_mut() { | ||
if *neighbor == Some(entity) { | ||
*neighbor = None; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we forced the graph to be symmetric we could make this significantly more efficient (and constant-time bounded).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sadly I don't think that's viable. As discussed on Discord, the "three small elements beside one big one" layout is not symmetric.
/// Adds an edge between two entities in the navigation map. | ||
/// Any existing edge from A in the provided direction will be overwritten. | ||
/// | ||
/// The reverse edge will not be added, so navigation will only be possible in one direction. | ||
/// If you want to add a symmetrical edge, use [`add_symmetrical_edge`](Self::add_symmetrical_edge) instead. | ||
pub fn add_edge(&mut self, a: Entity, b: Entity, direction: CompassOctant) { | ||
self.neighbors | ||
.entry(a) | ||
.or_insert(NavNeighbors::EMPTY) | ||
.set(direction, b); | ||
} | ||
|
||
/// Adds a symmetrical edge between two entities in the navigation map. | ||
/// The A -> B path will use the provided direction, while B -> A will use the [`CompassOctant::opposite`] variant. | ||
/// | ||
/// Any existing connections between the two entities will be overwritten. | ||
pub fn add_symmetrical_edge(&mut self, a: Entity, b: Entity, direction: CompassOctant) { | ||
self.add_edge(a, b, direction); | ||
self.add_edge(b, a, direction.opposite()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I think there might be good perf reasons to only have the symmetric case. It's hard for me to imagine a good UI that is not symmetric and the non-symmetric edges do have a real cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IQuick143 has pointed out to me that requiring symmetry may not be a good idea, and I will also admit that perf of graph modification isn't really a major concern.
I'll weaken this to an extremely surface-level nit: Perhaps we should make the symmetric edge fn (which I think is probably what most people will want most of the time) the "default" by making it shorter: add_edge
and add_asymetric_edge
? Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that symmetric edges present too much of a footgun to make the default :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with that. What do you think about allowing multiple vertices of the focus graph to correspond to the same ui element entity, instead of making them 1-to-1? Later in the same discussion we ran into some situations where this seemed desireable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's reasonable, but best left to follow-up. This is the sort of situation that was pushing me towards having a history, but I think there's a bunch of design to chew on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally fair. This has good bones, I think we should run with it largely as is.
/// Starts at 0 for [`CompassOctant::North`] and increments clockwise. | ||
pub const fn to_index(self) -> usize { | ||
match self { | ||
Self::North => 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random thought I had, (this may be irrelevant or out of scope for this change) - is it worth foregoing the nice index integer incrementing and instead making the cardinal directions match up between CompassOctant
and CompassQuadrant
? I'm thinking of a scenario where the integer data is serialized & conversion between the two becomes a bit frustrating for the user of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to just have a From
+TryFrom
impls for the conversions, the indices are better to have a reasonable order, because they'll be used as indices to arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like me to add From + TryFrom traits too? I'd like to keep the existing methods since they're const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way - I'll defer to your judgement here. I think I was partially confused too and thought that the NavNeighbors
get/set
took a CompassQuadrant
as well - though I do think it's a potential pitfall that NavNeighbors.neighbors[CompassOctant::East.to_index()] != NavNeighbors.neighbors[CompassQuadrant::East.to_index()]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that the strongly typed helper methods are going to be preferred so we should be safe.
An alternate approach: rather than a fixed length array of neighbors, have a |
Yep, I think that this is best suited for highly tuned mostly static UIs. For extremely dynamic stuff, you'll be better off with a heuristic combining distance and position like Aevyrie suggested IMO :) |
Co-authored-by: Rob Parrett <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
I think that the current approach is a bit nicer. I don't expect the conversion to relations to be particularly challenging, and if it is we can do this refactor just before that change. |
Yeah I'm personally a fan of the octant approach. I can see how it would make for a clean editor UI as well. |
Co-authored-by: Rob Parrett <[email protected]>
We could also do eight fields on the struct. I'm not sure if that's faster / cleaner though. The little index -> compass octant helpers seem generally quite nice. |
Personally not a huge fan of eight fields on a struct, I think the best type-safe option would be to make a newtype around |
|
||
for node in self.neighbors.values_mut() { | ||
for neighbor in node.neighbors.iter_mut() { | ||
if let Some(entity) = *neighbor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using let-else syntax for less nesting!
impl DirectionalNavigation<'_> { | ||
/// Navigates to the neighbor in a given direction from the current focus, if any. | ||
pub fn navigate(&mut self, octant: CompassOctant) { | ||
if let Some(current_focus) = self.focus.0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let-else here and the next line perhaps
/// Returns an error if there is no focus set or if there is no neighbor in the requested direction. | ||
/// | ||
/// If the result was `Ok`, the [`InputFocus`] resource is updated to the new focus as part of this method call. | ||
pub fn navigate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for another PR, but I wonder if we should also consider "adjacent octants". It may be confusing to users if they accidentally pressed up-and-left on a UI with just a left and their action had no effect. I recognize this is ambiguous (what if you have both a left and an up but no top-left), but I think picking any direction is better than doing nothing.
#[derive(SystemParam, Debug)] | ||
pub struct DirectionalNavigation<'w> { | ||
/// The currently focused entity. | ||
pub focus: ResMut<'w, InputFocus>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For another PR: we might want to consider making InputFocus
a component. It would be nice to support multiple "cursors". For example, in a couch-coop game you may want to vote for the next minigame or something - having independent cursors for each player would be nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've talked a bit about multi-focus before :) It's relatively niche, and we're worried that it will complicate the happy path for handling UI. Open to having my mind changed by a design in the future though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed; there are always going to be minority use cases that don't fit within the model. Fortunately, it's not that hard to roll your own implementation, which doesn't require any changes to Bevy. This is an area where third-party crates can fill the gaps.
Objective
While directional navigation is helpful for UI in general for accessibility reasons, it is especially valuable for a game engine, where menus may be navigated primarily or exclusively through the use of a game controller.
Thumb-stick powered cursor-based navigation can work as a fallback, but is generally a pretty poor user experience. We can do better!
Prior art
Within Bevy, https://github.com/nicopap/ui-navigation and https://github.com/rparrett/bevy-alt-ui-navigation-lite exist to solve this same problem. This isn't yet a complete replacement for that ecosystem, but hopefully we'll be there for 0.16.
Solution
UI navigation is complicated, and the right tradeoffs will vary based on the project and even the individual scene.
We're starting with something simple and flexible, hooking into the existing
InputFocus
resource, and storing a manually constructed graph of entities to explore in aDirectionalNavigationMap
resource. The developer experience won't be great (so much wiring to do!), but the tools are all there for a great user experience.We could have chosen to represent these linkages via component-flavored not-quite-relations. This would be useful for inspectors, and would give us automatic cleanup when the entities were despawned, but seriously complicates the developer experience when building and checking this API. For now, we're doing a dumb "entity graph in a resource" thing and
remove
helpers. Once relations are added, we can re-evaluate.I've decided to use a
CompassOctant
as our key for the possible paths. This should give users a reasonable amount of precise control without being fiddly, and playing reasonably nicely with arrow-key navigation. This design lets us store the set of entities that we're connected to as a 8-byte array (yay Entity-niching). In theory, this is maybe nicer than the double indirection of two hashmaps. but if this ends up being slow we should create benchmarks.To make this work more pleasant, I've added a few utilities to the
CompassOctant
type: converting to and from usize, and adding a helper to find the 180 degrees opposite direction. These have been mirrored ontoCompassQuadrant
for consistency: they should be generally useful for game logic.Future work
This is a relatively complex initiative! In the hopes of easing review and avoiding merge conflicts, I've opted to split this work into bite-sized chunks.
Before 0.16, I'd like to have:
One day, it would be sweet if:
Testing
CompassQuadrant
/CompassOctant
methods