#291 Fixing Pickshapes using farseer #579

Barsonax · 2017-10-13T19:23:45Z

No description provided.

update master

Explicit C# Language Version

Added Simple RigidBody Physics Test

…lygons

ilexp · 2017-10-13T21:50:25Z

Since there's a ToDo comment, is this still WiP? Let me know when it's ready for review 👍

Barsonax · 2017-10-13T21:53:21Z

The chainshape part is buggy so I left that out for now. Just did the circle and poly shapes for now. The chainshapes were not working before either so its not breaking anything.

ilexp · 2017-10-13T21:58:45Z

I'll take a look as soon as I get around to it. Since polygons and circles only would be equal to the old functionality, we could merge this PR even without the chain shape checks after review. Should be fine as long as we don't introduce a regression.

ilexp

Perf-wise, maybe provide a private (!) version of the fully parameterized instance PickShapes method that takes a Farseer polygon shape instance and (by-ref) transform? You could call this method from the regular public overload, and the static PickShapesGlobal method: No added benefit for the first one, but the second one would only create and configure the shape once.

Final question: How's performance compared to before? (And which test setup did you use? Good to know for context.)

So far I like the Farseer version of this fix much better, very compact and self-contained. Nice work on this 👍

ilexp · 2017-10-14T07:25:33Z

Source/Core/Duality/Components/Physics/RigidBody.cs

+					{
+						pickedShapes.Add(s);
+						continue;
+					}
 				}


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!

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.

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

ilexp · 2017-10-14T07:26:35Z

Source/Core/Duality/Components/Physics/RigidBody.cs

-			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>() {


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

ilexp · 2017-10-14T07:28:18Z

Source/Core/Duality/Components/Physics/RigidBody.cs

+				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();


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?

ilexp · 2017-10-14T07:30:10Z

Source/Core/Duality/Components/Physics/RigidBody.cs

+			Mat22 rot = new Mat22();
+			rot.SetIdentity();
+			Vector2 pos = new Vector2(0, 0);
+			FarseerPhysics.Common.Transform boxTransform = new FarseerPhysics.Common.Transform(ref pos, ref rot);


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.)

ilexp · 2017-10-14T07:31:15Z

Source/Core/Duality/Components/Physics/RigidBody.cs

 					{
 						pickedShapes.Add(s);
-						break;
+						continue;


ilexp · 2017-10-14T07:34:16Z

Source/Core/Duality/Components/Physics/RigidBody.cs

+				//	{
+				//		var jNext = j + 1;
+				//		if (jNext >= chainShape.Vertices.Count) jNext = 0;
+				//		var edgeShape = new EdgeShape(chainShape.Vertices[j], chainShape.Vertices[jNext]);


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.

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

Barsonax · 2017-10-14T09:52:11Z

When I get back home I will give you the setup I used for measuring performance. So far the performance was slower than my own implementation.

Good point about the private parameterized instance method I will add that.

…l bodies.

Barsonax · 2017-10-14T17:50:59Z

Tested the performance and compared to my own implementation the farseer is slower. How much depends on the shape and where the collision is:
-Circles are ~2x as slow. The performance is pretty much constant no matter where you touch the circle
-Simple polygons (3 vertices). are 1.2x-3x as slow. The farseer version seems to have a constant performance no matter where you touch the polygon while in my own implementation it matters what edge or vertice you touch.
-Complex polygons (14 vertices). The farseer version seems to again have a constant performance no matter where you touch the polygon. The performance of my own implementation seems to vary as much as 10x depending on where you touch the polygon. Roughly speaking they have the same performance but farseer is more balanced. There seems to be a trend that the more complex polygons seem to favor farseer.

Now the question is is it worth sacrificing some performance to avoid duplicate collision code? Note that my own implementation is not optimized yet. I think it should be possible to operate directly on the farseer data now I understand farseer a little better for instance.

Tbh I don't think anyone will be running in performance issues anytime soon with PickShapes as I had to call it quite a lot of times before you could start noticing it.

Attached is the Data and Source folder I used when testing this. To use:

Copy the Data and Source folder to the duality/Build/Output
Build the project in the branch you want to test (this one or the other one with my implementation)
Debug the ProjectPlugins solution
The time it took to perform the PickShapesGlobal is printed to the output. Note that PickShapesGlobal is called 50000 times per update in order to make the performance more visible.

PickshapesTest.zip

Note that there are currently no unit tests in this branch. I could copy some over from #577.

ilexp · 2017-10-15T10:14:04Z

Source/Core/Duality/Components/Physics/RigidBody.cs

+			FarseerPhysics.Common.Transform boxTransform = new FarseerPhysics.Common.Transform();
+			boxTransform.SetIdentity();
+
+			return PickShapes(boxShape, boxTransform, pickedShapes);


Please use this. for instance member access as well. Would merge anyway since it's really just a detail, so whoever of us gets to it first can do it.

ilexp · 2017-10-15T10:17:19Z

Changes look good so far!

Now the question is is it worth sacrificing some performance to avoid duplicate collision code?

Good question! In this case, I think the answer should be "yes, if it's not too severe", but it should definitely be on a list of potential optimization for when issue #570 is done.

Will take a closer look at performance myself before merging - if it doesn't have a serious impact except in benchmarks, I think it should be fine. Will have to see how close it is to real-world impact.

#FIX: Fixed a bug where complex polygon shapes would be returned multiple times in the result list of PickShapes when intersecting more than one convex segment.

#CHANGE: Renamed local variable from single-letter indexer style to a descriptive name.

ilexp · 2017-10-15T11:51:38Z

Note that there are currently no unit tests in this branch. I could copy some over from #577.

Took a brief look and probably wouldn't do a merge for them as-is, so I'd keep this PR separate for now so we can get it merged. Tests for this would be great, but I'd probably do a draft / design pass in the discussion of #533 first before implementing it.

Found a small bug where a ShapeInfo was sometimes added multiple times to the result list for complex polygons and fixed it.

Looked into performance and getting between 30ms and 350ms for 50000 tests / up to 0.007 ms per test on my machine depending on which bodies are checked - not great, but not catastrophic either. I do see some optimization potential on the Farseer side of things, but that's an issue for another day.

A bit busy right now, so will have to do it later, but I'd say this is a merge. Nice work 👍

ToDo:

After the merge, add a note to Fix the Rect Version of RigidBody.PickShapesGlobal #291 stating that there's still the chain version to do, and link to the code segment in question.
Add a note to Write RigidBody / Physics Unit Tests #533 for potential to add unit tests for this as well and briefly discuss the where / which / how.

Barsonax · 2017-10-15T13:12:38Z

Hmm well the contains check also adds overhead in all cases atm. Not sure what would be faster since that will also depend on how much shapes you are picking. This would need benchmarking.

Changing the signature would break compatibility so definitely not for v2.

ilexp · 2017-10-15T21:06:29Z

Hmm well the contains check also adds overhead in all cases atm. Not sure what would be faster since that will also depend on how much shapes you are picking. This would need benchmarking.

Yep, but also different kinds of overhead! Perf right now, but design in the other case, e.g. forcing the callsite to use a more complex collection. Totally agree on the perf consideration though, if it shows up on the profiler at some point, this will have to change.

Barsonax and others added 4 commits October 8, 2017 10:48

Merge pull request #1 from AdamsLair/master

0d53bf4

update master

Merge pull request #2 from AdamsLair/master

e5f5262

Explicit C# Language Version

Merge pull request #3 from AdamsLair/master

e243a80

Added Simple RigidBody Physics Test

#291_Farseer: Implemented PickShapes using farseer for circles and po…

ccb3e57

…lygons

Barsonax mentioned this pull request Oct 13, 2017

#291 Fixing pickshapes code #577

Closed

ilexp requested changes Oct 14, 2017

View reviewed changes

Barsonax added 3 commits October 14, 2017 19:09

#291: cleanup/optimizations/PR requests

e7e308c

#291 Prevent creating boxshape and transform if there are no potentia…

7a9c401

…l bodies.

#291: cleanup

b0b3f49

ilexp reviewed Oct 15, 2017

View reviewed changes

ilexp approved these changes Oct 15, 2017

View reviewed changes

ilexp added 2 commits October 15, 2017 13:16

PickShapes Bugfix

0462b48

#FIX: Fixed a bug where complex polygon shapes would be returned multiple times in the result list of PickShapes when intersecting more than one convex segment.

Renamed Local Variable

c6f6499

#CHANGE: Renamed local variable from single-letter indexer style to a descriptive name.

ilexp merged commit c8cecf1 into AdamsLair:master Oct 18, 2017

This was referenced Oct 18, 2017

Fix the Rect Version of RigidBody.PickShapesGlobal #291

Open

Write RigidBody / Physics Unit Tests #533

Open

ilexp added this to the General milestone Oct 18, 2017

ilexp added Bug It's broken and should be fixed Cleanup Improving form, keeping function Core Area: Duality runtime or launcher labels Oct 18, 2017

Barsonax deleted the #291_Farseer branch October 31, 2018 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#291 Fixing Pickshapes using farseer #579

#291 Fixing Pickshapes using farseer #579

Barsonax commented Oct 13, 2017

ilexp commented Oct 13, 2017

Barsonax commented Oct 13, 2017 •

edited

Loading

ilexp commented Oct 13, 2017

ilexp left a comment

ilexp Oct 14, 2017

Barsonax Oct 14, 2017

Barsonax Oct 14, 2017

ilexp Oct 14, 2017

ilexp Oct 14, 2017

ilexp Oct 14, 2017

ilexp Oct 14, 2017

ilexp Oct 14, 2017

Barsonax Oct 14, 2017

Barsonax commented Oct 14, 2017

Barsonax commented Oct 14, 2017 •

edited

Loading

ilexp Oct 15, 2017

ilexp commented Oct 15, 2017

ilexp commented Oct 15, 2017

Barsonax commented Oct 15, 2017

ilexp commented Oct 15, 2017

#291 Fixing Pickshapes using farseer #579

#291 Fixing Pickshapes using farseer #579

Conversation

Barsonax commented Oct 13, 2017

ilexp commented Oct 13, 2017

Barsonax commented Oct 13, 2017 • edited Loading

ilexp commented Oct 13, 2017

ilexp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barsonax commented Oct 14, 2017

Barsonax commented Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

ilexp commented Oct 15, 2017

ilexp commented Oct 15, 2017

ToDo:

Barsonax commented Oct 15, 2017

ilexp commented Oct 15, 2017

Barsonax commented Oct 13, 2017 •

edited

Loading

Barsonax commented Oct 14, 2017 •

edited

Loading