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

[1.0] Various fixes related to spring bone #1002

Merged
merged 3 commits into from
Jul 6, 2022
Merged

[1.0] Various fixes related to spring bone #1002

merged 3 commits into from
Jul 6, 2022

Conversation

0b5vr
Copy link
Contributor

@0b5vr 0b5vr commented Jul 4, 2022

Description

Each commit does different things:

  • 2cdaa59: fix default value of gravityDir.
    • default gravityDir was (0.0, 1.0, 0.0) which is against the spec default (0.0, -1.0, 0.0)
  • a4b5db0: SpringBoneJoint.reset did not respect center space
  • bac34aa: gravity should be evaluated in world space instead of center space

Points need review

  • Does each commit make sense?

0b5vr added 3 commits July 4, 2022 20:05
default gravityDir was (0.0, 1.0, 0.0) which is against the spec default (0.0, -1.0, 0.0)

three-vrm-girlがgravityPowerがあるけどgravityDirがないパターンで(なぜ?)、この影響を受けた
…pace

it should apply matrixWorldToCenter to _currentTail
@0b5vr 0b5vr added bug Something isn't working VRM 1.0 labels Jul 4, 2022
@0b5vr 0b5vr added this to the VRM 1.0 milestone Jul 4, 2022
@0b5vr 0b5vr self-assigned this Jul 4, 2022
@ke456-png ke456-png self-requested a review July 6, 2022 02:21
Comment on lines -208 to +210
this.bone.localToWorld(this._currentTail.copy(this._initialLocalChildPosition));
const matrixWorldToCenter = this._getMatrixWorldToCenter(_matA);
this.bone.localToWorld(this._currentTail.copy(this._initialLocalChildPosition)).applyMatrix4(matrixWorldToCenter);
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ 座標系 : local => world => center

centerはSpringboneの揺れものの中心オブジェクト

Comment on lines -169 to +172
gravityDir: new THREE.Vector3().fromArray(prevSchemaJoint.gravityDir ?? [0.0, 1.0, 0.0]),
gravityDir:
prevSchemaJoint.gravityDir != null
? new THREE.Vector3().fromArray(prevSchemaJoint.gravityDir)
: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

デフォルト値の設定をここより下流でやっている意図が気になりました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

どちらでも良いのですが、あえて言えば、Loaderを介さずにSpringBoneを扱う場合向けに VRMSpringBoneJoint 側でデフォルト値がセットされるようになっています。

Copy link
Contributor

Choose a reason for hiding this comment

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

Loaderを介さずにSpringBoneを扱う場合向けに VRMSpringBoneJoint 側でデフォルト値がセットされるようになっています。

理解しました。
この実装で良いと思います。

@0b5vr 0b5vr merged commit c7dcee3 into 1.0 Jul 6, 2022
@0b5vr 0b5vr deleted the fix-springbone-2 branch July 6, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working VRM 1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants