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

Collision point checker #91354

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

AdeptusForge
Copy link

@AdeptusForge AdeptusForge commented Apr 30, 2024

Gives the scripting API access to 'contains_point' logic for Shape2D's and Collision Shape2D's

@AdeptusForge AdeptusForge requested a review from a team as a code owner April 30, 2024 08:05
@AThousandShips

This comment was marked as outdated.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you need to run clang-format to make sure your code follows the style guide

Also the cases that just are placeholders now should be fixed, this shouldn't be merged in a half finished state IMO

scene/resources/2d/shape_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics/collision_shape_2d.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips requested a review from a team April 30, 2024 08:14
@RedMser
Copy link
Contributor

RedMser commented Apr 30, 2024

Doing something similar for Shape3D would be very appreciated as well! Most of the primitives would likely be trivial as well.

@kleonc
Copy link
Member

kleonc commented Apr 30, 2024

  • You need to generate the docs (and fill them).
  • Since this PR seems to be WIP I suggest converting it into a draft (until it's ready for review).
  • CollisionShape2D::contains_point needs to be clearer about whether it takes local/global point in relation to the CollisionShape2D (maybe it's fine as is and just documenting would be enough).
  • Should Shape2D::contains_point be exposed as well? 🤔 Since it's added anyway...

@AdeptusForge AdeptusForge requested a review from a team as a code owner May 1, 2024 07:46
@AdeptusForge
Copy link
Author

  • You need to generate the docs (and fill them).
  • Since this PR seems to be WIP I suggest converting it into a draft (until it's ready for review).
  • CollisionShape2D::contains_point needs to be clearer about whether it takes local/global point in relation to the CollisionShape2D (maybe it's fine as is and just documenting would be enough).
  • Should Shape2D::contains_point be exposed as well? 🤔 Since it's added anyway...

Docs Generated and filled, contains_point now works globally, and shape2d's don't have their 'contains_point' functions exposed because they only work locally.

Only thing(s) left I reckon is to pass the workflow approval and to perhaps expand it outward to the Shape3D's too.

scene/resources/2d/shape_2d.h Outdated Show resolved Hide resolved
scene/2d/physics/collision_shape_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics/collision_shape_2d.cpp Outdated Show resolved Hide resolved
Comment on lines +212 to +215
if (!collision_object || !Object::cast_to<Node2D>(collision_object->shape_owner_get_owner(owner_id))) {
return false;
}
return shape->contains_point(to_local(p_point));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for such check, there's no need for a CollisionShape2D to be a child of a CollisionObject2D to be able to check whether the given point is within its shape. 🤔

What needs to be checked however is whether there's a valid shape assigned in the first place.

Suggested change
if (!collision_object || !Object::cast_to<Node2D>(collision_object->shape_owner_get_owner(owner_id))) {
return false;
}
return shape->contains_point(to_local(p_point));
return shape.is_valid() && shape->contains_point(to_local(p_point));

Unless we want this method to be working in the "physics world", according to the physics server. In such case checking for collision object existence etc. makes sense. But then we wouldn't want to use to_local etc. because it's using node-cached global transform, which means there could be potential discrepancy between its state and what's the current state within the physics server (depending on when you'd call this method).
So I'd say if it's supposed to be working directly with the data as per physics server then the transforms would also need to be fetched from it.

I don't know if one approach is preferred over the other. Not sure what makes more sense API-wise etc. I think before accepting/merging either of the approaches this would need some input from the @godotengine/physics team.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The math I was doing prior to using to_local would work, given its not node-cached. The validation was originally there for that math.

Personally speaking, running point-checking off the current physics state makes more sense given what it would be used for during runtime, and is more imminently useful. Seems overall better for the end-user.

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.

Checking if Points are within a given CollisionShape
4 participants