search

pmndrs / react-three-rapier

🤺 Rapier physics in React
https://react-three-rapier.pmnd.rs
MIT License
1.03k stars 57 forks source link

feat: update @dimforge/rapier3d-compat to 0.12.0 #601

Closed isaac-mason closed 7 months ago

isaac-mason commented 7 months ago

Description

Changes

Checklist:

changeset-bot[bot] commented 7 months ago

🦋 Changeset detected

Latest commit: 239019c5489cfff27282e59401142bcd1968b31c

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 commented 7 months ago

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 commented 7 months ago

Hey @0xtito!

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

0xtito commented 7 months ago

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 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 commented 7 months ago

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!

https://github.com/pmndrs/react-three-rapier/assets/92818759/7547c10b-f267-4f52-8a0d-05bbb66a9285

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:

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 commented 7 months ago

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.

https://github.com/pmndrs/react-three-rapier/assets/92818759/61434543-b47e-4198-8e85-43b4a4318c93

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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?

https://github.com/pmndrs/react-three-rapier/discussions/573

isaac-mason commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.