Enforce MustIncludeC in GetClosestPointOnTriangle

[bttner]

This PR is referring to the discussion #740 (reply in thread) and fixes the described problem.

Note that we can overwrite the variable ab as the function returns after testing the edge BC and, therefore, its last use only involves testing the edge AB.

[CLAassistant]

All committers have signed the CLA.

[jrouwe]

I've taken a quick look at the failing unit tests. These don't mean that the collision detection code is wrong but it means that the collision response changed enough that the penetration between a simulated box and the floor is slightly larger than before. I will take a look at it, but it is going to take me some time to verify the new behavior.

[bttner]

Unfortunately, the Unit test "TestLinearCastBoxVsInactiveDiscreteBoxAngled" fails.

When these lines

JoltPhysics/UnitTests/Physics/MotionQualityLinearCastTests.cpp

Lines 306 to 310 in b9bdabe

	CHECK_APPROX_EQUAL(box1.GetLinearVelocity(), new_velocity1, 1.0e-4f);
	CHECK_APPROX_EQUAL(box1.GetAngularVelocity(), Vec3::sZero(), 2.0e-4f);
	CHECK_APPROX_EQUAL(box2.GetPosition(), RVec3::sZero());
	CHECK_APPROX_EQUAL(box2.GetLinearVelocity(), new_velocity2, 1.0e-4f);
	CHECK_APPROX_EQUAL(box2.GetAngularVelocity(), Vec3::sZero(), 2.0e-4f);

become

CHECK_APPROX_EQUAL(box1.GetLinearVelocity(), new_velocity1, 1.0e-3f);
CHECK_APPROX_EQUAL(box1.GetAngularVelocity(), Vec3::sZero(), 2.0e-3f);
CHECK_APPROX_EQUAL(box2.GetPosition(), RVec3::sZero());
CHECK_APPROX_EQUAL(box2.GetLinearVelocity(), new_velocity2, 1.0e-3f);
CHECK_APPROX_EQUAL(box2.GetAngularVelocity(), Vec3::sZero(), 2.0e-3f);

then the mentioned test passes. So the accuracy is worse by a factor of 10.

What is odd is that this test calls GetClosestPointOnTriangle with MustIncludeC = false. I am now investigating why that is. The test should not fail.

[bttner]

The problem is the following.

In the mentioned test MustIncludeC is false and swap_ac is true. The distance to edge AB is (almost) the same as to Edge AC.

• Before the change, edge AC is tested before edge AB (the block with comment // Edge BC), which is why edge AC is chosen.
• After the change, edge AB is tested before edge AC, which is why edge AB is chosen.

In #740 (reply in thread), we also discussed the impact of swapping A and C on the dot products.

• Before the change, I get (0.1, 0, 0) as the closest point in both calculations in edge AB (the block with comment // Edge BC) and AC.
• After the change (thus, discarding swap_ac), I get (0.1, 0, 5.96e-08) for AB and (0.1, 0, 0) for AC.

Now the big question is, which value for AB is correct? If I base my opinion on the theoretical background of GJK, that is, if I consider that GJK should improve every iteration until it converges (in an ideal world), the point (0.1, 0, 5.96e-08) seems more reasonable for AB than (0.1, 0, 0). So, the impact of swapping AC might actually be negative on the dot products. What do you think?

Edit: changed my reasoning

[bttner]

I changed the order of testing the edges, i.e., edge AC and BC are getting prioritized now. This change should pass all unit tests.

Moreover, even if MustIncludeC is false, the closest point can be expected to be most of the time on the edge AC, BC, or inside. So it makes sense to test these cases first.

[jrouwe]

I've been testing with this change and there was one issue with it: If we test AC before A and the closest point is A, we will find that v == 0 and calculate best_dist_sq = |A|^2. If we then later test vertex A we find !(a_len_sq < best_dist_sq) (because they're equal) so while the returned position is correct, the returned outSet is not (it returns 0b101 instead of 0b001). For CastShape this results in more vertices being used in GJK and a slight slowdown.

Note that I applied a fix for this that still keeps the unit tests working.

[jrouwe]

Thanks for your contribution btw!