Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for easier creation of a convex hull #155

Closed
wants to merge 1 commit into from
Closed

Allow for easier creation of a convex hull #155

wants to merge 1 commit into from

Conversation

Dannie226
Copy link

This is a pull request for allowing easier creation of a convex polyhedron. There is a new ConvexHull class, and all you need to input is an array of Vec3's, and the hull will be computed from there.

Usage

import { ConvexHull, Body, Vec3 } from "cannon-es"
const body = new Body({mass:10});
const shape = new ConvexHull([
    new Vec3( 1, 0, 0),
    new Vec3(-1, 0, 0),
    new Vec3( 0, 1, 0),
    new Vec3( 0,-1, 0),
    new Vec3( 0, 0, 1),
    new Vec3( 0, 0,-1)
]);
body.addShape(shape);

Notes

TS config

The TS config file was changed slightly. I decided I didn't wish to deal with the strict null checks when implementing this, and I don't know of any way to turn off strict null checks for a certain file.

Input -> Output

No points are pruned. All the points inputted get fed into the CANNON.ConvexPolyhedron that this extends. In the future, I could prune unused points and re-index faces, but that would be an issue for a later date.

@isaac-mason
Copy link
Member

isaac-mason commented Jul 30, 2022

Nice PR! 🙂

I have a suggestion for the API - perhaps instead of adding a new shape class, we could add a static method to the ConvexPolyhedron class, e.g. ConvexPolyhedron.fromPoints?

e.g.

export class ConvexPolyhedron extends Shape {
    // ...
    fromPoints(points: Vec3[]): ConvexPolyhedron {
        const faces = QuickHull.createHull(vertices);
        
        return new ConvexPolyhedron({
            vertices,
            faces
        });
    }
    // ...
}

Also, I'll let others speak to this, but I imagine we'll want to keep the strictNullChecks tsconfig as-is. There's also a bit of general code cleanup & linting that could be done in the QuickHull class. Happy to help with this!

@isaac-mason
Copy link
Member

isaac-mason commented Jul 30, 2022

Happy to help however I can with getting something like this merged!

I'm looking to add easy-to-use shape inference hooks into @react-three/cannon using three-to-cannon. Right now the convex hulls that three-to-cannon generates don't support convex-convex collision. From a quick test, it looks like the hulls created with the approach in this PR do.

I could just steal this approach to fix the issue in three-to-cannon, but it'd be great to make cannon-es itself easier to use 🙂

@isaac-mason
Copy link
Member

isaac-mason commented Jul 30, 2022

@marcofugaro (& any others), do you have any thoughts on what the API should look like?

@isaac-mason
Copy link
Member

isaac-mason commented Jul 30, 2022

Oh - @Dannie226 looks like you've already suggested a similar ConvexPolyhedron.createHullFromPoints API. #103 (comment)

Playing catch-up on the discussion 😅

LMK what your thoughts are regardless!

@Dannie226
Copy link
Author

Honestly, I don't care what happens to this. Whether a new class is made or if it's just a static function on the ConvexPolyhedron class. I just wanted to see some improvement made to the ConvexPolyhedron class. That is why I turned off strict null checks, so I could have a quick implementation that works, and then could be fixed later. I would love help with said fixing, but I could do it myself. I also didn't add any *.test.ts for the ConvexHull class, again, part of the "Make it quickly, see how people react, make improvement later," idea. And, another thought I had (this would be in the future if the convex polyhedron from points idea is accepted), is pruning points. Some points that you input into the polyhedron wont be part of the final hull, so go through, prune those points and reindex the faces. Again, not something I would want to work on until I know that it might actually be accepted.

@isaac-mason
Copy link
Member

isaac-mason commented Aug 2, 2022

"Make it quickly, see how people react, make improvement later,"

Makes sense! A good approach 🙂

I don't see any reason we can't merge something if we make sure to:

  • Check some other contributors are happy with the API
  • Keep the change backward-compatible
  • Add sufficient tests
  • Update docs

Development on cannon-es has of course slowed, many of the pmndrs folks are starting to use rapier / @react-three/rapier now. That said, there's nothing stopping us from putting together a high-quality PR, bugging a couple of people to make sure they're happy, and getting something merged!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants