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.

Sign up for GitHub

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

#291 Fixing Pickshapes using farseer #579

Merged
merged 9 commits into from
Oct 18, 2017
Merged

#291 Fixing Pickshapes using farseer #579

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0d53bf4
Merge pull request #1 from AdamsLair/master
Barsonax Oct 8, 2017
e5f5262
Merge pull request #2 from AdamsLair/master
Barsonax Oct 8, 2017
e243a80
Merge pull request #3 from AdamsLair/master
Barsonax Oct 8, 2017
ccb3e57
#291_Farseer: Implemented PickShapes using farseer for circles and po…
Barsonax Oct 13, 2017
e7e308c
#291: cleanup/optimizations/PR requests
Barsonax Oct 14, 2017
7a9c401
#291 Prevent creating boxshape and transform if there are no potentia…
Barsonax Oct 14, 2017
b0b3f49
#291: cleanup
Barsonax Oct 14, 2017
0462b48
PickShapes Bugfix
ilexp Oct 15, 2017
c6f6499
Renamed Local Variable
ilexp Oct 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 51 additions & 33 deletions Source/Core/Duality/Components/Physics/RigidBody.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
using Duality.Cloning;
using Duality.Resources;
using Duality.Properties;
using FarseerPhysics.Common;
using FarseerPhysics.Collision;
using FarseerPhysics.Collision.Shapes;

namespace Duality.Components.Physics
{
Expand Down Expand Up @@ -208,7 +211,7 @@ public float Friction
{
if (this.shapes != null)
{
foreach (var s in this.shapes)
foreach (ShapeInfo s in this.shapes)
s.Friction = value;
}
}
Expand All @@ -225,7 +228,7 @@ public float Restitution
{
if (this.shapes != null)
{
foreach (var s in this.shapes)
foreach (ShapeInfo s in this.shapes)
s.Restitution = value;
}
}
Expand Down Expand Up @@ -772,50 +775,65 @@ public bool PickShapes(Vector2 worldCoord, Vector2 size, List<ShapeInfo> pickedS
{
if (this.body == null) return false;

Vector2 fsWorldCoord = PhysicsUnit.LengthToPhysical * worldCoord;
FarseerPhysics.Collision.AABB fsWorldAABB = new FarseerPhysics.Collision.AABB(fsWorldCoord, PhysicsUnit.LengthToPhysical * (worldCoord + size));

PolygonShape boxShape = new PolygonShape(new Vertices(new List<Vector2>() {
Copy link
Member

Choose a reason for hiding this comment

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

Glad you didn't bother keeping the old fsWorldCoord naming / variable scheme. That was terrible. This one looks definitely cleaner.

PhysicsUnit.LengthToPhysical * worldCoord,
PhysicsUnit.LengthToPhysical * new Vector2(worldCoord.X + size.X, worldCoord.Y),
PhysicsUnit.LengthToPhysical * (worldCoord + size),
PhysicsUnit.LengthToPhysical * new Vector2(worldCoord.X, worldCoord.Y + size.Y) }), 1);
Mat22 rot = new Mat22();
Copy link
Member

Choose a reason for hiding this comment

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

Please add a newline, so the box transform creation is its own block, similar to the body transform retrieval below. Also, maybe move the oldCount and manifold declarations down to directly in front of the loop, so it doesn't get mixes up with the transform creation blocks?

rot.SetIdentity();
Vector2 pos = new Vector2(0, 0);
FarseerPhysics.Common.Transform boxTransform = new FarseerPhysics.Common.Transform(ref pos, ref rot);
Copy link
Member

Choose a reason for hiding this comment

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

Are transform position and rotation public fields? If so, no need to declare and assign rotation and position separately! Just call SetIdentity directly on the box transform rotation field, and assign the position directly without declaring an extra variable. Could also use the default / empty constructor in that case.

(If they are not public fields, disregard my comment.)

Manifold manifold = new Manifold();
int oldCount = pickedShapes.Count;

FarseerPhysics.Common.Transform bodyTransform;
this.body.GetTransform(out bodyTransform);

for (int i = 0; i < this.body.FixtureList.Count; i++)
{
Fixture f = this.body.FixtureList[i];
ShapeInfo s = f.UserData as ShapeInfo;

FarseerPhysics.Collision.AABB fAABB;
FarseerPhysics.Common.Transform transform;
this.body.GetTransform(out transform);
f.Shape.ComputeAABB(out fAABB, ref transform, 0);

if (fsWorldAABB.Contains(ref fAABB))
PolygonShape polygonShape = f.Shape as PolygonShape;
if (polygonShape != null)
{
pickedShapes.Add(s);
continue;
Collision.CollidePolygons(ref manifold, boxShape, ref boxTransform, polygonShape, ref bodyTransform);
if (manifold.PointCount > 0)
{
pickedShapes.Add(s);
continue;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If it's a polygon shape, but does not have any collisions, it will continue checking whether it could be a circle shape next. Consider changing this to an if-else structure, or moving the continue statement out of the point count check!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ye missed that, it should obviously not do the circle check after it already done the polygon one.

Too bad we dont have C# 7 as pattern matching would make this so much more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found out there is a ShapeType enum property on the fixture so I can use that in a switch. Much more readable.

else if (!FarseerPhysics.Collision.AABB.TestOverlap(ref fsWorldAABB, ref fAABB))
continue;

FarseerPhysics.Collision.AABB fAABBIntersect;
fAABBIntersect.LowerBound = Vector2.Max(fAABB.LowerBound, fsWorldAABB.LowerBound);
fAABBIntersect.UpperBound = Vector2.Min(fAABB.UpperBound, fsWorldAABB.UpperBound);

Vector2 fsWorldCoordStep = PhysicsUnit.LengthToPhysical * (new Vector2(MathF.Max(s.AABB.W, 1.0f), MathF.Max(s.AABB.H, 1.0f)) * 0.05f);
Vector2 fsTemp = fAABBIntersect.LowerBound;
do
CircleShape circleShape = f.Shape as CircleShape;
if (circleShape != null)
{
if (f.TestPoint(ref fsTemp))
Collision.CollidePolygonAndCircle(ref manifold, boxShape, ref boxTransform, circleShape, ref bodyTransform);
if (manifold.PointCount > 0)
{
pickedShapes.Add(s);
break;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

See above!

}
}

fsTemp.X += fsWorldCoordStep.X;
if (fsTemp.X > fAABBIntersect.UpperBound.X)
{
fsTemp.X = fAABBIntersect.LowerBound.X;
fsTemp.Y += fsWorldCoordStep.Y;
}
if (fsTemp.Y > fAABBIntersect.UpperBound.Y) break;
} while (true);
// This one is still buggy
//var chainShape = f.Shape as ChainShape;
//if (chainShape != null)
//{

// for (int j = 0; j < chainShape.Vertices.Count - 1; j++)
// {
// var jNext = j + 1;
// if (jNext >= chainShape.Vertices.Count) jNext = 0;
// var edgeShape = new EdgeShape(chainShape.Vertices[j], chainShape.Vertices[jNext]);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that will fix it, but instead of creating a new edge shape every loop iteration, create it once and let the chain shape configure it. There's a GetChildEdge method that is public and I believe also used by Farseers internals, here.

Alternatively, if you want to keep chain shapes our of this PR for now, maybe just add a note to the commented out section regarding this, for future reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was still experimenting with this part. I already tried it with GetChildEdge but got some weird results so have to investigate that some more. Not for this PR though so Ill put a clarifying comment there

// Collision.CollideEdgeAndPolygon(ref manifold, edgeShape, ref bodyTransform, boxShape, ref boxTransform);
// if (manifold.PointCount > 0)
// {
// pickedShapes.Add(s);
// break;
// }
// }
//}
}

return pickedShapes.Count > oldCount;
Expand Down