Simplify flipping in contact manifold generators

[Ralith]

At the cost of one extra dynamic dispatch in get_contact_algorithm, this allows for a dramatic reduction in boilerplate in ContactManifoldGenerator implementations by encoding flipping into the preexisting dynamic dispatch through ContactAlgorithm.

The extra dynamic dispatch could be removed with a more invasive refactoring, but it's not clear that that's justified, so it's left for future work.

WIP because not all manifold generators have been updated to benefit, but I'd like feedback on the idea before proceeding with that tedious work.

[Ralith]

Tests pass fine locally, so I'm guessing the CI is just broken, though the Details link doesn't work...

[sebcrozet]

Hi! Thank you for this PR!

The flip flag is not only there for flipping the arguments of the contact generation method, otherwise the contact generation methods (do_update) themselves would not have to know the value of flip (like there) and there would not be that much boilerplate because of it in the first place.

The internal contact generation method (often named do_update) need to know if its argument were flipped so it can generate the contact manifolds with the right arguments order. For example:

if !flip {
  contact = Contact::new(world1, world2, plane_normal, depth);
  kinematic.set_approx1(f1, local1, approx_plane);
  kinematic.set_approx2(f2, local2, approx_ball);
  kinematic.set_dilation2(ball.radius());
  let _ = manifold.push(contact, kinematic, Point::origin(), proc1, proc2);
} else {
  contact = Contact::new(world2, world1, -plane_normal, depth);
  kinematic.set_approx1(f2, local2, approx_ball);
  kinematic.set_dilation1(ball.radius());
  kinematic.set_approx2(f1, local1, approx_plane);
  let _ = manifold.push(contact, kinematic, Point::origin(), proc2, proc1);
}

The !flip test will ensure that the contact points of the manifolds are output in such a way that the first contact point passed to Contact::new is actually the contact point on the first shape passed as argument to its generate_contacts caller. The same applies to the contact kinematics. These order must be respected so that we are able to associate correctly the colliders and its contacts.

It would be possible to handle this in your FlippedContactManifoldGenerator by flipping all the contacts and contact kinematics after the call to self.0.generate_contacts. But I think this could have a negative performance impact for touching a potentially significant amount of memory just for flipping.

Why do you want to get rid of the flip flag in the first place? Is is just to reduce boilerplate on the contact generator code, or is there another reasons?

[Ralith]

The internal contact generation method (often named do_update) need to know if its argument were flipped so it can generate the contact manifolds with the right arguments order.

Good catch, thanks! The original approach would work fine for shapes that are defined in terms of other shapes, e.g. heightfields, because they never directly generate contacts, but for primitive shapes like triangles it would break down.

It would be possible to handle this in your FlippedContactManifoldGenerator by flipping all the contacts and contact kinematics after the call to self.0.generate_contacts. But I think this could have a negative performance impact for touching a potentially significant amount of memory just for flipping.

I've refactored the change to instead flip contacts and kinematics inside ContactManifold::push, which should avoid significant additional memory access. How does that look to you?

Is is just to reduce boilerplate on the contact generator code

Yep; there is rather a lot, and the duplication it involves is a bit error prone. My hope is this will significantly improve maintainability; e.g. any future changes to the interfaces used inside duplicate code will be much easier with this change.