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 navigation mesh edge connections optional #75601

Merged
merged 1 commit into from
May 12, 2023

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Apr 2, 2023

Makes navigation mesh edge connections optional.

Helps with performance issues like #68642

For context, the NavigationServer merges different navigation region either by polygon edgekey or by edge connection.

Edgekeys require a "perfect" overlap of edges as their two vertex need to fall into the same map cell. This navigation polygon merge is very quick and in comparison to edge connections costs nearly no performance.

Edge connections on the other hand create connections by checking free edges for distance and angle and project the edges against each other which is a very costly operation for each possible and free edge pair everytime the navigation map changes.

Edge connections are a great tool for small navigation maps and also help especially beginners to get those navigation meshes that have no "perfect" edge overlap to still somehow connect for the pathfinding e.g. because the navigation meshes are sloppy created / placed by hand. Their rather heavy performance cost makes them ill-suited for runtime changes on large navigation maps with many navigation mesh polygon edges so more advanced users struggle with their performance impact.

Edge connections get more and more problematic for performance the larger a navigation map is cause the growing number of unmerged edges need to be compared against each other for a possible edge connection. Also another issue with edge connections is that they are only virtual connections and not real navigation mesh polygons which causes quality problems with point queries and pathfinding / path resets that some users never expect. They make it hard to predict merge behavior at runtime with procedual navigation.

The gist is users might not want to use edge connections for one reason or another so giving users the option to disable edge connections either per navigation region or for the entire navigation map makes sense.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I'm not very qualified on navigation issues, but lacking any other reviewer or user to provide feedback, I'm happy to go with your assessment of the need for this feature.

Makes navigation mesh edge connections optional.
@smix8 smix8 force-pushed the edge_connections_optional_4.x branch from 5097a97 to f986b52 Compare May 11, 2023 16:47
@smix8
Copy link
Contributor Author

smix8 commented May 12, 2023

Groud mentioned in dev chat that the pr looks ok but also mentioned it makes it difficult to merge navigation polygons when edge connections are disabled. That is expected and ok as the option is targeted at experts that accept the more strict requirements for performance.

It is a trade-off option between high performance with more strict requirements and allowing more willy-nilly creation and placement of polygons which comes at added cost.

The engine should not make this choice for users hence why this pr turns edge connections into a user controlled option. By default the status quo is kept with the edge connections enabled everywhere.

Users that think they do not require edge connections need to disable them deliberately in the options or with the NavigationServer API.

@akien-mga akien-mga merged commit 942cb47 into godotengine:master May 12, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 4.x, 4.1 May 12, 2023
@smix8 smix8 deleted the edge_connections_optional_4.x branch May 12, 2023 08:24
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.

3 participants