Add checks to NavigationAgent*D to account for overshoots#114195
Add checks to NavigationAgent*D to account for overshoots#114195SourceOfHTML wants to merge 1 commit intogodotengine:masterfrom
Conversation
fe37161 to
644792b
Compare
| return p_origin.distance_to(navigation_path[navigation_path_index]) < path_desired_distance; | ||
| Vector2 waypoint = navigation_path[navigation_path_index]; | ||
| return p_origin.distance_to(waypoint) < path_desired_distance || | ||
| waypoint.distance_squared_to(Geometry2D::get_closest_point_to_segment(waypoint, previous_origin, p_origin)) < path_desired_distance * path_desired_distance; |
There was a problem hiding this comment.
I think these changes need to be benchmarked to make sure this doesn't create any regressions, depending on how heavily this method is called this could be a problem, for 2D and 3D
There was a problem hiding this comment.
I agree, makes sense. Should I benchmark with multiple agents + avoidance, or just one? I should note that I haven't had to use pathfinding in any of my projects so far, so I'm very unfamiliar with the setup of it and how it's done (and tutorials on the subject are bare at best)
There was a problem hiding this comment.
Not sure exactly how the method is used, a good starting point is probably the benchmark project, building with the exact same build set up with and without these changes
There was a problem hiding this comment.
Did the benchmarks, and the results are:
"name": "1000 Moving Agents",
"results": {
- "render_cpu": 0.06149,
- "time": 0.021
+ "render_cpu": 0.07324,
+ "time": 2.237
}
},
@@ -13,6 +13,6 @@
"name": "1000 Moving Agents",
"results": {
- "render_cpu": 0.01894,
- "time": 0.154
+ "render_cpu": 0.02224,
+ "time": 0.65
}
},
@@ -21,5 +21,5 @@
"name": "Navigation 10 000 Random Paths",
"results": {
- "time": 7.075
+ "time": 7.809
}
},
@@ -28,5 +28,5 @@
"name": "Navigation 10 000 Random Paths",
"results": {
- "time": 8.078
+ "time": 8.04
}
}
The lines prepended with a - were done in the master branch.
Apparently my version performs 100x worse in 2D, but only ~4 times worse in 3D?
There was a problem hiding this comment.
That might be due to differences in get_closest_point_to_segment (something to evaluate separately perhaps, but in this case far more time is probably spent in other parts of the code making this change have less of an impact), but this indicates that more detailed benchmarking might be needed to check the impact to have that data available for evaluating this PR
To be clear a loss of performance isn't a reason alone to stop this change, but it's important information to evaluate, especially as this might be an edge case but would affect all users for no benefit for most
There was a problem hiding this comment.
Point taken, I'll see how I can perf the moving agents benchmark. Hopefully this time it won't take forever to... Do whatever it is perf does below the hood.
There was a problem hiding this comment.
The performance metrics are in!
The sample costs in the master branch (more specifically, v4.6.beta.custom_build [551ce8d]) are thus:
_is_within_waypoint_distance: 0% of cycles for self, 0.00426% cycles aggregate
_is_within_target_distance: 0% of cycles for self, 0.0197% cycles aggregate
Sample costs for this PR:
_is_within_waypoint_distance: 0.0217% of cycles for self, 0.0748% cycles aggregate
_is_within_target_distance: 0.0147% of cycles for self, 0.0578% aggregate.
This is the result of running the godot-benchmarks project for only the 2D version of the 1,000 moving agents (only once, I am still new to perf record) benchmark test, as that one seems to have been hit harder for performance.
It doesn't seem particularly close to the results given by godot-benchmarks itself (Less than 100 times increase from the cycle costs themselves. Still, it's a very strange difference between the two), but it is data.
|
So to clarify the thing above about performance I'd say the following should be considered:
Nothing to prevent this but it'd be important information to evaluate this change |
|
I am still in the process of setting things up for a proper look at the specifics of performance, but while I do that, I'd like to throw in my two cents: I don't believe that this fix/change is worth merging (at least, not as I've implemented it, costly as it looks to be). The problem is too specific (High Speed with requirements of low If it is decided that we want to merge this change, I believe it would be better if the user could control whether or not this extra check is performed. Though, even now the Inspector tab for |
|
644792b to
65c4f1a
Compare
resolves #114006
Between a sphere-line segment check and this point-line segment check, the latter was faster (it certainly uses fewer square root options), so I went with that one.
I do have one hang-up about this PR: I had to add new variables to NavigationAgent*D, which I don't think is good design, but I can't come up with any other solution.