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 NavMeshGenerator2D::generator_bake_from_source_geometry_data performance #98957

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

kiroxas
Copy link
Contributor

@kiroxas kiroxas commented Nov 8, 2024

Removed a lot of unecessary copies and duplicated work in NavMeshGenerator2D::generator_bake_from_source_geometry_data.

Tested on all godot examples project 2D Navigation. (navigation, navigation_astar, navigation_mesh_chunks)
Reduced navmesh creation for the chunk project from 34 000 000 cycles to 17 000 000 cycles on average. (mesured with RDTSC)
So roughly a 2x speed up.

@smix8
Copy link
Contributor

smix8 commented Nov 8, 2024

Agree that the 2d baking could be improved and some of the changes here look fine.
Although making the internal source geometry arrays and locks public can't be done. This needs to be handled differently.

@kiroxas
Copy link
Contributor Author

kiroxas commented Nov 8, 2024

We can probably get away with it with some getter returning const references. They exist (at least two of them) but they look wrong to me :
const Vector<Vector<Vector2>> &NavigationMeshSourceGeometryData2D::_get_traversable_outlines() const { RWLockRead read_lock(geometry_rwlock); return traversable_outlines; }
as the lock here is not locked when accessing the data later, which makes it wrong to use this instead of what I'm doing. But I can totally use them and create another one for _get_projected_obstructions, if this is the preferred way.

@kiroxas
Copy link
Contributor Author

kiroxas commented Nov 8, 2024

So the performance is roughly the same with the getters, so I made the change. Only the function is changed now.

@kiroxas kiroxas force-pushed the NavBakeSourceImprovement branch 3 times, most recently from c7a6af7 to 08f353b Compare November 10, 2024 12:19
@kiroxas
Copy link
Contributor Author

kiroxas commented Nov 10, 2024

As suggested by smix8, I took the friend path instead of public.

@kiroxas kiroxas force-pushed the NavBakeSourceImprovement branch 2 times, most recently from 87f824c to 88ce90e Compare November 10, 2024 13:50
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.

Tested a few projects and bakes fine without any noticeable error.

Still needs a code review.

@smix8 smix8 modified the milestones: 4.x, 4.4 Nov 10, 2024
modules/navigation/2d/nav_mesh_generator_2d.cpp Outdated Show resolved Hide resolved
modules/navigation/2d/nav_mesh_generator_2d.cpp Outdated Show resolved Hide resolved
…` performance

Avoid copies and redundant work.
@akien-mga
Copy link
Member

Just pushed an amend to edit the commit message to match the PR title and be more explicit.

@Repiteo Repiteo merged commit 95a5034 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks! Congratulations on your first contribution! 🎉

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