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

b2EPCollider::Collide method doesn't use m_radius of the shapes #428

Closed
louis-langholtz opened this issue Nov 23, 2016 · 9 comments
Closed

Comments

@louis-langholtz
Copy link

The collision manifold calculating code of the b2EPCollider::Collide method in b2CollideEdge.cpp (as of commit 374664b from Jun 19, 2016) calculates the total radius on line 435 as 2 * b2_polygonRadius. A b2EdgeShape instance or a b2PolygonShape instance can have a different m_radius than b2_polygonRadius however.

For shapes having m_radius settings greater than b2_polygonRadius, this results in a dynamic polygon body in a gravitational world that would otherwise sit atop the edge repeatedly bouncing up and down from the edge.

Everything appears to work correctly if the total radius calculation is changed to be the sum of the m_radius values of each shape.

@MelvMay-Unity
Copy link

MelvMay-Unity commented Nov 24, 2016

In a sort of similar problem, b2CollidePolygon.cpp in function b2CollidePolygons(), the 'm_radius' of each edge is just extended along the edge and tangent to it which is fine for a small radius (such as b2_polygonRadius) however if it's increased then collisions are not detected using a correct radius around the edge vertices (the edges act like boxes effectively). This can been seen when using the Capsule shape contribution which uses an edge with a radius; it just doesn't work correctly if you want to use Polygon/Polygon with a radius that's substantial (visible!).

@louis-langholtz
Copy link
Author

louis-langholtz commented Nov 24, 2016

@MelvMay Thank you for bringing a problem with b2CollidePolygons to our attention! I'm uncertain however what the problem is that you're describing. Are you pointing out how collisions with vertices (as opposed to edges/faces formed by vertex pairs) aren't been treated as rounded corners except for edge-circle collisions (where a e_circles type manifold should really be getting calculated)?

@MelvMay-Unity
Copy link

MelvMay-Unity commented Nov 25, 2016

So I'm the guy who integrates 2D physics into Unity: Various stuff here.
When I was implementing its CapsuleCollider2D my first approach was to use the Box2D capsule contribution and all seemed well until the capsule tried to slide off a platform corner. I noticed that the capsule acted like a box rather than have rounded ends. I ended up writing my own version rather than fixing-up Box2D but the problem with the b2CollidePolygons() still exists.

So, the capsule contribution above highlights the problem as it uses a single edge polygon with a radius as a capsule. This works, as you referred to, when doing edge/circle (e_circles type manifold) however for edge/edge i.e. polygon/polygon (e_faceA & e_faceB type manifold) then obviously b2CollidePolygons() is used. This function results in both edges+radius acting like simple boxes. Sutherland-Hodgman clipping is used here with the segments extended by the total radius of both edges.

This is a shame because the b2Distance() function that uses simplexes works perfectly for a simplex-2 (edge+radius) so overlap checks are fine.

As an example, create two b2PolygonShape boxes with a fairly large and clearly visible radius. Then position them so that the rounded corners don't quite overlap. The existing algorithm detects that as a colliison as if the boxes were not rounded (see attached image where the yellow boxes show the effective area that the intersection test works on). Note that you can do the same with a single box with large radius and a box with the stock b2_polygonRadius and get the same result.

Hopefully, this image will help explain a little better:
polygonpolygonintersection

@louis-langholtz
Copy link
Author

@MelvMay Thanks for the clarification! This is indeed the problem that I was thinking you could have meant. I've actually been working on the collision code to add handling like we're talking about. I have the code changes necessary for the polygon-circle manifold calculating function done and I'm working on the changes for the remaining collision-pair types. I've also created a separate issue (issue #429) for this other problem.

@MelvMay-Unity
Copy link

I'd be interested in seeing your solution.

My code for CapsuleCollider2D inside Unity is a little convoluted and I'll be working on making it more efficient in the new year for Unity 5.6 but I'd be interested to see what you've come up with. I can only say that if it's better than what I've got then, with your permission, I'd have no problem switching to your code, with the proper attribution of course!

As an aside, I did have an 'Edge Radius' in 5.6 however it's been removed due to the problems with poly/poly + radius above. Various other things have stopped me working on this unfortunately but again, it stems from the same issue above.

@louis-langholtz
Copy link
Author

@MelvMay I have the code working now for this in my dev branch (prior to commit dfd51ec). Sorry it took me till now. I've been working on another thing that I wanted to change in the code but got this done now as a side-effect of sorts of trying to make the other change. Here's a screen shot of it:

trianglecapsulecirclesmanifold

This picture shows a circles-type collision occurring between a triangle and a capsule. These are implemented both as polygons (the triangle having 3 vertices and the capsule having 2).

Unfortunately I believe it will take some translation and temporal Kung-fu of my code to recognize where things are getting done for this. When I developed the code, I found it preferable to generalize the polygon shape type to handle two or one vertices rather than create yet another new shape type. And this code is still very much in flux. If you want to see it though, look around in Box2D/Collision/CollideShapes.cpp, specifically from the Manifold CollideShapes(const PolygonShape& shapeA, const Transformation& xfA, const PolygonShape& shapeB, const Transformation& xib) function.

Algorithmically speaking, I achieved this solution essentially by removing the addition of the skin radius total from the incident edge clipping and then checked for the vertices being within that total radius of each other when edge collision wasn't detected. If the vertices were within the total radius of each other, then a 'e_circles' type manifold is returned from the manifold calculating code (instead of the face type manifold that would have formerly been returned).

@MelvMay-Unity
Copy link

MelvMay-Unity commented Jan 9, 2017

I worked on this over xmas and it sounds similar to what I came up with by removing the 'side offsets' part then checking the vertex. I'll check out your code this week. I very much appreciate you getting back to me, thanks!

@erincatto
Copy link
Owner

Hi Guys, sorry I'm late to respond. The radius exists for polygonal shapes only to support continuous collision detection. There is no support for rounded polygons. In my experience it is best to implement capsule collision directly and handle all the different cases with new collision functions. I'm making the change suggested in b2EPCollider::Collide, but that is just for correctness and does not provide new capability. I've also added this comment to b2Shape::m_radius:
/// Radius of a shape. For polygonal shapes this must be b2_polygonRadius. There is no support for
/// making rounded polygons.

Genbox added a commit to Genbox/VelcroPhysics that referenced this issue May 19, 2017
@1vanK
Copy link

1vanK commented Sep 21, 2019

@MelvMay-Unity which way is currently used in Unity? Do you use edge radius or recommended separate capsule shape?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants