pmndrs / use-cannon

👋💣 physics based hooks for @react-three/fiber
https://cannon.pmnd.rs
2.74k stars 154 forks source link

Make usePlane work like Three's Plane #295

Closed krispya closed 2 years ago

krispya commented 2 years ago

When using use-cannon it took me a bit longer than I'd like to admit to realize that usePlane created an infinite plane and did not respect the args unlike the other shapes I was creating like useBox and useSphere. What this PR does is use the Cannon Box type to create a plane that has dimensions like a Three plane allowing the args to be spread into both as expected. It also creates a new hook called useInfinitePlane that uses the Plane type and is a little more explicit about what you will get.

Now the issues! After digging through the cannon-es code I understand why their Plane object works like this. It is a dumb object that assumes itself to have infinite mass and that everything underneath it is solid. Because of this they are able to take shortcuts with collision calculations so collisions tend to be much more accurate with a Cannon Plane than a Cannon Box. For example, take a look at the Convex Polyhedron example and you can see how these complex shapes have trouble colliding with the Cannon Box. Replace it with useInfinitePlane and these issues go away. But this isn't the only place this inaccurate collision detection is a problem. Take a look at the Monday Morning example and how everything has trouble colliding with the table, even the coffee cup.

For reference, here is the plane-convex collision code: https://github.com/pmndrs/cannon-es/blob/28248468d27496ff3b029aa141b0a930505f8ac3/src/world/Narrowphase.ts#L1058

And here is the box-convex collision code: https://github.com/pmndrs/cannon-es/blob/28248468d27496ff3b029aa141b0a930505f8ac3/src/world/Narrowphase.ts#L1125

Which is really just pointing the convex-convex collision code that gives us our problem: https://github.com/pmndrs/cannon-es/blob/28248468d27496ff3b029aa141b0a930505f8ac3/src/world/Narrowphase.ts#L767

So I think this PR is still worth considering despite this drawback as this collision code needs to get reconciled anyway. The ultimate goal is to have expectations for the API align with users.

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/use-cannon/BVZBMbLzPmcyJzon4Uq4ZuqvFxLd
✅ Preview: https://use-cannon-git-fork-krispya-exp-infinite-plane-pmndrs.vercel.app

krispya commented 2 years ago

I tested this some more and I couldn't reproduce the polyhedron collision issue with boxes in my cannon-es environment. This might be a different bug all together. I'll have to investigate more. The only drawback I saw from this is that Cannon Box being used to make the plane needs to have a minimum thickness depending on the number of iterations to stop clipping through. This is easy to test with the heap example. The benefit of the Cannon Plane is that it assumes everything is solid ground below it so thickness doesn't matter. But these kinds of considerations might just be what someone needs to be aware of to use a physics library effectively. With that in mind it might even help to call it useFloor instead of useInfinitePlane. Okay, I have written enough for discussion. Look forward to seeing what people think.

bjornstar commented 2 years ago

I'm not entirely sold on the changing the plane API as it's a direct reflection of the Cannon API. I understand that it causes some confusion when coming from three.js.

I'd like to hear what other people have to say.

In the meantime, could you remove the changes to the lock files?

drcmda commented 2 years ago

i think infinite planes can be useful but its almost always a finite plane that corresponds to my mesh that i want. though i agree, this should probably fixed in cannon first, and even if not, we'd need two hooks: usePlane and useFinitePlane (for backward compat i think it's better like that).

stockhuman commented 2 years ago

Thanks for this, and I agree that it's not intuitively obvious that Three's finite, two dimensional shape can't map to a 3D physics sim. usePlane as it is now behaves like what you expect your physics professor means when saying "assume a plane", but things like the debugger representation give a very different impression given the arbitrarily small (10x10) values it uses to show that infinity.

The hook as you note, with all its inherent shortcuts, is really useful when used for a "ground" object with fast computation and I think it's well-named as is. I'd support Paul's useFinitePlane, or even something that suggests it uses a box internally, if shape compatibility were ever to change.

krispya commented 2 years ago

i think infinite planes can be useful but its almost always a finite plane that corresponds to my mesh that i want. though i agree, this should probably fixed in cannon first, and even if not, we'd need two hooks: usePlane and useFinitePlane (for backward compat i think it's better like that).

I can look at making a finite plane in cannon-es but conceptually I think it has problems. Right now it represents a kind of barrier, so there is no concept of an underside collision. If I make the plane finite so that a ball can roll off it, then it's unclear what to do once the ball gets under the plane except to just treat it like a box now.

Conceptually, we can create a box with extremely small depth but then we start hitting the limitations of our quick integration method where small objects clip through for a given dt. I'm trying to read up on how other physics engines are written so if I stumble onto something I'll update people, but math isn't my strong suit so it's slow going. 😅

I see the reasoning to flipping it so we call the new method useFinitePlane. Works for me either way, but I'll just add that I think we could set the default dimensions of the box plane to something large like 40x40 and it would likely cover most legacy cases without issue.

What this hook would do as opposed to just using useBox is what I haven't implemented yet -- it would automatically adjust the position fed to the ref so that the Three Plane gets positioned at the top the box as opposed to the middle. So you get a nice shortcut for working with R3F and cannon together.

krispya commented 2 years ago

Okay, as a proof of concept I added a (hacky) method for adjusting the mesh position for the Plane-Box type so that the plane mesh is positioned at the top of the bounding box. Going through this process made me think if this is something we want in the library, it might be better to make more explicit what it is doing and call it usePlaneBox for exactly what it composes or useThreePlane for how we expect it to be used.

bjornstar commented 2 years ago

Would a method to control the attachment point of the geometry solve the problem? three.js users could do something like useBox([1, 0.1, 1, '+z']) to have the plane attach to the face in the initial +z direction? A plane in three.js is just the face of a box.

We can improve our documentation for usePlane and direct users who are looking for a finite plane to useBox with this option. Additionally, we can improve the debugger so that the plane does not appear to be finite, I think that would reduce a lot of confusion.

krispya commented 2 years ago

Would a method to control the attachment point of the geometry solve the problem? three.js users could do something like useBox([1, 0.1, 1, '+z']) to have the plane attach to the face in the initial +z direction? A plane in three.js is just the face of a box.

We can improve our documentation for usePlane and direct users who are looking for a finite plane to useBox with this option. Additionally, we can improve the debugger so that the plane does not appear to be finite, I think that would reduce a lot of confusion.

This sounds right to me. We can establish a best practices for use-cannon with three.js and keep the hooks transparent with cannon-es. Would an offset be the best solution or an additional option that works more like an attach origin? For example: origin: 'center | top | bottom' I'm not sure how useful a mesh offset is generally.