`add_edges` and `add_grid` helpers for `DirectionalNavMap` by alice-i-cecile · Pull Request #17247 · bevyengine/bevy

alice-i-cecile · 2025-01-08T23:01:25Z

Objective

Prompted by #17224, I realized that we need more helpers to make configuring DirectionalNavMaps for directional (gamepad) navigation easier.

Solution

Add a simple add_edges helper, which does not loop around the end of the list.
Add a add_grid helper for working with grids.
- Obviously, this is really helpful for wiring up e.g. Terraria-style inventory screens.
- Less obviously, this is a powerful mid-level API for defining directional nav layouts more broadly. The coordinates don't need to be physical, and the ability to duplicate entities in the map makes modelling large objects a breeze.
Added more helpers to CompassQuadrant / CompassOctant to make this code prettier: these simple methods should be generally useful to users.

Testing

I'm currently relying on unit tests for these changes. Once #17224 is merged, we can update that the example to test these in a more hands-on way.

To do

make sure tests pass and logic is correct

crates/bevy_input_focus/src/directional_navigation.rs

Co-authored-by: IQuick 143 <IQuick143cz@gmail.com>

IQuick143 · 2025-01-09T19:59:57Z

crates/bevy_input_focus/src/directional_navigation.rs

+    // which uses u16 values. We need signed integers here, but we should be able to cast them losslessly.
+    // PERF: This is a very flexible / "clever" implementation, but it won't be the fastest for simple cases.
+    // If there's user demand for it, we can add a more optimized / less flexible version that requires non-sparse rectangular grids.
+    pub fn add_grid(&mut self, entity_grid: NavGrid, should_loop: bool) {


This promises support for placing entities at multiple locations, however it's not well defined what the result is.
Consider the following grid:

X A Y A

Now... what is the left neighbour of A? It seems to be completely implementation dependent.

Yeah, we should probably choose, document and test a rule for this.

IQuick143 · 2025-01-09T20:00:25Z

crates/bevy_input_focus/src/directional_navigation.rs

+                    // Inserting the same entity at multiple locations is supported,
+                    // so we need to check for this case
+                    if neighbor != entity {
+                        // PERF: we could also add the reverse edge here, to save work later


Multiple positions for the same node would make this very tricky/not possible.

Right 🤔 Okay, I'll cut the PERF note.

IQuick143 · 2025-01-09T20:01:29Z

crates/bevy_input_focus/src/directional_navigation.rs

+
+                if let Some(neighbor) = maybe_neighbor {
+                    // Entities should not be neighbors of themselves
+                    // Inserting the same entity at multiple locations is supported,


Note: This collision can happen also by just an entity looping to itself, which may or (probably) may not be desired.

IQuick143 · 2025-01-09T21:04:01Z

crates/bevy_math/src/compass.rs


 impl CompassOctant {
+    /// The list of all possible [`CompassOctant`] variants.
+    pub const VARIANTS: [CompassOctant; 8] = [


Might be nice to test that this is in the same order as from_index (could we implement from_index like that?)

cart · 2025-01-09T23:09:35Z

crates/bevy_input_focus/src/directional_navigation.rs

@@ -21,9 +21,11 @@ use bevy_ecs::{
    prelude::*,


As a general thought, I think my ideal system is an automatic system that is overridden by manually defined nav targets.

Ex: label ui nodes as "navigable", then for each "navigable" node, compute the closest/best-fit navigable node in the direction specified, unless a node for that direction has been manually specified.

Forcing users to define the graph up front is too cumbersome for the majority of use cases. Ex: a flat list of navigable nodes is trivially implicitly navigable.

This is especially important because many lists of nodes will have different targets based on their current layout. We essentially need those cases to automatically/implicitly adapt to the current context.

As a general thought, I think my ideal system is an automatic system that is overridden by manually defined nav targets.

Fully agree here! I've been exploring APIs and algorithms to make this possible :)

alice-i-cecile · 2025-01-15T19:33:53Z

I think this design of add_grid is a bit too complex / fraught for now. I may revisit it if we want to come back to this later.

# Objective While `add_looping_edges` is a helpful method for manually defining directional navigation maps, we don't always want to loop around! ## Solution Add a non-looping variant. These commits are cherrypicked from the more complex #17247. ## Testing I've updated the `directional_navigation` example to use these changes, and verified that it works. --------- Co-authored-by: Rob Parrett <robparrett@gmail.com> Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>

# Objective While `add_looping_edges` is a helpful method for manually defining directional navigation maps, we don't always want to loop around! ## Solution Add a non-looping variant. These commits are cherrypicked from the more complex bevyengine#17247. ## Testing I've updated the `directional_navigation` example to use these changes, and verified that it works. --------- Co-authored-by: Rob Parrett <robparrett@gmail.com> Co-authored-by: Benjamin Brienen <benjamin.brienen@outlook.com>

@cart

# Context Renaming `Parent` to `ChildOf` in #17247 has been contentious. While those users concerns are valid (especially around legibility of code IMO!), @cart [has decided](https://discord.com/channels/691052431525675048/749335865876021248/1340434322833932430) to stick with the new name. > In general this conversation is unsurprising to me, as it played out essentially the same way when I asked for opinions in my PR. There are strong opinions on both sides. Everyone is right in their own way. > > I chose ChildOf for the following reasons: > > 1. I think it derives naturally from the system we have built, the concepts we have chosen, and how we generally name the types that implement a trait in Rust. This is the name of the type implementing Relationship. We are adding that Relationship component to a given entity (whether it "is" the relationship or "has" the relationship is kind of immaterial ... we are naming the relationship that it "is" or "has"). What is the name of the relationship that a child has to its parent? It is a "child" of the parent of course! > 2. In general the non-parent/child relationships I've seen in the wild generally benefit from (or need to) use the naming convention in (1) (aka calling the Relationship the name of the relationship the entity has). Many relationships don't have an equivalent to the Parent/Child name concept. > 3. I do think we could get away with using (1) for pretty much everything else and special casing Parent/Children. But by embracing the naming convention, we help establish that this is in fact a pattern, and we help prime people to think about these things in a consistent way. Consistency and predictability is a generally desirable property. And for something as divisive and polarizing as relationship naming, I think drawing a hard line in the sand is to the benefit of the community as a whole. > 4. I believe the fact that we dont see as much of the XOf naming style elsewhere is to our benefit. When people see things in that style, they are primed to think of them as relationships (after some exposure to Bevy and the ecosystem). I consider this a useful hint. > 5. Most of the practical confusion from using ChildOf seems to be from calling the value of the target field we read from the relationship child_of. The name of the target field should be parent (we could even consider renaming child_of.0 to child_of.parent for clarity). I suspect that existing Bevy users renaming their existing code will feel the most friction here, as this requires a reframing. Imo it is natural and expected to receive pushback from these users hitting this case. ## Objective The new documentation doesn't do a particularly good job at quickly explaining the meaning of each component or how to work with them; making a tricky migration more painful and slowing down new users as they learn about some of the most fundamental types in Bevy. ## Solution 1. Clearly explain what each component does in the very first line, assuming no background knowledge. This is the first relationships that 99% of users will encounter, so explaining that they are relationships is unhelpful as an introduction. 2. Add doc aliases for the rejected `IsParent`/`IsChild`/`Parent` names, to improve autocomplete and doc searching. 3. Do some assorted docs cleanup while we're here. --------- Co-authored-by: Eagster <79881080+ElliottjPierce@users.noreply.github.com>

alice-i-cecile added 6 commits January 8, 2025 13:54

Add add_edges

faf8c87

CompassOctant / Quadrants variants constant

b4935f7

Add Compass*::to_offset helpers

851726d

Add DirectionalNavMap::add_grid

35e3d0d

Add tests

1b67f1d

Test for add_edges

e0aaca9

alice-i-cecile changed the title ~~add_edges and add_grid helpers for DirectionalNavMap`~~ add_edges and add_grid helpers for DirectionalNavMap Jan 8, 2025

alice-i-cecile added 2 commits January 8, 2025 15:07

Test for big grid entities

4651304

Guard against self-edges

4c7142e

alice-i-cecile requested review from IQuick143 and NthTensor January 8, 2025 23:10

alice-i-cecile added 5 commits January 8, 2025 15:13

Test for sparse grids

55527da

Newtype NavGrid, per @sixfold-origami

4522dd8

Clippy

5c16421

Fix broken doc link

1d5c102

Test for trivial grids

305d3e8

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 9, 2025

alice-i-cecile added this to the 0.16 milestone Jan 9, 2025

IQuick143 reviewed Jan 9, 2025

View reviewed changes

crates/bevy_input_focus/src/directional_navigation.rs Outdated Show resolved Hide resolved

The same typo again

36cedb5

Co-authored-by: IQuick 143 <IQuick143cz@gmail.com>

IQuick143 reviewed Jan 9, 2025

View reviewed changes

cart reviewed Jan 9, 2025

View reviewed changes

alice-i-cecile mentioned this pull request Jan 15, 2025

add_edges helper for directional navigation #17389

Merged

alice-i-cecile closed this Jan 15, 2025

alice-i-cecile mentioned this pull request Feb 16, 2025

Improve the docs for ChildOf and Children #17886

Merged

alice-i-cecile mentioned this pull request Feb 17, 2025

Make ChildOf a named struct and support named struct Relationship types #17896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`add_edges` and `add_grid` helpers for `DirectionalNavMap`#17247

`add_edges` and `add_grid` helpers for `DirectionalNavMap`#17247
alice-i-cecile wants to merge 14 commits intobevyengine:mainfrom
alice-i-cecile:edges-and-grid

alice-i-cecile commented Jan 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

IQuick143 Jan 9, 2025

Uh oh!

alice-i-cecile Jan 9, 2025

Uh oh!

IQuick143 Jan 9, 2025

Uh oh!

alice-i-cecile Jan 9, 2025

Uh oh!

IQuick143 Jan 9, 2025

Uh oh!

IQuick143 Jan 9, 2025

Uh oh!

cart Jan 9, 2025 •

edited

Loading

Uh oh!

cart Jan 9, 2025

Uh oh!

alice-i-cecile Jan 9, 2025

Uh oh!

alice-i-cecile commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

alice-i-cecile commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

To do

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cart Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alice-i-cecile commented Jan 8, 2025 •

edited

Loading

cart Jan 9, 2025 •

edited

Loading