pmndrs / react-three-rapier

🀺 Rapier physics in React
https://react-three-rapier.pmnd.rs
MIT License
1.06k stars 58 forks source link

feat: continuation of update @dimforge/rapier3d-compat to 0.12.0 + optimizations #619

Closed 0xtito closed 8 months ago

0xtito commented 8 months ago

Description

https://github.com/pmndrs/react-three-rapier/assets/92818759/945157bb-24bd-4024-b4aa-c5d2c0ec4c0e

https://github.com/pmndrs/react-three-rapier/assets/92818759/47725b4b-d216-43e7-a710-f91a89494f2d

Changes

Checklist:

changeset-bot[bot] commented 8 months ago

πŸ¦‹ Changeset detected

Latest commit: fd4652926a05151fd651356ab2187017fd0c126a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | -------------------------- | ----- | | @react-three/rapier | Minor | | @react-three/rapier-addons | Major |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

isaac-mason commented 8 months ago

Before releasing we'll also want to docs for the new joints to README.md

isaac-mason commented 8 months ago

Also realised we can add the "Generic" joint too - but again, doesn't have to all be in this PR. We can add that seperately.

Added JointData.generic (3D only) to create a generic 6-dof joint and manually specify the locked axes.

https://github.com/dimforge/rapier.js/blob/master/CHANGELOG.md

isaac-mason commented 8 months ago

@0xtito added some docs to readme.md now πŸ™‚

isaac-mason commented 8 months ago

Let me know if the changes I added look good to you @0xtito πŸ™‚

drcmda commented 8 months ago

beautiful! thank you all!

0xtito commented 8 months ago

Let me know if the changes I added look good to you @0xtito πŸ™‚

Looks great to me! Thanks for the additions.

There is one thing that is kinda bothering me though. The entire simulation just feels slower compared to the rapier/rapier.js examples. For example in the wrecking ball video above, the ball seems to move slow, and even when the boxes are hit they seem to fall slowly.

This isn't necessarily a problem, but I do wonder why this is the case. I also think this could be discussed/dissected in another PR (unless the answer why this happens is already known, then i'd love to know why).

0xtito commented 8 months ago

just changed the text for <RopeJointExample /> button to "Rope Joint" to keep the styling in the demo consistent.

0xtito commented 8 months ago

is there anything else that needs to be added before the PR can be merged? I left the checklist as is, but does the default checklist apply here?

0xtito commented 8 months ago

Marked as ready for review - if there are still things needed to be done feel free to change it back to a draft!

isaac-mason commented 8 months ago

These changes look good to me! Let's get a review from @wiledal too.

There is one thing that is kinda bothering me though. The entire simulation just feels slower compared to the rapier/rapier.js examples. For example in the wrecking ball video above, the ball seems to move slow, and even when the boxes are hit they seem to fall slowly.

I can take a look at this shortly.

wiledal commented 8 months ago

Changes look good to me! I'm wondering though why the performance has degraded and is now noticeably stuttery on my machine compared to the old demo -- new settings defaults? (possibly due to the erp value change)

Also, the contact force events are not firing like they used to πŸ€”

0xtito commented 8 months ago

Changes look good to me! I'm wondering though why the performance has degraded and is now noticeably stuttery on my machine compared to the old demo -- new settings defaults? (possibly due to the erp value change)

Also, the contact force events are not firing like they used to πŸ€”

Regarding the performance, the performance seems to be the exact same in both environments for me. Do you think it could it could be a browser problem? I am using Arc, but could test it across different browsers to see. Not sure why that would be happening.

Also regarding the contact force, that is odd. In the new contact-force-events demo, it only emits the contact force once the ball is ~starts to bounce very little~ at a complete stop, and only then does it bounce higher + emit the contact force to the console. I'll try to debug this now.

0xtito commented 8 months ago

I also realized the Spring and Joint sections were missing from the Readme topics so I just added them

wiledal commented 8 months ago

I'm on Arc as well. I double checked, and it does not stutter anymore. Sometimes I seem to get a stuttering version after random refreshes, that might not have anything to do with this update however.

I'm looking at the contact force event issue. It seems it just doesn't fire anymore unless the body is almost at rest. Unclear why that would be.

wiledal commented 8 months ago

There's a clear difference between the contact force event from Rapier.js 0.11.2 and 0.12.0.

0.11.2 ![Screenshot 2024-02-07 at 22 12 34](https://github.com/pmndrs/react-three-rapier/assets/1035169/9f00e482-ea28-4566-ba5d-013d8db1aeab) 0.12.0 ![Screenshot 2024-02-07 at 22 13 56](https://github.com/pmndrs/react-three-rapier/assets/1035169/3a410226-6e12-4c63-9776-24bcb03b964e)

This is a plain vite setup with rapier3d-compat, and nothing else. It's odd that we are hardly getting the collision events at all in r3/rapier tho... I'll investigate a bit more, but we might continue without this for now.

0xtito commented 8 months ago

Mhmmm thats so weird.

I noticed in rapier/rapier.js there is a contactForceEventThreshold get/set on the collider class. I do not think we have that implemented..? Assuming its not already implemented, I am going to do so and try playing with the value to see if the problem is that the threshold is not being met

0xtito commented 8 months ago

Just mentioning this change, I am unsure if this is directly related, but in rapier.js v0.12, the names of contactsWith and intersectionsWith have been changed to contactPairsWith and intersectionPairsWith, respectively.

image

link to PR here

0xtito commented 8 months ago

Another thing I have noticed is that, in the new r3-rapier, if you just let the ContactForceEvents example run for a while and watch the log - the contactForce behaves very erratically. Like it will seemingly randomly fire off the onContactForce event. Sometimes launching the ball in back to back bounces.

So I changed the default numSolverIterations to 1 in the Physics.tsx file to see if it was a problem with the new solver/solverIterations. And that did the trick, now every bounce the onContactForce callback is running. On top of that, the totalForceMagnitude has the values we are used to (the higher values).

I am going to keep dissecting this to see why the numSolverIterations is exactly causing this.

Here is a video of the ContactForceEventsExample in this PR's version of r3-rapier, with numSolverIterations set to 1

https://github.com/pmndrs/react-three-rapier/assets/92818759/f29e5c97-0ae1-4fb3-8ed9-69dab8cfbe6d

isaac-mason commented 8 months ago

Let's also see if anyone on the rapier discord can help us, posted a message now: https://discord.com/channels/507548572338880513/748814906953826334/1204933879677329488

isaac-mason commented 8 months ago

Also created an issue: https://github.com/dimforge/rapier.js/issues/261

wiledal commented 8 months ago

Thanks fellas! Since this seems to be an issue with Rapier, and not specifically this library, we'll include it as a migration caveat for this update.

Edit, added here: https://github.com/pmndrs/react-three-rapier/wiki/1.2.1-to-1.3.0-Migration-guide

0xtito commented 8 months ago

Sounds great! Ya it definitely seems to be a problem with Rapier. Once they address the issue, we can merge in those changes then.

Also, there are a few other things we need to add to fully upgrade to 0.12 (like integrating the switchToStandardPgsSolver and switchToSmallStepsPgsSolver methods, adding the Generic JointData, add DynamicRayCastVehicleController, and update the CharacterController with the changes specified here), but what we have now seems more than enough to merge this PR!

0xtito commented 8 months ago

Let me know @wiledal if there is anything else I can add/do for this PR πŸ™‚

0xtito commented 8 months ago

Hey @isaac-mason, do you think this PR is ready to be merged? No rush if you would like to make some additions/changes, but if not I would love for this to be merged so I can start testing it out in my library πŸ™‚

wiledal commented 8 months ago

I will put the repo in canary mode, and merge this so that it can be tested without breaking existing builds.

0xtito commented 8 months ago

Great! Thanks @wiledal - I am using the spring joint to try to build a rapier-integrated hand for mixed reality so i will definitely be testing some edge cases. If I find any errors I will mention them here or as an issue, dependent on the error/your preference (sorry for any hassle i've caused, this is my first time meaningfully contributing to OS and I don't know the right course of action sometimes lol)

wiledal commented 8 months ago

I've done some initial tests in canary, and I think it's seems stable to release normally! Thanks for the awesome work, and for keeping at it.

You should join us on the Poimandres discord for easier communication outside of the pull request context πŸ˜„ https://discord.com/channels/740090768164651008/969941082144124959