[DRAFT] Add non-allocating versions of PhysicsDirectSpaceState[2D|3D] query methods - #120508
[DRAFT] Add non-allocating versions of PhysicsDirectSpaceState[2D|3D] query methods#120508Chestnut45 wants to merge 1 commit into
PhysicsDirectSpaceState[2D|3D] query methods#120508Conversation
8c26ba8 to
64fa2b3
Compare
64fa2b3 to
4f63443
Compare
4f63443 to
2933c8b
Compare
|
Okay, I've done some tests and microbenchmarks on the current state of this PR to make sure everything is bound correctly and the returned values are the same as the existing method. The testing was done by casting through a CSGBox at the origin in a test scene with the following script: Test and Benchmark Tool Script@tool
extends Node3D
@export_tool_button("Test") var test_btn = test
@export_tool_button("Bench") var bench_btn = bench
@export var bench_iterations := 1_000_000
func _ready() -> void:
if Engine.is_editor_hint():
return
test()
bench()
func bench() -> void:
var state := get_world_3d().direct_space_state
var ray_query := PhysicsRayQueryParameters3D.new()
var ray_result := PhysicsRayQueryResult3D.new()
ray_query.from = Vector3(0, 1, 0)
ray_query.to = Vector3(0, -1, 0)
var start := Time.get_ticks_usec()
for i in bench_iterations:
var _dict := state.intersect_ray(ray_query)
var end := Time.get_ticks_usec()
print("intersect_ray: ", (end - start) / float(bench_iterations), "μs")
start = Time.get_ticks_usec()
for i in bench_iterations:
var _bool := state.intersect_ray_no_alloc(ray_query, ray_result)
end = Time.get_ticks_usec()
print("intersect_ray_no_alloc: ", (end - start) / float(bench_iterations), "μs")
func test() -> void:
var state := get_world_3d().direct_space_state
var ray_query := PhysicsRayQueryParameters3D.new()
var ray_result := PhysicsRayQueryResult3D.new()
ray_query.from = Vector3(0, 1, 0)
ray_query.to = Vector3(0, -1, 0)
var dict_result := state.intersect_ray(ray_query)
var noalloc_rv := state.intersect_ray_no_alloc(ray_query, ray_result)
if not dict_result.is_empty() == noalloc_rv:
print("Both hit" if noalloc_rv else "Both missed")
else:
push_error("Return value is different")
var passed := true
if noalloc_rv:
if dict_result.position != ray_result.position:
push_error("Position is different")
passed = false
if dict_result.normal != ray_result.normal:
push_error("Normal is different")
passed = false
if dict_result.collider != ray_result.collider:
push_error("Collider is different")
passed = false
if dict_result.collider_id != ray_result.collider_id:
push_error("Collider ID is different")
passed = false
if dict_result.face_index != ray_result.face_index:
push_error("Face Index is different")
passed = false
if dict_result.rid != ray_result.rid:
push_error("RID is different")
passed = false
if dict_result.shape != ray_result.shape:
push_error("Shape is different")
passed = false
if passed:
print("Tests passed")Microbenchmark Results (1,000,000 iterations)Specs:
This amounts to around ~400% speedup in raycast execution time, though this is obviously a very limited microbenchmark and bottlenecks often lie elsewhere. My preliminary benches on the rope simulation are likely a lot closer to what users can actually expect to gain by switching to the non-allocating version, depending on context (since raycasts are rarely done in isolation like in the microbenchmark). Users using GDScript can expect moderate gains in raycast-heavy hot paths, and C++ users can expect very significant speedups (close to the theoretical 4x) if they are truly bottlenecked by the allocation and access of dictionaries1. Footnotes
|
|
Very nice! Can this be extended to cover shape intersections, motion casts, and 2D physics? Edit: Just noticed you seem a bit hesitant about adding this extra functionality. Personally, I think if we're adding support for one query, we should do all of them in the same PR, so they can all be added at once instead of waiting for individual PRs to get merged. And even if there's no consensus on naming, users can still grab this PR and patch it into a custom Godot build while still maintaining a source of truth. |
|
You make a good point! I was planning on opening a proposal in the coming days to see if we can come to a consensus on naming before I did the rest, but I think I'll go ahead and add them while the discussion is ongoing. The refactoring wouldn't take that long if some of the names need to be changed. Also, #113970 has already done 99% of the work for this in a very clean way. If they are still working on it and plan to switch it over to a non-breaking API I don't mind closing this PR, but I do have an interest in getting this merged in the near future if possible, so I'm more than happy to just translate the work that was already done, I don't think it would take me longer than a couple days or so of picking away at it between other work. (I just don't want to seem like I'm trying to swoop in and steal an existing PR or anything 😅) |
|
Is it possible to have an api that writes the result into a user-provided pointer so as not to incur any allocation at all? |
I think something like that was attempted in #112433 (if by pointer you mean to a native struct) but it seems to have some inconsistencies with how different languages handle it. Passing a RefCounted already gives huge performance gains, avoids having to allocate a new result object for every query since they can be reused, and it will work in all languages (GDScript, C#, GDExtension, ...) so personally I think it's the best way forward. I don't think that we'll see any meaningful performance gains over this by using raw pointers instead, especially not gains that won't also come with some other cost or inconsistency/annoyance. A RefCounted pretty much behaves like a pointer anyway (albeit with a small amount of overhead and bookkeeping, but it's negligible at this scale, and it's already ~4x faster than the current implementation on the couple of machines I've tested) I've been unexpectedly busy over the last few days so I haven't had time yet to open a proposal for naming discussion and to add the other (2D, shape casts, motion casts, etc.) methods and types to this PR, but I will get to it soon I promise! |
PhysicsDirectSpaceState[2D|3D] query methods
|
Closing this in favor of #113970 since they plan on adopting this approach, discussion will continue in this proposal |
While looking for related PRs I noticed some recent ones that attempted to do something very similar, but they were entirely AI generated, so I feel the need to say that I do not and will never submit AI generated code for review.
What problem(s) does this PR solve?
PhysicsDirectSpaceState3D#112433PhysicsDirectSpaceState2D/3D: add typed result objects #113970Additional information
This PR adds a non-allocating version of intersect_ray to
PhysicsDirectSpaceState3Dby registering a new RefCounted type:PhysicsRayQueryResult3D, and addingPhysicsDirectSpaceState3D.intersect_ray_no_alloc(params: PhysicsRayQueryParameters3D, result: PhysicsRayQueryResult3D). This gives an alternate raycasting path with very good performance on all supported languages, without requiring waiting for native structs or custom variants to come to GDScript.This PR is very similar in spirit to #113970 (and the same motivation applies) except it adds the non-allocating version alongside the existing implementation to avoid breaking changes.
I've run some preliminary benchmarks on the non-allocating version using a raycasting rope simulation in a custom 4.5.1 build with the same changesMore accurate isolated benchmarks below.TODO:
get_result_ptr()to result type, avoid calling setters manually in_intersect_ray_no_alloc