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

Improve NavMeshQueries3D::polygons_get_closest_point_info performance #97928

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

kiroxas
Copy link
Contributor

@kiroxas kiroxas commented Oct 7, 2024

Changed the logic of the function polygons_get_closest_point_info in the NavMeshQueries3D class.

Instead of retriangulating each polygons, we iterate over each edge of the polygon, and check on which side lies the point we are looking for. If it's clockwise of all the sides it lies on the interior and we can just return the point. Otherwise, on the sides where it is on the exterior, we calculate the closest point for this edge and take the minimum of those.

All these timings are with the godot demo project navigation_mesh_chunks, with different sizes of chunks.

Timings

Those timings were wrong, there was an error in the code, check the post below for actual timings

(The smaller the chunkSize, the more polygons on the navmesh)
The new implementation has two benefits :

  • less cycles per polygon which scales much better to huge navmeshes
  • we early out if we find the point inside a polygon, which can reduce even further in some cases.

This is unclear if we want the early out or not, as if polygons do not have the same normal and the point we're searching does not belong to any plane where a polygon lies, the result could be wrong. If this is the desired case, we just need to remove the early break, and estimate the distance with a dot product between the point and the normal when we find it inside.

@kiroxas kiroxas requested a review from a team as a code owner October 7, 2024 12:51
@AThousandShips AThousandShips changed the title Improved polygons_get_closest_point_info performance Improve NavMeshQueries3D::polygons_get_closest_point_info performance Oct 7, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Oct 7, 2024
@smix8 smix8 modified the milestones: 4.x, 4.4 Oct 15, 2024
@kiroxas
Copy link
Contributor Author

kiroxas commented Oct 17, 2024

So it appears there was an error in the code, and it was missing one side check causing the early out to be taken when it should not.
This is fixed in current code, but timings are now less impacting than previously.

Timings

All the timings below are been taken using rdtsc. The lower, the better.

Previous Implementation with chunk size 256: 68 000 cycles
Previous Implementation with chunk size 32: 1 350 000 cycles

Edge Implementation with chunk size 256 : 56 000 cycles (x 1.25 )
Edge Implementation with chunk size 32 : 1 160 000 cycles (x 1.16 )

So, when the early out is not taken, we have between 15% and 25% improvement.
I also hardened the early out, to only be taken if the projected point lies into the plane, which will not change anything for 2D, but will give correct points for 3D.

TLDR: Expect a 15% speed up on points outside of the navmesh, and possibly a lot more if the point lies inside.

@kiroxas kiroxas force-pushed the navImprovement branch 3 times, most recently from b23db4f to 70897fe Compare October 18, 2024 06:20
@kiroxas kiroxas force-pushed the navImprovement branch 3 times, most recently from 3eb5d67 to f326f0e Compare November 6, 2024 12:49
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.

I did some testing and it seem to work fine and the speed up is noticeable on larger maps.

I would appreciate if more people could test it considering that in the past these kind of more math heavy optimization changes regularly made small edge case regresssions show up only very late.

I am still willing to merge not knowing if this will be the case because the performance gain is very good and this is a frequently used function in projects.

Just a fair warning note, because this is a hot function for some projects it especially needs to work reliable. In the worst case if things turn out broken in the RC stages we might need to revert this on a hurry.

Still needs a code review.

@akien-mga akien-mga merged commit 7f3a8d0 into godotengine:master Dec 2, 2024
20 checks passed
@akien-mga
Copy link
Member

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.

4 participants