PhysicsDirectSpaceState2D/3D: add typed result objects - #113970
Conversation
a8367c9 to
bdbead3
Compare
5d1900b to
e3d2154
Compare
747916a to
bd00fab
Compare
|
Thank you for your contribution! This is a pretty majorly breaking change, is there a proposal to track this or to evaluate support? This would benefit from having a proper proposal created here to discuss the details and evaluate support for this, as it would be something that would need good support to justify the changes and the need for them I'd say backwards compatibility needs to be maintained by creating new methods instead of replacing the existing ones, as this would be very disruptive to anyone using the existing interfaces |
0481c49 to
1b9423b
Compare
|
This is a very valuable pull request, however I share @AThousandShips's concerns about compatibility breakage. This would not only create extra work for users when updating Godot versions, but it would make it very difficult to write plugins that support multiple engine versions simultaneously. To align on the direction, it would be great if you could respond to the high-level concerns before changing implementation details. Please also note that every push notifies 4 reviewer teams. Thanks! 🙂 |
Thanks for the feedback. I agree this is a significant breaking change, and I understand the concerns around backward compatibility and plugin support. This PR was intended to demonstrate a redesigned API focused on performance and type safety, not to incrementally extend the existing one while keeping compatibility. Since this direction would need a proposal and broader consensus first, and maintaining parallel APIs is outside what I can commit to right now, I will close this PR in its current form. If anyone wants to follow up with a proposal or continue the discussion, it can always be reopened. Apologies for spamming the notifications with all the pushes 😅 |
893a7d1 to
1fc32ac
Compare
|
I want to put my 2 cents in now that I've had a chance to read over the new implementation. This is the perfect solution. I understand and agree that it's important to vet large PRs, especially from new contributors, but please don't let a gem like this fall through the cracks without a careful review first just because it's a feature PR (It's arguable that this is even a "new" feature, since much of the current API was only meant to be a temporary solution until a better method was found). Under the guidelines I believe I'm also considered a new contributor, but I do have a degree in computer science with a specialization in video game programming, and would be more than happy to contribute to any formalized review process if the motivation isn't there for current maintainers. |
|
It may be ideal to encourage discussions on this PR on RocketChat, possibly in the #physics channel. |
PhysicsDirectSpaceState2D/3D: reusable strongly typed resultsPhysicsDirectSpaceState2D/3D: add typed result objects
1fc32ac to
2c63f83
Compare
2c63f83 to
c8ad4e8
Compare
|
I wonder if the Initially, I chose these names for consistency with |
|
I think the internal consistency of the new result types' properties following the methods for naming like that is a good idea and is unambiguous, but it may be simpler (and more in line with the current API) to use I have no strong feelings either way though, and think both are perfectly acceptable. Whatever is preferred by maintainers :) |
|
Linking the small discussion that was had in RocketChat here so it's easier to find for reviewers: https://chat.godotengine.org/channel/physics/thread/6WEXTx24Rdt6LijTv Towards the end of the thread I explain why structs / custom variants are necessarily worse solutions, since they would still require per-query allocations even if we had those features today, unlike this PR. As well, please don't be put off by the +2,000 lines. It's mostly just documentation and class boilerplate for the new result types and methods, and there isn't even any new logic to review. This should be a slam dunk; It benefits all users, there's no downsides, and it solves the problem at the source without adding any complexity. |
This PR adds new query methods to
PhysicsDirectSpaceState2D/3Dthat write results into typed result objects. This avoids allocating dictionary and array results for each query, improving performance.New API
Methods
The following methods are added to
PhysicsDirectSpaceState2D:bool intersect_ray_typed(parameters: PhysicsRayQueryParameters2D, result: PhysicsRayIntersectionResult2D)bool intersect_point_typed(parameters: PhysicsPointQueryParameters2D, result: PhysicsPointIntersectionResult2D)bool intersect_shape_typed(parameters: PhysicsShapeQueryParameters2D, result: PhysicsShapeIntersectionResult2D)bool cast_motion_typed(parameters: PhysicsShapeQueryParameters2D, result: PhysicsCastMotionResult2D)bool collide_shape_typed(parameters: PhysicsShapeQueryParameters2D, result: PhysicsShapeCollisionResult2D)bool get_rest_info_typed(parameters: PhysicsShapeQueryParameters2D, result: PhysicsRestInfoResult2D)The equivalent methods are added to
PhysicsDirectSpaceState3D.Classes
PhysicsRayIntersectionResult2D/3DPhysicsPointIntersectionResult2D/3DPhysicsShapeIntersectionResult2D/3DPhysicsCastMotionResult2D/3DPhysicsShapeCollisionResult2D/3DPhysicsRestInfoResult2D/3DMotivation
Result objects can be reused to avoid dictionary and array allocations on every call:
The new result objects eliminate the need for manual type declarations and dictionary lookups:
The existing methods require dictionary lookups and explicit type annotations:
This follows the same approach as
body_test_motion()inPhysicsServer2D/3D, which usesPhysicsTestMotionParameters2D/3DandPhysicsTestMotionResult2D/3D.Microbenchmarks and Equivalence Tests
Environment
v4.8.dev.custom_build.1fc32ac76Intel(R) Core(TM) i9-9980HKWindows 10scons -j16 d3d12=no accesskit=no platform=windows target=editor production=yes optimize=speedgodot --headless --path .Script
Closes
PhysicsDirectSpaceState[2D|3D]query methods godot-proposals#15215Related PRs
PhysicsDirectSpaceState3D.intersect_ray_no_alloc#105549test_rayfunction: a version of intersect_ray that has optional result parameter #108680PhysicsDirectSpaceState3D#112433PhysicsDirectSpaceState[2D|3D]query methods #120508