three-to-cannon icon indicating copy to clipboard operation
three-to-cannon copied to clipboard

Use connected faces from generated convex hull

Open isaac-mason opened this issue 1 year ago • 2 comments

  • Use connected faces from generated convex hull
    • Fixes #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
    • https://github.com/pmndrs/cannon-es/issues/103

isaac-mason avatar Aug 15 '22 10:08 isaac-mason

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%) :arrow_up:
src/index.ts 76.34% <100.00%> (-0.66%) :arrow_down:

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

codecov-commenter avatar Aug 18 '22 11:08 codecov-commenter

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 🙂

isaac-mason avatar Aug 18 '22 11:08 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.

isaac-mason avatar Oct 11 '22 14:10 isaac-mason

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:

https://github.com/donmccurdy/three-to-cannon/blob/f570a1c8391269636c045672b6258490d624137a/src/index.ts#L246-L255

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!

donmccurdy avatar Oct 14 '22 20:10 donmccurdy

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

isaac-mason avatar Oct 16 '22 22:10 isaac-mason

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

donmccurdy avatar Oct 25 '22 22:10 donmccurdy

Thanks for the help @donmccurdy!

isaac-mason avatar Oct 26 '22 08:10 isaac-mason