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

Add new method for getting multiple rest infos #101676

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DinkeyKing
Copy link
Contributor

@DinkeyKing DinkeyKing commented Jan 17, 2025

Proposal: godotengine/godot-proposals/issues/11609
(Implements this proposal, but with a new method instead: godotengine/godot-proposals/issues/9341)

This PR aims to add a new method for PhysicsDirectSpaceState (2D and 3D), that retrieves shape rest info from all contacts, with the ability to limit the number of results returned. This needs to be implemented in Godod Physics 3D, 2D, and Jolt Physics.

Initially I only implemented this for Godod Physics 3D, so I can get an approval for the idea and the current commited changes.

Currently there is no method for checking a shape's intersections in a space and getting detailed information about all contacts, which I think is an important feature to have. Ideally, this is how the PhysicsDirectSpaceState.get_rest_info method should work, since if you want only one result to be returned, you can limit the number of results returned to 1 with the max_results parameter of this proposed new method, and you get the same result as PhysicsDirectSpaceState.get_rest_info, which can only return one. To avoid breaking compatibility though, this has to be a new method.

@DinkeyKing DinkeyKing requested a review from a team as a code owner January 17, 2025 11:46
@AThousandShips AThousandShips added this to the 4.x milestone Jan 17, 2025
@DinkeyKing DinkeyKing marked this pull request as draft January 17, 2025 12:07
@fire
Copy link
Member

fire commented Jan 17, 2025

There's some whitespace problems.

Try.

python3 install pre-commit
pre-commit run -a

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented Jan 21, 2025

I'm not familiar enough with the code of get_rest_info to judge whether this is the optimal way to obtain multiple results, but on a principle level this seems sound and I can think of a few use cases myself where I would like to have access to this code.

For instance in XR the player can be teleported to a new location and while we check for collisions at that location, other tracked elements like the players hands, and anything the player is holding may end up colliding.
A quick check after teleport to find all colliding elements and create temporary exceptions for them is a way to solve this momentary issue.
There are a few more variations of that scenario that would require this function.

I'm not sold on the name of the new function but I can't currently think of a better one that fits our current approach. This logic should be part of the original get_rest_info function but as discussed on the physics channel in rocket chat, that would be a serious breaking change.

The other long outstanding wish that has come up many times is for functions like this not to return a dictionary, but instead create a data resource class with proper storage and access methods. We then return an array of these objects (they will be Ref<> entries so we also benefit from not copying data around and from automatic memory management on those objects).
It may be an idea to start doing so with this new function instead of perpetuating the dictionary approach.
It could be the first to show a new way forward, but it would also make this the odd duck out. I'm not sure what is smart here.

@DinkeyKing
Copy link
Contributor Author

DinkeyKing commented Jan 21, 2025

The other long outstanding wish that has come up many times is for functions like this not to return a dictionary, but instead create a data resource class with proper storage and access methods. We then return an array of these objects (they will be Ref<> entries so we also benefit from not copying data around and from automatic memory management on those objects). It may be an idea to start doing so with this new function instead of perpetuating the dictionary approach. It could be the first to show a new way forward, but it would also make this the odd duck out. I'm not sure what is smart here.

I support this idea, since, assuming someday the dictionary return types will be replaced, this will save some future time and can be done for this new function without breaking compatibility. Yes, this would make this the odd duck out, but this change should happen someday to the rest of the query functions.

So, I could create the ShapeRestInfo3D and ShapeRestInfo2D classes, and this function could return an array of those. Another option is that the ShapeRestInfoXD classes contain all the contact datas, and similiar to MotionTestResult3D, the getters could be indexed to retrieve data of a given contact. The latter one makes more sense to me, since I interpret 'shape rest info' as the collection of all contacts' data of a "resting" shape, but the currently defined ShapeRestInfo struct only contains one contact's data, so I'm not sure.

... Also, I noticed the collision depth is not currently exposed by get_rest_info, which I think would be a useful addition to the returned collision data.

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.

4 participants