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

[3.x] Fix navigation merge errors on too small triangles #56879

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

Scony
Copy link
Contributor

@Scony Scony commented Jan 17, 2022

Fixes #56786 and part of #57254

See #56786 (comment) for detailed explanation.

Please note this change may potentially have a severe impact on navigation, so it would be nice if someone else can test the lack of errors as well as Agents traversing navmesh.

Once this is accepted I'll open similar for master.

@Duroxxigar @AndreaCatania

@Calinou Calinou added this to the 3.5 milestone Jan 17, 2022
@akien-mga akien-mga requested a review from a team January 18, 2022 09:03
@akien-mga akien-mga changed the title [3.x] Fix navigation merge errors 3.x [3.x] Fix navigation merge errors on too small triangles Jan 18, 2022
@Scony Scony force-pushed the fix-navigation-merge-errors-3.x branch from 1be0142 to 6c90165 Compare January 18, 2022 16:28
@Scony Scony marked this pull request as draft January 18, 2022 18:59
@Scony
Copy link
Contributor Author

Scony commented Jan 18, 2022

I've created a proper demo to visualize paths going through "small" triangles which I've skipped and it turns out the triangles are not bypassed. They're removed. See the demo with drawing random paths on generated navigation with 2 merge errors:
2022-01-18-195711_4480x1440_scrot

And hence I need to improve it further so they're taken into account.

@Scony
Copy link
Contributor Author

Scony commented Jan 18, 2022

While analyzing further I've found yet another problem - when calculating cell key (id) we're dividing Y coordinate by cell_size while cell_height should be used.

With additional precision improvement, everything works as expected, but it's still not perfect.

I'll sync regarding it with @AndreaCatania offline.

@Scony Scony force-pushed the fix-navigation-merge-errors-3.x branch from 6c90165 to 210a1c7 Compare January 24, 2022 23:15
@Scony
Copy link
Contributor Author

Scony commented Jan 24, 2022

I had to change detail/sample_max_error default to 5 so that vertices computed by recast are aligned with the voxels grid much better. This way we're avoiding problems even on very complex meshes while sacrificing some navmesh details.

In practice (flat meshes or meshes with some gentle hills) this makes no difference while this way we're able to avoid potential problems.

The grid alignment algorithm will have to be re-done anyway, but IMO this commit at least polishes what's already there.

@Scony Scony force-pushed the fix-navigation-merge-errors-3.x branch from 210a1c7 to be88ad4 Compare January 24, 2022 23:29
@Scony Scony marked this pull request as ready for review January 24, 2022 23:38
@Scony Scony requested review from a team as code owners January 24, 2022 23:38
Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

It looks nice! Great job, just a small comment on the function get_point_key.

modules/navigation/nav_map.cpp Outdated Show resolved Hide resolved
@Scony Scony force-pushed the fix-navigation-merge-errors-3.x branch from be88ad4 to d131e11 Compare February 2, 2022 22:47
- improved `detail/sample_max_error` default value
- improved floating point precision handling in cell key calculations
- improved `merge error` error message
- exposed `cell_height` of `nav_map` to the `Navigation`
- fixed cell key `y` calculation
@Scony Scony force-pushed the fix-navigation-merge-errors-3.x branch from d131e11 to 6c6e50b Compare February 2, 2022 22:52
Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

Looks good!! Thanks 🍉

@akien-mga akien-mga merged commit 9ee7527 into godotengine:3.x Feb 3, 2022
@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