Code cleanup and small optimizations in RecastFilter.cpp rcFilterLedgeSpans #672
Conversation
Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal.
Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal.
Revise Comment
| if (neighborBot < accessibleNeighborMinHeight) accessibleNeighborMinHeight = neighborBot; | ||
| if (neighborBot > accessibleNeighborMaxHeight) accessibleNeighborMaxHeight = neighborBot; | ||
| } | ||
| else if (accessibleNeighbourHeight < -walkableClimb) |
There was a problem hiding this comment.
Is it safe to break out of the loop here? minNeighborHeight might not get set correctly if there are multiple adjacent walkable areas that overlap more than walkableClimb with the walkable area above the current span.
There was a problem hiding this comment.
oh nvm I guess since iteration order is bottom to top it's fine
|
Ty! |
…eSpans (recastnavigation/recastnavigation#672) - recastnavigation/recastnavigation@3e94c3b * Code cleanup and minor refactor in RecastFilter.cpp rcFilterLedgeSpans Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal. * Code cleanup and minor refactor in RecastFilter.cpp rcFilterLedgeSpans Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal. * Update RecastFilter.cpp Revise Comment
…eSpans (recastnavigation/recastnavigation#672) - recastnavigation/recastnavigation@3e94c3b * Code cleanup and minor refactor in RecastFilter.cpp rcFilterLedgeSpans Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal. * Code cleanup and minor refactor in RecastFilter.cpp rcFilterLedgeSpans Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal. * Update RecastFilter.cpp Revise Comment
|
This PR changed the result of navmesh generation. Does not look like it broke something but it's unclear if this was intentional. If this is an optimization of the generation process it should not affect the data. One more reason to do #678 . What exactly has been changed in the navmesh data requires additional investigation but the change is visible because different paths are generated for the same input (to recast to generate navmesh to be used by detour). In a test case where I've noticed this just one point is added in the middle of the path and it looks legitimate. |
Thank you for your investigation. Could you please send me the problematic test cases (3D models along with their corresponding parameters)? Also, could you specify where the Navmesh encountered issues? I'm really sorry. I might not be able to guarantee solving this problem right now because my current schedule doesn't allow for in-depth investigation. If there are issues with the walkability of the edge span, then it indicates there might be a problem with my PR. If different situations occur, they are very apparent in the Walkable Voxels view. This function only handles the edge parts of the span, detecting walkable spans around it (above, below, and adjacent) based on the Agent's Max Climb and Height. If there are significant drops around the span (cliffs, overhangs, window sills, steep slopes, etc.), it marks the span as impassable. |
It's not very simple to share the exact model. The problem appears in a scene composed out of many models and the contents is protected by the license not allowing to share it (it's not owned by me). Basically you need to buy the game. Although the program using the content to generate navmesh and find paths is open source. If you're interested I can share more details.
I'm interested if the change was intentional or expected. If it does not break anything, no need to worry. Here is how it looks like:
|
This PR shouldn't modify the Navmesh. |
Not without writing code. I can try but this might take some time. |
|
Here are the screenshots with rendered walkable voxels ( before:
after:
There is at least one voxel with no longer walkable surface now. |
Merge recastnavigation/recastnavigation#672 Actually improves the filtering of ledge spans as they are now a lot firmer against their limits, this is also beneficial for traverse link generation as we get more accurate ledge spans to work with (making the offsetting math do its job better since it assumes accurate ledge spans). There was a report further down this pull request that pathing results differed after this change, along with a new one at recastnavigation/recastnavigation#729. I was however not able to replicate it, nor did any behavior change in-game on a navmesh generated on kings canyon.
|
|
||
| // Skip neighbour if the gap between the spans is too small. | ||
| if (rcMin(top, neighborTop) - rcMax(bot, neighborBot) > walkableHeight) | ||
| if (rcMin(top, neighborTop) - rcMax(bot, neighborBot) >= walkableHeight) |
There was a problem hiding this comment.
Why the comparison is changed from > to >=?
…lterLedgeSpans (recastnavigation#672)" This reverts commit 3e94c3b.
|
|
||
| // Skip neighbour if the gap between the spans is too small. | ||
| if (rcMin(top, neighborTop) - rcMax(bot, neighborBot) > walkableHeight) | ||
| if (rcMin(top, neighborTop) - bot >= walkableHeight) |
There was a problem hiding this comment.
Why the comparison is changed from >to >=?
recastnavigation#672 changed comparison operators from > to >= in 2 places for unclear reason which changed how navmesh is generated (recastnavigation#729). Add test case affected by this change.
recastnavigation#672 changed comparison operators from > to >= in 2 places for unclear reason which changed how navmesh is generated (recastnavigation#729). Add test case affected by this change.
recastnavigation#672 changed comparison operators from > to >= in 2 places for unclear reason which changed how navmesh is generated (recastnavigation#729). Add test case affected by this change.
recastnavigation#672 changed comparison operators from > to >= in 2 places for unclear reason which changed how navmesh is generated. Add test case affected by this change.
Because span.smax is always > 0, bot > 0 as well, and (-walkableClimb - bot) is always < -walkableClimb. Furthermore, as long as minNeighborHeight < -walkableClimb' at least once, there is no need to continue the traversal.