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

Optimize spring bones computations #1539

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Nov 25, 2024

Spring bones tend to be quite performance intensive on the CPU side. I looked into ways to optimize them, while retaining their original behaviour. This PR consists of three commits each focussing on a different area.

See the following table which shows the average duration a call to vrm.update took. The changes are stacked, so the Collider shape offset column contains all three optimizations.

Avatar Before PR Pre-compute order World space computations Collider shape offset
VRM1_Constraint_Twist_Sample.vrm 658.0 µs 179.1 µs 164.3 µs 145.7 µs
AvatarSample_C.vrm 1.0 ms 615.3 µs 473.8 µs 415.2 µs
Zonko_VRM_221128_ps.vrm 3.2 ms 318.0 µs 269.1 µs 200.1 µs
AKAI Original VRM by Shugan.vrm 161.7 µs 105.8 µs 92.4 µs 95.1 µs

Pre-computing spring joint update order (a21e474)

The order in which spring bones need to be updated can be pre-computed making the actual update a lot more straightforward and performant. Whenever a spring bone is added or removed, the order is recomputed. The following things are precomputed:

  • The order in which the spring bones should be updated when taking all dependencies into account (_sortedJoints)
  • The ancestors from the lowest common ancestor of all the spring bone roots to the spring bone roots (_ancestors)

Computing stiffness and gravity in world space (2e14729)

The VRMSpringBoneJoint.update method converted all forces into center space. This is required for inertia, but for both stiffness and gravity this isn't strictly needed. It saves a couple of matrix multiplications by handling these in world space instead.

Additionally some of the vector math could be simplified (e.g. .add(v3.multiplyScalar(scalar)) -> .addScaledVector(v3, scalar)) slightly reducing the amount of operations required.

Aligning collider position with collider shape offset (7449a0d)

The VRMSpringBoneCollider is a THREE.Object3D with an identity transform. The collision shapes contain an offset, which is used to compute the world position of the collider shape. But this is done each time calculateCollision is called, which can be many times per-frame, despite the actual world position of the collider not changing in between.

By setting the position of VRMSpringBoneCollider to the offset, the world position can be retrieved from the worldMatrix instead, ensuring it's only calculated once per collider per update. To further reduce the number of calculations done, the output vector (target) is now only updated in case the distance is negative.

This did require some changes to the unit tests, as they don't use VRMSpringBoneCollider but test against the VRMSpringBoneColliderShape directly.

Notes

For benchmarking and testing I created a setup where I loaded the VRM, called vrm.update(FIXED_DELTA) and compared the world positions and rotations of all joints to reference values based on running the same on dev. So for the cases and VRM models tested above, the output after one update was identical (with EPSILON=0.0001).

/**
* List of dependencies that need to be updated before this joint.
*/
public get dependencies(): THREE.Object3D[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] Existing VRMConstraints already have signatures get dependencies(): Set<THREE.Object3D>. I want to align the interface unless it needs to be an array instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you moved this to the public property though? I don't think it's a bad idea, but it is still called only from VRMSpringBoneManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I moved it as I wanted to cache the list of dependencies, since it caused a lot of memory allocations in the hot-path (VMRSpringBoneManager.update). The VRMSpringBoneJoint itself seemed like a good place to keep the cached list as it could also invalidate it when needed. But after pre-computing the joint order, it was no longer in the hot-path, so I removed the caching again.

In the end the move wasn't needed. But I like the consistency with the VRMConstraints, so I'll update the signature to return a Set<THREE.Object3D> as well.

@0b5vr
Copy link
Contributor

0b5vr commented Nov 26, 2024

I reviewed the first two commits.
I've left several comments on the first one. I might miss the point so feel free to correct them.
The second one looks perfect to me.

I will review the third one tomorrow.

@mrxz mrxz force-pushed the optimize-spring-bones branch from 7449a0d to a7d1201 Compare November 26, 2024 10:49
@mrxz mrxz force-pushed the optimize-spring-bones branch from a7d1201 to 856df6f Compare November 26, 2024 10:55
@0b5vr
Copy link
Contributor

0b5vr commented Nov 27, 2024

I'm kinda negative about the concept of the third commit. This change would prevent users from setting offset and tail after initializing, which is a breaking change in the first place.
We have yet to provide many editing features so far in three-vrm, but I still want to be conservative about making this change.

The save on the length() calculation looks great. Does this alone provide a performance improvement?

@mrxz mrxz force-pushed the optimize-spring-bones branch 2 times, most recently from 08136c9 to 0fda17e Compare November 27, 2024 12:47
@mrxz
Copy link
Contributor Author

mrxz commented Nov 27, 2024

I'm kinda negative about the concept of the third commit. This change would prevent users from setting offset and tail after initializing, which is a breaking change in the first place.

I agree, the commit indeed makes changing the colliders after initialization difficult. This isn't a problem when just loading an avatar and using it, but users might want to implement editing features.

I've changed it so that the colliderMatrix is computed in the updateWorldMatrix method of VRMSpringBoneCollider instead. This way the offset and tail can still be changed and the collider matrix is still only computed once per update. The VRMSpringBoneManager guarantees that updateWorldMatrix is called for the colliders before updating the spring bones. Let me know what you think.

@mrxz mrxz force-pushed the optimize-spring-bones branch from 0fda17e to a26974e Compare November 28, 2024 09:44
Copy link
Contributor

@0b5vr 0b5vr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it's working correctly, and it's much more performant, especially on models with a lot of spring bones. Thank you for your contribution!

@0b5vr 0b5vr merged commit b35bc21 into pixiv:dev Dec 2, 2024
@0b5vr 0b5vr added this to the next milestone Dec 2, 2024
@0b5vr 0b5vr added the performance Performance issue label Dec 2, 2024
this._hasWarnedCircularDependency = true;
}
return;
}
springBonesTried.add(springBone);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrxz We forgot to put this line to _insertJointSort!

I want to ask you why you deleted this line, just in case. Have you encountered any issues with having this line? If you simply forgot, it's all fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0b5vr I must have accidentality removed that line while moving code around, sorry. It should be added back in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see I see, thank you for the explanation!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PR adds the line: #1556 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants