Skip to content
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 islands to NavMesh API #1866

Merged
merged 13 commits into from
Sep 19, 2022
Merged

Add islands to NavMesh API #1866

merged 13 commits into from
Sep 19, 2022

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Sep 16, 2022

Motivation and Context

Though the underlying C++ nav module has been computing islands on the back-end, we hadn't yet exposed island filtered queries for NavMesh operations.

This PR introduces new island operations:

  • get_island(point) - get the island index closest to a point
  • island_area(index) - get the area of a specific island by index
  • island_radius(point) and island_radius(index) - get the radius heuristic for island size
  • num_islands - property for number of islands on a NavMesh

and adds optional island specification to other operations:

  • get_random_navigable_point() and get_random_navigable_point_near() - now can query a specific island by index
  • snap_point() - now can snap to a specified island
  • build_navmesh_vertices() - can get vertex buffer for a specific island
  • build_navmesh_vertex_indices() - can get index buffer for a specific island

Also adds and updates docstrings for the full Nav module in both C++ and Python.

How Has This Been Tested

Added new pytests for island functionality.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 16, 2022
Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

I added some code suggestions to address some performance concerns with std::unordered_map (which is admittingly complex to use properly pre-C++17 with try_emplace).

Mainly:

  1. Calling reserve() to prevent tons of reallocations / rehashing of the hashmap as the container grows
  2. Emplacing the std::vector directly into the map during construction
  3. Emplacing the value directly if it makes sense (non-primitive type).

src/esp/nav/PathFinder.cpp Show resolved Hide resolved
src/esp/nav/PathFinder.cpp Outdated Show resolved Hide resolved
src/esp/nav/PathFinder.cpp Outdated Show resolved Hide resolved
src/esp/nav/PathFinder.cpp Show resolved Hide resolved
src/esp/nav/PathFinder.cpp Outdated Show resolved Hide resolved
aclegg3 and others added 4 commits September 19, 2022 08:25
Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>
Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>
Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>
Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Simplify a bit. I don't think explicitly default initializing the unordered_map is even necessary at all. (For default constructible values at least).

src/esp/nav/PathFinder.cpp Outdated Show resolved Hide resolved
Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com>
@aclegg3 aclegg3 merged commit 83fe46b into main Sep 19, 2022
@aclegg3 aclegg3 deleted the nav-islands branch September 19, 2022 18:27
@aclegg3 aclegg3 mentioned this pull request Oct 19, 2022
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants