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

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

Merged
merged 13 commits into from
Feb 13, 2024

Conversation

0xtito
Copy link
Contributor

@0xtito 0xtito commented Feb 6, 2024

Description

r3-rapier_springjoint_example.mov
  • Rope Joint example
r3-rapier_ropejoint_example.mov
  • Changed the type for body1Anchor and body2Anchor to react-three/fiber's Vector3 + created vector3ToRapierVector.
    • I did not make changes to all joint params, only for the spring and rope joints.
    • This was done because I believe we are unnecessarily creating threejs Vector3 objects, when all that is required is a Vector3Tuple, but a threejs Vector3 also works. So now you can pass either a threejs Vector3, a Vector3Tuple, or a number. If it is a Vector3Tuple, then we are just creating a RapierVector which is essentially an object with x, y, and z as props.
  • Added setter for additionalSolverIterations to mutableRigidBodyOptions
  • Change default values for erp and predictionDistance to 0.8 and 0.002, respectfully.
    • to be honest, I completely understand why we changed to default value of erp to 0.2. Inside of rapier.js, the default value is 0.2, as shown here.

Changes

  • Update <Physics /> to reflect to integrationParameters for @dimforge/rapier3d-compat v0.12.0
  • Updated useSpringJoint and useRopeJoint started by @isaac-mason
    • Will need to add some good docs/comments for these because the order of the passed in bodyAnchors is significant
  • Enhanced <SpringExample /> and <RopeJointExample /> examples
  • Created vector3ToRapierVector utility function
  • Testing changing bodyAnchor types to react-three/fiber's Vector3.
    • This will allow someone to pass either a threejs Vector3, Vector3Tuple, or a number.
    • if there is no errors/bugs, we should change all JointParams to react-three/fiber's Vector3 so we do not unnecessarily create threejs Vector3 classes.

Checklist:

  • 🔍 I have performed a self-review of my code
  • 💬 I have commented my code, particularly in hard-to-understand areas
  • 📗 I have made corresponding changes to the documentation
  • ⭐️ My changes generate no new warnings
  • 🧪 I have added tests
  • 🟢 All new and existing unit tests pass

Copy link

changeset-bot bot commented Feb 6, 2024

🦋 Changeset detected

Latest commit: fd46529

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
Copy link
Member

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

@isaac-mason
Copy link
Member

isaac-mason commented Feb 6, 2024

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
Copy link
Member

@0xtito added some docs to readme.md now 🙂

@isaac-mason
Copy link
Member

Let me know if the changes I added look good to you @0xtito 🙂

@drcmda
Copy link
Member

drcmda commented Feb 6, 2024

beautiful! thank you all!

@0xtito
Copy link
Contributor Author

0xtito commented Feb 6, 2024

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
Copy link
Contributor Author

0xtito commented Feb 6, 2024

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

@0xtito
Copy link
Contributor Author

0xtito commented Feb 6, 2024

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 0xtito changed the title feat: continuation of update to @dimforge/rapier3d-compat to 0.12.0 + optimizations feat: continuation of update @dimforge/rapier3d-compat to 0.12.0 + optimizations Feb 6, 2024
@0xtito 0xtito marked this pull request as ready for review February 6, 2024 18:49
@0xtito
Copy link
Contributor Author

0xtito commented Feb 6, 2024

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
Copy link
Member

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
Copy link
Member

wiledal commented Feb 7, 2024

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
Copy link
Contributor Author

0xtito commented Feb 7, 2024

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
Copy link
Contributor Author

0xtito commented Feb 7, 2024

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

@wiledal
Copy link
Member

wiledal commented Feb 7, 2024

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
Copy link
Member

wiledal commented Feb 7, 2024

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

0.12.0

Screenshot 2024-02-07 at 22 13 56

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
Copy link
Contributor Author

0xtito commented Feb 7, 2024

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
Copy link
Contributor Author

0xtito commented Feb 7, 2024

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
Copy link
Contributor Author

0xtito commented Feb 7, 2024

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

correct_contactForceExample_numSolverIterations1.mov

@isaac-mason
Copy link
Member

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
Copy link
Member

Also created an issue: dimforge/rapier.js#261

@wiledal
Copy link
Member

wiledal commented Feb 12, 2024

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
Copy link
Contributor Author

0xtito commented Feb 12, 2024

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
Copy link
Contributor Author

0xtito commented Feb 12, 2024

Let me know @wiledal if there is anything else I can add/do for this PR 🙂

@0xtito
Copy link
Contributor Author

0xtito commented Feb 13, 2024

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
Copy link
Member

wiledal commented Feb 13, 2024

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

@wiledal wiledal merged commit 1517acb into pmndrs:main Feb 13, 2024
1 of 2 checks passed
@0xtito
Copy link
Contributor Author

0xtito commented Feb 13, 2024

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
Copy link
Member

wiledal commented Feb 13, 2024

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

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.

4 participants