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

Optimize some parts of NavMap::sync #90182

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Apr 3, 2024

This is a few optimizations I've done while investigating navigation on a large procedural streaming terrain.

  • Reduced the amount of HashMap lookups
  • Use a "pair" structure for connections instead of heap-allocated Vector<T> because they only ever have up to 2 elements
  • Pre-allocate instead of letting several reallocations occur in push_back and HashMap re-hashs
  • Use squared distance instead of distance to avoid square root calls

Note: I saw that in Find the compatible near edges there is a double for loop, which seems to pretty much iterate every possible pair of free edges. This will iterate each pair twice, (A, B) and (B, A). Unless I'm missing something, the inner loop could use for (unsigned int j = i + 1; j < free_edges.size(); j++) { so each pair only has to be iterated once. But I haven't done that change because I'm not sure if the current loop is actually intented.

@smix8 smix8 added this to the 4.x milestone Apr 4, 2024
@smix8 smix8 modified the milestones: 4.x, 4.4 Sep 22, 2024
Copy link
Contributor

@smix8 smix8 left a comment

Choose a reason for hiding this comment

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

Approve for the change in general. This still needs someone code review and a rebase / snyc with #93005 that edits similar parts.

Nit imo would be good to change the unsigned int to uint32_t as that is used for all other LocalVector loops so things stay consistent.

I had time to test this PR more thoroughly with current master and because it reduced the amount of allocation it is a small net performance gain without complicating the code base too much. Might want to move things later to a struct to make the HashMaps and LocalVectors also reusable to keep the memory available but that can be done with another PR. I didnt have time to really look into the link loop but that is definitely an area that also needs improvements at some point as it is a major bottleneck that does not scale well.

@akien-mga akien-mga changed the title Optimize some parts of NavMap::sync Optimize some parts of NavMap::sync Sep 23, 2024
@smix8
Copy link
Contributor

smix8 commented Oct 18, 2024

@Zylann PR #93005 has now been merged. Would you be available to rebase on top?

@Zylann Zylann force-pushed the nav_map_optimizations branch from 0cf6a73 to a92e1d4 Compare October 19, 2024 14:14
@Zylann Zylann requested a review from a team as a code owner October 19, 2024 14:14
@Repiteo Repiteo merged commit 6206578 into godotengine:master Nov 5, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 5, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants