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: update @dimforge/rapier3d-compat to 0.12.0 #601

Closed
wants to merge 1 commit into from

Conversation

isaac-mason
Copy link
Member

@isaac-mason isaac-mason commented Jan 28, 2024

Description

Changes

  • Update <Physics /> props per world configuration properties added and removed in 0.12.0
  • Add joint hooks for new joints introduced in 0.12.0
    • useSpringJoint, useRopeJoint
  • Added examples for new spring and rope joints
    • Would be good to flesh out the rope joint example
    • The spring joint example doesn't work yet - need to investigate

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 Jan 28, 2024

🦋 Changeset detected

Latest commit: 239019c

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

This PR includes changesets to release 1 package
Name Type
@react-three/rapier Minor

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

@0xtito
Copy link
Contributor

0xtito commented Feb 1, 2024

Hey @isaac-mason - I love to see a feat already created! Springs would be very useful for a library I'm working on so I'm eager to help contribute and get r3-rapier up to date. I am going to spend some time diving into rapier.js and r3-rapier's codebase, and then pick up where you left off 👨‍💻 exciting times

@isaac-mason
Copy link
Member Author

Hey @0xtito!

If you're keen to help, there's a couple of things blocking this PR:

  • The spring hook doesn't work yet (see the new example). We need to investigate whether this is an issue here in rt/rapier or upstream in rapier.js

  • The rapier constraint solver got a big rework in the latest release. We need to do some testing to see how much this changes physics simulations when upgrading, and potentially write some upgrade docs

@0xtito
Copy link
Contributor

0xtito commented Feb 1, 2024

Hey @0xtito!

If you're keen to help, there's a couple of things blocking this PR:

  • The spring hook doesn't work yet (see the new example). We need to investigate whether this is an issue here in rt/rapier or upstream in rapier.js
  • The rapier constraint solver got a big rework in the latest release. We need to do some testing to see how much this changes physics simulations when upgrading, and potentially write some upgrade docs

Nice sounds good, I have already cloned this draft locally and have noticed some errors/bugs.

Regarding the Springs

  • The error message seems to be from rapier.js - similar to what you said, it'll take some digging to see if that is because we are failing to set some property or if the problem is deeper. It probably has to do with the new constraints solver, leading to your next point.
    image

The rapier constraint solver

  • I have been spending most of the my time trying to understand the changes and see how r3-rapier initialized the original collider. There are some examples, like the Components and Cluster examples, that seem to either break or have immense FPS drops on collision, even though the the components have not been changed at all.

I'll continue to look into the changes between the old and new collider and make sure we are properly setting/updating the state

@0xtito
Copy link
Contributor

0xtito commented Feb 4, 2024

Quick update - after having no real breakthroughs on figuring out whether this was a rapier/rapier.js problem or something on our end, I decided to replicate the spring example they showed in the changelog, here, but with rapier.js.

And it worked!

rapierjs_springs_example_1.mp4

Working with rapier.js and recreating this example has all but confirmed to me the problem is on our end (thankfully). It likely has to do with the the solver. Inside of the island solver, link, the init_and_step function has had an overhaul. (Last time this file was altered was 2 years ago, link to that commit here.

Out of the whole function, I think the most significant change is the following:

        counters.solver.velocity_resolution_time.resume();
        let num_solver_iterations = base_params.num_solver_iterations.get()
            + islands.active_island_additional_solver_iterations(island_id);

        let mut params = *base_params;
        params.dt /= num_solver_iterations as Real;
        params.damping_ratio /= num_solver_iterations as Real;

Use not taking into accountdt being divided by num_solver_iterations could be the (one?) reason why we are seeing significant frames drops in some of are components, notably those that have many/intensive collisions.

Why the spring is not working is an entirely other beast 😅

So here is what I am gonna be focusing on the next few days:

  • establish that the compat version of rapier3d is not causing the errors
  • test different approaches for how we are initiating each rapier step
  • find the source of the errors with the spring joint
  • ensure we are have added all the setters/getters for rigid bodies and colliders
  • create fleshed out examples for both the spring and rope joints

Sorry for the long comment, just wanted to get everything I have been doing down in one comment. If you guys have any thoughts/feedback on my analysis and approach for solving the problems, I am all ears!

@0xtito
Copy link
Contributor

0xtito commented Feb 4, 2024

Another update

Managed to get the figure out what was causing the error with the spring joints, it was just how it was being set. The order of how it is set (which is body1 vs body2) matters as well, as well as the location of where the rigidBodies are (I believe). I can do a little write up why that is tomorrow or monday, but my brain is all tapped out right now haha.

Still though, it does not completely work. I am using the same exact example as the one from rapier and rapier.js since they are good baseline, but collision detection isn't working properly. The values are the exact same as the other examples, thus it seems all errors stem from the solver.

I have not commited anything yet since things still seem to be somewhat incomplete, but if you guys want to see how I've implemented it I can commit it.

Here is a video of the SpringsExample in action.

r3-rapier_springs_example_1.mov

P.S - Is there a reason why for every joint hook we are converting a Vector3Tuple into a THREE.Vector3? I do not see why it is necessary, since we are creating a new THREE Vector3, when all that is required is to turn it in a vec3 (or just a object with x,y,z as props)? I am passing the anchors as a vec3 and just passing that into rapier.JointData.spring, but if their is a good reason ofc I'll just change it back.

@wiledal
Copy link
Member

wiledal commented Feb 4, 2024

Thanks for the hard work put into this!

For the tuple conversions, are you specifically referring to rows like these?
https://github.com/pmndrs/react-three-rapier/blob/main/packages/react-three-rapier/src/hooks/joints.ts#L164
In those places, there's no benefit other than ease of use and normalization. Any suggestion for improvements are of course welcome.

There are a lot of improvements that I have been thinking of, but I haven't had time to make them a reality. I imagine a v2 where the Rapier lib can be dropped in allowing the user to use the non-compat version. And the eager initiation of imperative objects needs work to make it more compatible with other things.

@0xtito
Copy link
Contributor

0xtito commented Feb 4, 2024

No problem @wiledal! I'm getting a much better understanding of how rapier and r3-rapier are working under the hood so its been a joy.

For the tuple conversions, are you specifically referring to rows like these? https://github.com/pmndrs/react-three-rapier/blob/main/packages/react-three-rapier/src/hooks/joints.ts#L164 In those places, there's no benefit other than ease of use and normalization. Any suggestion for improvements are of course welcome.

Yes exactly, it seems unnecessarily resource intensive to create a three.js Vector3, when all that is need to is a obj with x, y, and z props. I can try to refactor parts of the code which could instead use vec3 and see if their are any noticeable performance bumps. Could add some tests as well

There are a lot of improvements that I have been thinking of, but I haven't had time to make them a reality. I imagine a v2 where the Rapier lib can be dropped in allowing the user to use the non-compat version.

I was wondering why we use the compat version. I have noticed that most downloads are from the compat version as well. Is it due to bundling errors, performance boosts, or some other reason?

And the eager initiation of imperative objects needs work to make it more compatible with other things.

can you elaborate on this? Are you referring to the useImperativeHandle / useImperativeInstance hooks?

@isaac-mason
Copy link
Member Author

The compat version inlines the wasm in the JavaScript bundle. This makes consuming the library easier, as consumers don't need to be using a bundler that supports importing wasm files. But there's some file size and initialisation time tradeoffs when inlining.

@isaac-mason
Copy link
Member Author

And the eager initiation of imperative objects needs work to make it more compatible with other things.

That may be referring to issues like this?

#573

@isaac-mason
Copy link
Member Author

I have not commited anything yet since things still seem to be somewhat incomplete, but if you guys want to see how I've implemented it I can commit it.

Feel free to create another draft PR and push to it as you go. I'm keen to follow along 🙂

@0xtito
Copy link
Contributor

0xtito commented Feb 5, 2024

I have not commited anything yet since things still seem to be somewhat incomplete, but if you guys want to see how I've implemented it I can commit it.

Feel free to create another draft PR and push to it as you go. I'm keen to follow along 🙂

Okay I will do that! I'll branch of from your initial work since you did a great job setting up the types and reflecting many of the changes from v0.11.2 to v0.12.

I will try to keep the scope of the PR related the upgrade to v0.12, but @wiledal do you mind if I also commit changes regarding optimizations, as an example, like the Vector3 to vec3? Or would you rather have optimizations not directly related to upgrading v0.11.2 to v0.12 be in a separate PR?

@wiledal
Copy link
Member

wiledal commented Feb 5, 2024

But there's some file size and initialisation time tradeoffs when inlining.

People have been asking for ways to do this. In the best of worlds, the core of the library is Rapier library agnostic. Using the bundled version is the most straight forward, and can still be default behavior, but being able to also import a tree-shaken <Physics /> wrapper and supplying the library yourself would allow advanced users to be more flexible.

Are you referring to the useImperativeHandle / useImperativeInstance hooks?

Yes, those, and the singletonProxy are hijinks attempting to out-maneuver React's lifecycle callbacks.
Problems occur when you try to access a property on an object at render, when the object itself is not created until on mount. You have to eagerly create the object as soon as it is referenced, but that also creates difficulties when destroying objects. I want to attempt a rewrite, but it's a large undertaking. I should start a discussion around this topic 😄

do you mind if I also commit changes regarding optimizations

I don't mind!

@0xtito
Copy link
Contributor

0xtito commented Feb 6, 2024

Just created a new PR. The updated examples are there and it seems like everything is back to normal regarding performance :)

Honestly embarrassing to admit, but one of the key parts that was causing the lag, with all components, is how the default value for erp was set to 0.8. I mention it in the PR, but in rapier.js - they say the default value is 0.8. Yet it seems for us it is the inverse value. So either: a) the value we pass into erp is somehow being processed in reverse (1 - erp vs erp), b) the default value is not a high enough value for the error detection to properly work, c) compilation error when generating @dimforge/rapier3d-compat. I have a feeling it is option a but worth looking into.

@isaac-mason isaac-mason closed this Feb 6, 2024
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