Skip to content

Conversation

@kumaryash6352
Copy link
Contributor

Objective

Solution

Turn NavNeighbors from storing an array of Option<Entity> to an array of NavNeighbor, an enum with 3 values: Unset (no explicitly set thing in this direction), Blocked (assume there is nothing in this direction to path to), and Neighbor(Entity).

Testing

Changes tested using a new test, test_respects_set_blocks, and manually using the directional_navigation_override example. Each button on second page is restricted to left/right movement, as up/down are blocked.

Showcase

I'm not quite sure how to showcase this, as it's a very input-oriented feature and the feature working means that nothing happens.

Changes navigation to use a 3-state enum, distinguishing
between "unset" neighbors, "explicitly removed" neighbors,
and "known" neighbors. Unset neighbors can be overwritten by
auto nav building, while explicitly removed slots cannot.
Documents surprising behavior when inputting a
diagonal direction (ie. `NorthWest`) when
one of the components are blocked (ie. `North`)
pub enum NavNeighbor {
/// No neighbor explicitly set.
#[default]
Unset,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that it will be left to be determined automatically, so maybe Auto would be clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to Auto in latest commit.

/// Do not find a neighbor.
Blocked,
/// The neighbor is known and set.
Neighbor(Entity),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NavNeighbor::Neighbor seems a bit redundant, maybe NavNeighbor::Set(Entity)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to Set in latest commit.

.set(direction, b);
}

/// Adds an edge blocking automatic naviation from an entity in a direction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Adds an edge blocking automatic naviation from an entity in a direction.
/// Adds an edge blocking automatic navigation from an entity in a direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that, fixed typo in latest commit.

@kfc35 kfc35 added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2026
@kfc35 kfc35 self-requested a review January 22, 2026 02:10
Copy link
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great. I just have small wording fixes / nits. Thank you for your work!

/// Adds an edge blocking automatic navigation from an entity in a direction.
/// Any existing edge from A in the provided direction will be overwritten.
///
/// The reverse block will not be added, so navigation will only be possible from other entities
Copy link
Contributor

@kfc35 kfc35 Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence sounds a little unclear

Suggested change
/// The reverse block will not be added, so navigation will only be possible from other entities
/// The reverse block will not be added, so navigation will still be possible from other entities
/// to the provided entity in the [`CompassOctant::opposite`] direction

self.add_edge(b, a, direction.opposite());
}

// TODO: not quite sure if this is necessary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK to provide; it is small enough to include

}

// TODO: not quite sure if this is necessary
/// Adds a symmetrical edge between two entities in the navigation map.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Adds a symmetrical edge between two entities in the navigation map.
/// Adds a symmetrical blocking edge between two entities in the navigation map.


// TODO: not quite sure if this is necessary
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The A -> B path will use the provided direction, while B -> A will use the [`CompassOctant::opposite`] variant.
/// The blocking A -> B path will use the provided direction, while B -> A will use the [`CompassOctant::opposite`] variant.

///
/// If the entity is not in the map, [`None`] will be returned.
/// Note that the set of neighbors is not guaranteed to be non-empty though!
/// Note that the set of neighbors may be the default value!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By “default value” do you mean the EMPTY value?

direction: CompassOctant,
},
/// Navigation explicitly blocked in the requested direction.
#[error("Naviation explicitly blocked from {current_focus} in the {direction:?} direction.")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[error("Naviation explicitly blocked from {current_focus} in the {direction:?} direction.")]
#[error("Navigation explicitly blocked from {current_focus} in the {direction:?} direction.")]

pages_entities[1][3],
CompassOctant::SouthEast,
);
// Add one-way blocking within the triangle page (Page 1) for down nav.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Add one-way blocking within the triangle page (Page 1) for down nav.
// Add one-way blocking within the first grid page (Page 1) for down nav.

.id();
text_entities.push(footer_info);
}
if page_num == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of copying the if block, you can just have a variable that the Text is set to that is switched based on page_num i.e. something like (excuse the formatting)
let text = if page_num == 2 {
"Vertical Navigation has been manually overridden to be inverted! \ ^ moves down, and v (down) moves up.”, } else {
"Vertical movements disabled on each button, but you can still go to the next row by going off the right side.”
}


// Manually set a block from A to B
// A should NOT be able to nav East to B
// but SHOULD be able to nav South to C
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add an assert for this? it seems like there should be an assert by the way this comment is written, but it isn’t right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants