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 ShapeCast2D node #54803

Merged
merged 2 commits into from
Nov 12, 2021
Merged

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Nov 9, 2021

Implements the 2D part of godotengine/godot-proposals#710.

image

Ported from Goost to Godot 4.0, implementation mostly equivalent to https://github.com/goostengine/goost/blob/46379f65c5bc9324dc0be1cb9c13f34c22b681b2/scene/physics/2d/shape_cast_2d.cpp, except collisions are processed regardless whether shape cast is already colliding or not.

image

Drawing could be improved, but perhaps that something @pouleyKetchoupp could build upon, as well as implementing the 3D counterpart based on this.

Test project:
shape-cast-2d.zip

(left mouse button click to set shape cast global position, right click to set target position)

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

There are a few details to fix before merging, but otherwise it looks great. Thanks!

A few more things can be improved but they don't have to be part of this PR. I'm leaving a list here for the record:

  • Improve debug draw by making a convex hull around the whole path
  • Add a shape transform property to allow rotation/scale without changing the cast
  • Add an equivalent node for 3D (still needs some tests to decide how to draw it)

scene/2d/shape_cast_2d.cpp Outdated Show resolved Hide resolved
scene/2d/shape_cast_2d.cpp Outdated Show resolved Hide resolved
scene/2d/shape_cast_2d.cpp Show resolved Hide resolved
scene/2d/shape_cast_2d.cpp Outdated Show resolved Hide resolved
scene/2d/shape_cast_2d.h Outdated Show resolved Hide resolved
@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 10, 2021

There are a few details to fix before merging, but otherwise it looks great. Thanks!

Done!

Improve debug draw by making a convex hull around the whole path

I have no idea what kind of drawing capabilities there are in 4.x, but perhaps there's a way to simply use an appropriate blending mode. Otherwise convex hull will require to refactor Shape2D.draw() into Shape2D.get_points() + draw().

Add a shape transform property to allow rotation/scale without changing the cast

I've tried to implement something like this myself but couldn't find a way to do this on the front-end side, I think shape transform should be done during cast_motion()/rest_info(), especially if there's a possibility to interpolate rotation between origin and target positions to simulate angular motion, not just linear, but this is probably too much for Godot (?)

In either case, yeah it would be nice to merge this sooner so that we don't lose motivation to work on all this stuff!

@Xrayez Xrayez requested review from pouleyKetchoupp and removed request for a team November 10, 2021 14:53
@fire
Copy link
Member

fire commented Nov 10, 2021

There is a deluanlay tetrahedralize for 3d points can the 2d version be used?

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 10, 2021

There's Geometry2D.convex_hull() or similar, so it should be possible already. But we'd need to either expose Shape2D points for each shape, or hardcode point generation for each shape locally in ShapeCast2D. I'd prefer to expose Shape2D.get_points() in order to get geometrical representation of the shape via polygon vertices, because Shape2D.draw() already generates points internally for them to be drawn, so it's a matter of refactoring existing code.

We could keep Shape.get_points() for internal use, but there are several proposals at Godot that could benefit from this, such as godotengine/godot-proposals#3495 or godotengine/godot-proposals#1925.

But this is something which touches core parts of the engine, so it's kind of out of scope for this PR, unless you think that implementing Shape2D.get_points() for each shape makes sense.

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

I've left more comments after testing the ShapeCast2D node.

It generally works well, but there are some inconsistencies between what the documentation states and the results you get, so I've tried to suggest minimal changes to make it consistent until this node can be improved along with the physics API.

scene/2d/shape_cast_2d.cpp Show resolved Hide resolved
bool intersected = true;
while (intersected && result.size() < max_results) {
PhysicsDirectSpaceState2D::ShapeRestInfo info;
intersected = dss->rest_info(params, &info);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using rest_info() is not actually giving the results you're expecting. It's meant for contact detection and gives you the deepest collision point, so depending on the motion length and shape size you can get different results, not necessarily the nearest collision along the path.

Unfortunately there's currently no good API that gives you all the information you need for all collisions.

What I would suggest for now is:

  • You can keep using cast_motion() for the safe/unsafe distances, and later the physics server API can be improved to provide more information along with it, so you will be able to get more details about the closest collision.

  • For the multiple collision results, you can call intersect_shape() just once instead of rest_info() multiple times to get information for all collision (but not contact points or normals). Later I think the best would be to use collide_shape() and modify the physics server API to provide more information than just contact positions.

Copy link
Contributor Author

@Xrayez Xrayez Nov 10, 2021

Choose a reason for hiding this comment

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

I've found rest_info() to be the best compromise in 3.x, because:

  1. You can detect multiple bodies along the cast given detected bodies get subsequently excluded from intersection:

image

  1. It does return useful information such as normal (and metadata which is removed in 4.x due to alternatives), but apparently incorrect. 😮

To be honest, I've mostly used this as a continuous collision detection area in 3.x, so yeah I get what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the position and normal would be good to have, but given the current API it just returns collisions in a random order and the positions are not correct so it's not really usable in common cases.

What you're trying to do makes sense. I would just rather wait for making the proper changes in the physics API before exposing the extra information to avoid confusing users, since it's not working as intended for now.

Copy link
Contributor Author

@Xrayez Xrayez Nov 10, 2021

Choose a reason for hiding this comment

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

What if we keep rest_info() when motion == Vector2(), and use intersect_shape() when motion != Vector2()? Would that be a good compromise? If someone from contributors would want to port this PR to 3.x, I'd need this to be able to get normal/contact info in static collision case, I wouldn't want to create a different node just for this purpose in Goost.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that could be a good compromise as long as it's clear and well documented. It does sound like a better solution than using a separate node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was all wrong from the start due to porting error, see #54803 (comment).

[b]Note:[/b] [code]enabled == true[/code] is not required for this to work.
</description>
</method>
<method name="get_closest_collider" qualifiers="const">
Copy link
Contributor

Choose a reason for hiding this comment

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

The get_closest_ functions are not going to return correct results until some changes are made in the physics server API, so better remove them for now (except for safe/unsafe distances).

doc/classes/ShapeCast2D.xml Outdated Show resolved Hide resolved
doc/classes/ShapeCast2D.xml Outdated Show resolved Hide resolved
doc/classes/ShapeCast2D.xml Outdated Show resolved Hide resolved
scene/2d/shape_cast_2d.cpp Outdated Show resolved Hide resolved
@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 10, 2021

Unfortunately there's currently no good API that gives you all the information you need for all collisions.

By the way, given above suggestions, I'm fine for this PR to be postponed and be used as a reference point until physics backend is improved to match the proposed API, to be honest. I wouldn't want to remove those closest_* methods and other features just for them to be implemented again, nor anybody else would want to do double work, I presume.

I'd personally like to see complete feature set, so I'm fine to wait, or somebody else could build upon my work.

@pouleyKetchoupp
Copy link
Contributor

@Xrayez Ok, that makes sense! What I can propose is to make the adjustments to the physics server API in the next two weeks or so, and then you can update this PR to have the basic functionality work as intended and we can merge it.

After that, other contributors will be able to help with adding functionalities and improving the rendering.

And for now, this PR can be in draft mode until the API is ready.

@Xrayez Xrayez marked this pull request as draft November 10, 2021 19:39
@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 11, 2021

@pouleyKetchoupp @djrain so I thought I was losing my mind while porting the node to Godot 4.0, but I've checked the implementation in Goost I've made a year ago for Godot 3.x and figured that:

  • rest_info() does not take into account motion (deliberately set to Vector2() in order to get static collisions, not with motion).
  • Therefore, I manually translate the original global transform to the point of unsafe motion fraction like this in 3.x right after cast_motion():
global_transform.set_origin(global_transform.get_origin() + to * collision_unsafe_fraction);

Only then I use rest_info(), but only at the point of impact (or when the shape is already stuck or target_position == Vector2(0, 0)), and I'm not using intersect_shape() because unlike rest_info(), it doesn't return normal/contact info etc.

I have now properly ported implementation from 3.x to 4.x, it should work alright now (hopefully), sorry for confusion as I got confused by current API in Godot 4.0 while porting this. I have updated the test project according to suggested renames as well.

Current code is compatible with Godot 3.x feature-wise, so if you think the result is satisfactory and decide to merge this now, it should be possible to just copy-paste existing 3.x implementation I have in Goost, if users express interest in this in Godot 3.x. If not, I've made this PR to work as intended, at least... 😅

@Xrayez Xrayez marked this pull request as ready for review November 11, 2021 15:49
@pouleyKetchoupp
Copy link
Contributor

Ok, the contact positions seem to work, although now it always only returns one collision result even if the shape cast goes through several bodies/areas. In order to get exactly the intended result, maybe you would need to use cast_motion() again for each iteration? Otherwise it just tests for rest info several times at the first hit position.

Here's the project I'm using for tests:
shape-cast-4.0.zip

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 12, 2021

The "intended" result I've shown at #54803 (comment) wasn't actually intended, sorry!

The intended result was to hit a single object at the time of impact, but potentially return multiple collisions at point of impact in case there are multiple bodies there. This is consistent with RayCast2D, which doesn't support detecting multiple collisions along the cast motion, see proposal #25695.

The "closest" object in this case would be the one that first intersects with rest_info() in static case (if multiple bodies are also detected at that position).

If you'll say that rest_info() wouldn't return closest object at static collision in some cases, then I think get_closest_* methods could be renamed to get_first_impact_collision() or something like that.

Due to above, and if #25695 is implemented, then RayCast2D would also need to introduce get_closest_* methods in order to differentiate those bodies which are detected first, and bodies that are detected subsequently, see also reduz proposed solution: #14608 (comment) (which I already do in the case of ShapeCast2D, but only in order to detect multiple bodies at time of impact, and not multiple bodies along the cast motion, which I feel like asks for another pull requests, since RayCast2D doesn't support this either at the moment).

That said, it's always possible to move ShapeCast2D/RayCast2D at the point of impact after the collision via script, and repeat the process.

Also, in your test project, ShapeCast2D was configured to collide with Area2D, while RayCast2D wasn't, which lead to different result.

@pouleyKetchoupp
Copy link
Contributor

The intended result was to hit a single object at the time of impact, but potentially return multiple collisions at point of impact in case there are multiple bodies there. This is consistent with RayCast2D, which doesn't support detecting multiple collisions along the cast motion, see proposal #25695.

Alright, makes sense to make it consistent with RayCast2D, and add extra features to both later. It all seems to work well, the only thing I noticed is if you increase the margin, rest info can fail. But I think I've found where the bug is in the physics server, I'll fix it in a separate PR (rest info can fail to return anything if margin is too high and there's no motion).

The "closest" object in this case would be the one that first intersects with rest_info() in static case (if multiple bodies are also detected at that position).

If you'll say that rest_info() wouldn't return closest object at static collision in some cases, then I think get_closest_* methods could be renamed to get_first_impact_collision() or something like that.

Honestly, I would rather remove the get_closest_* methods because whatever their name is it seems they might just add some confusion. On the other hand, the get_* methods that use a collision index could be clarified with some wording like one of the collisions at impact.

I'm going to add more review comments for some details too, I think it's almost ready but I'd like to make sure the documentation is clear.

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 12, 2021

Honestly, I would rather remove the get_closest_* methods because whatever their name is it seems they might just add some confusion. On the other hand, the get_* methods that use a collision index could be clarified with some wording like one of the collisions at impact.

That's good enough for me as long as multiple collisions are able to be detected at time of impact with contact info such as normals (that will be mostly useful for cases when shape cast starts in a colliding/stuck state at origin against other collision objects).

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Here are a few last things that can be made clearer in the documentation. The code itself looks fine to me!

doc/classes/ShapeCast2D.xml Outdated Show resolved Hide resolved
doc/classes/ShapeCast2D.xml Outdated Show resolved Hide resolved
doc/classes/ShapeCast2D.xml Outdated Show resolved Hide resolved
@djrain
Copy link

djrain commented Nov 12, 2021

Glad I read this before going any further with my gdscript shapecasting system 😅

I have something working well enough for now, so I'm wondering @pouleyKetchoupp what fixes/changes you currently have in mind for the physics API and when/where they might actually be merged (if you have any idea)?

Andrii Doroshenko (Xrayez) added 2 commits November 12, 2021 21:29
The physics API cannot provide needed functionality to ensure the correct behavior, which might lead to confusion (see `rest_info()`).

However `get_closest_collision_safe/unsafe_fraction()` methods are not removed, because they return correct result from `cast_motion()`.
@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 12, 2021

Honestly, I would rather remove the get_closest_* methods because whatever their name is it seems they might just add some confusion. On the other hand, the get_* methods that use a collision index could be clarified with some wording like one of the collisions at impact.

That's good enough for me as long as multiple collisions are able to be detected at time of impact with contact info such as normals (that will be mostly useful for cases when shape cast starts in a colliding/stuck state at origin against other collision objects).

I have removed those methods now and pushed a separate commit to document this, because that's what differs from Goost implementation now, and that's something to keep in mind when new/updated API is planned for physics regarding dynamic collisions in general.

@pouleyKetchoupp
Copy link
Contributor

I have something working well enough for now, so I'm wondering @pouleyKetchoupp what fixes/changes you currently have in mind for the physics API and when/where they might actually be merged (if you have any idea)?

I was thinking of making the different shape functions more consistent with each other, and allow all of them to return proper collision information (for example cast_motion currently returns just the fraction and maybe some others can be improved too).

Given the state of this PR, it's not really needed anymore, but it might help with optimizing the queries, especially if we want to support getting different results along the motion later.

Since it might include some compatibility breaking changes, I'll try to make a PR in the next few weeks, but I can't really give you a precise date.

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Looks good to go!

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.

7 participants