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

Make NavMap objects request sync only on demand #99646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Nov 24, 2024

Replaces brute-force sync check loop with a self-listing system where each map navobject is responsible for its own sync, requesting the sync only on demand once.

Previously each NavMap::sync() the map looped over every single navobject on the map on the main thread. For each physics step it looped over all regions, links, agents and obstacles, to check if any of them are dirty and needed to be synchronised. If dirty it called their sync function or whatever equivalent on the main thread in the middle of the physics step iteration.

Let's just say having potentially thousands of unchanged navobjects on the map checked every single physics step and updated on the main thread was less than ideal. Doing all the more costly sync updates while blocking physics on the main thread was even worse. Who would have guessed?

This PR does not change things about the threading but it prepares the base. Even if performance was a minor incentive for this change the current system was also not long-term sustainable with more and more different navobjects added or additional sync points required for async updates. This PR mostly copies how this is handled by similar servers or classes.

@smix8 smix8 added this to the 4.4 milestone Nov 24, 2024
@smix8 smix8 requested a review from a team as a code owner November 24, 2024 18:27
Replaces brute-force sync check loop with a self-listing system where each object is responsible for its own dirty sync, requesting it on demand only.
@fire
Copy link
Member

fire commented Nov 24, 2024

Do you have a method for testing this like a test project?

@smix8
Copy link
Contributor Author

smix8 commented Nov 24, 2024

Do you have a method for testing this like a test project?

For stability test just spawn, change, and delete (ten) thousands of NavigationRegion3D, NavigationLink3D, NavigationAgent3D and NavigationObstacle3D nodes at random intervals between physics steps until your CPU catches fire.

Also helps when the reported debug monitor navigation section numbers make sense after every change.

The --test -test-suite= *[Navigation]* already confirmed that there are no (major?) sync errors because those tests expect a valid result directly after a single sync, else the tests start screaming which I had multiple times while working on this.

Performance testing in a vacuum is mostly pointless for this PR as it was sliced from a bigger PR for async synchronisation missing the main accompanying pieces. I am piecemealing that larger PR atm with smaller PRs like here to escape the rebase hellscape as best as I can.

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.

2 participants