Use connected faces from generated convex hull

[isaac-mason]

• Use connected faces from generated convex hull
  • Fixes Physics simulation stops when generated convex hulls collide #76
  • Generated convex hulls did not have connected faces, which is required for convex-convex collision in cannon-es
  • Using the generated faces in ConvexHull addresses this
• Inspiration taken from discussion in cannon-es repo
  • Simplify ConvexPolyhedron creation pmndrs/cannon-es#103

[donmccurdy]

Thank you! Gathering connected faces from the hull sounds good to me, but had a few questions on how we gather the vertex positions here.

[codecov-commenter]

Codecov Report

Merging #80 (3a13f24) into main (144fd6b) will increase coverage by 1.80%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
+ Coverage   77.94%   79.74%   +1.80%     
==========================================
  Files           3        3              
  Lines        1904     1940      +36     
  Branches       46       45       -1     
==========================================
+ Hits         1484     1547      +63     
+ Misses        420      393      -27     

Impacted Files	Coverage Δ
lib/ConvexHull.js	86.04% <100.00%> (+2.55%)	⬆️
src/index.ts	76.34% <100.00%> (-0.66%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[isaac-mason]

Updated to address comments. This approach reverts back to using setFromObject and now removes vertices not included in the convex hull (interior vertices).

I'll give this another pass shortly - and any feedback would be appreciated 🙂

[donmccurdy]

Sorry for the slow reply here! Just had some questions on collectFacesAndVertices.

[isaac-mason]

Sorry, was busy for a sec! 🙂

Marking as ready for review now. I've done some testing and this PR looks good to go.

As an aside, the generated shapes are now returning less vertices. Take this snippet as an example:

const cylinderGeometry = new CylinderGeometry(0.8, 1, 1, 32, 32);
const cylinderMesh = new Mesh(cylinderGeometry, meshBasicMaterial);
const cylinderMeshResult = threeToCannon(cylinderMesh, {
    type: ShapeType.HULL,
});

With the existing implementation, the generated hull had 1170 vertices. With the new implementation, the generated hull had 213 vertices.

Also, something appears to be indeterministic - the number of faces and vertices vary in both the existing and new implementations on each run. I'm not going to address that in this PR though.

[donmccurdy]

Thank you! Will do another review pass soon.

Also, something appears to be indeterministic - the number of faces and vertices vary in both the existing and new implementations on each run

The vertex positions displaced by small random amounts, here:

three-to-cannon/src/index.ts

Lines 246 to 255 in f570a1c

	// Perturb.
	const eps = 1e-4;
	for (let i = 0; i < geometry.attributes.position.count; i++) {
	geometry.attributes.position.setXYZ(
	i,
	geometry.attributes.position.getX(i) + (Math.random() - 0.5) * eps,
	geometry.attributes.position.getY(i) + (Math.random() - 0.5) * eps,
	geometry.attributes.position.getZ(i) + (Math.random() - 0.5) * eps,
	);
	}

This may be the cause. If I remember correctly, it's a simple way to avoid edge cases we would otherwise need to deal with in the convex hull algorithm, related to co-planar points. Not a problem, for our purposes!

[isaac-mason]

Ah that looks like it, thanks. And yep, not really a big issue when cannon itself isn't deterministic!

[donmccurdy]

Thanks so much @isaac-mason! I've merged the changes and published v4.3.0.

[isaac-mason]

Thanks for the help @donmccurdy!