[1.0] Follow new constraint spec #934

0b5vr · 2022-03-04T10:14:58Z

Description

This PR follows the new constraint spec.

type definitions
implementations
test cases
examples

See: vrm-c/vrm-specification#355

Points need review

Do the implementations look good?
- I simply followed the reference implementation example in the spec (+ several obvious optimizations)
Do the test cases look good?
- I don't think it needs to be exhaustive but should be """enough"""
Any performance suggestions?

… one See: vrm-c/vrm-specification#355

… one (2) forgot to apply changes on constraint

…traint See: vrm-c/vrm-specification#355

… necessary

…sition

nyamadan

LGTM

自分の理解のために少しコメントを残しました。

またConstraintはroll/rotation/aimの選択式であるため型定義の提案をさせていただきました。

nyamadan · 2022-03-08T01:48:41Z

packages/three-vrm-node-constraint/examples/aim.html

-			constraintManager.addConstraint(constraint);
+			} );
+
+			const constraint = new THREE_VRM_NODE_CONSTRAINT.VRMAimConstraint( aim, lowerArm );


[MEMO]

aimがlowerArmに従う。
仕様より +Y 軸がlowerArmのほうにむく。

nyamadan · 2022-03-08T02:08:15Z

packages/three-vrm-node-constraint/examples/roll.html

-			const constraintBC = new THREE_VRM_NODE_CONSTRAINT.VRMPositionConstraint( cubeC, modelRoot );
-			constraintBC.setSource( cubeA );
-			constraintManager.addConstraint( constraintBC );
+			const constraint = new THREE_VRM_NODE_CONSTRAINT.VRMRollConstraint( roll, hand );


[MEMO]

rollがhandに従う。手首が回ると肘が回る。伝える軸はY軸

nyamadan · 2022-03-08T02:11:19Z

packages/three-vrm-node-constraint/examples/models/cubes.gltf

@@ -320,9 +320,9 @@
        "VRMC_node_constraint": {
          "specVersion": "1.0-draft",
          "constraint": {
-            "rotation": {
+            "roll": {


MEMO

RollConstraintの指定。
0番目のノードよりY軸回転の影響を受ける。

nyamadan · 2022-03-08T02:24:56Z

packages/three-vrm-node-constraint/examples/rotation.html

@@ -120,9 +125,16 @@

 				requestAnimationFrame( animate );

-				const deltaTime = clock.getDelta();
+				source.rotation.set(


[MEMO]

ここでquaternionが上書きされる。

nyamadan · 2022-03-08T02:34:48Z

packages/three-vrm-node-constraint/src/VRMRotationConstraint.ts

-    target.copy(QUAT_IDENTITY);
+  public setInitState(): void {
+    this._dstRestQuat.copy(this.destination.quaternion);
+    quatInvertCompat(this._invSrcRestQuat.copy(this.source.quaternion));


[MEMO]

初期化時のquaternionをここで保持しておく。

nyamadan · 2022-03-09T10:40:44Z

packages/three-vrm-node-constraint/src/VRMRollConstraint.ts

+     * What the quatFromTo is intended to be:
+     *
+     * ```ts
+     * const quatFromTo = _quatB.setFromUnitVectors( n0, n1 ).inverse();


[MEMO]

n0(this._v3RollAxis)→n1の逆元と等価。

nyamadan · 2022-03-09T12:25:16Z

packages/types-vrmc-node-constraint-1.0/src/Constraint.ts

@@ -1,10 +1,14 @@
+import { AimConstraint } from './AimConstraint';
+import { RollConstraint } from './RollConstraint';
 import type { RotationConstraint } from './RotationConstraint';

 /**
 * An object contains one of constraints.
 */
 export interface Constraint {


[IMO]
三者択一でしたらこんな感じに型定義してみるのはどうでしょうか。

interface ConstraintBase { extensions?: { [name: string]: any }; extras?: any; } interface ContraintWithRoll extends ConstraintBase { roll: RollConstraint; aim: undefined; rotation: undefined; } interface ContraintWithAim extends ConstraintBase { aim: AimConstraint; roll: undefined; rotation: undefined; } interface ContraintWithRotation extends ConstraintBase { rotation: RotationConstraint; roll: undefined; aim: undefined; } /** * An object contains one of constraints. */ export type Constraint = ContraintWithRoll | ContraintWithAim | ContraintWithRotation;

説明忘れてましたが、ここの型定義はspecのschemaに準拠するものです。

https://github.com/vrm-c/vrm-specification/blob/c518a9d9a17dfeb32c6056af4c6c5bee2e215672/specification/VRMC_node_constraint-1.0_draft/schema/VRMC_node_constraint.constraint.schema.json

extension および extras はどちらかというと gltfProperty.schema.json に準拠するものです。

https://github.com/KhronosGroup/glTF/blob/main/specification/2.0/schema/glTFProperty.schema.json

gltf-transformの型定義では以下の箇所かな。

https://github.com/donmccurdy/glTF-Transform/blob/3da81241fed236cb39c2698beb595c6631810efc/packages/core/src/types/gltf.ts#L30-L39

提案のコードはちょっとやりすぎじゃないかなあという気がしています。
TypeScriptで簡単にJSON SchemaのOneOfが書ければ良いのですけどね。
ContraintWithRoll 等の型推論ができたとして、実際のコードベース内でどういう恩恵が受けられるか気になります。

型推論の恩恵が得られるケースとしては具体的にはこちらの箇所で型推論が期待できます。

たとえば .roll != null でしたら、 .aim と .rotation は undefined になると推論できますので、不用意に.aim と .rotation にアクセスできません。

また、 .aim 、 .rotation 、 .roll すべてが undefined であった場合、どの型にもマッチしなくなるので never 型に推論できます。

こちらのsafeUnreachableのようにnever型とマッチする関数を作っておけば例えばConstraintが増えた場合にコンパイルエラーとできます。

確かに型は少し複雑になりますので取り入れるかどうかの判断はお任せします。

ちょっと他にも同様のoneOfあるので、いったんあとで判断でも良いですか。

他のoneOfはこのへんかな: https://github.com/pixiv/three-vrm/blob/1.0/packages/types-vrmc-springbone-1.0/src/SpringBoneColliderShape.ts

はい、いったんはこのままで。別のPullRequestなどで対応できればと思います。

nyamadan · 2022-03-09T12:28:38Z

packages/types-vrmc-node-constraint-1.0/src/RotationConstraint.ts

  /**
   * The weight of the constraint.
   */
  weight?: number;
+
+  extensions?: { [key: string]: { [key: string]: any } };


[Q]
ここが二次元の連想配列になっているのは何故でしょうか？

あ。なんかこれはミスってそうだな。ちゃんと調査して直します。

gltf-transform の定義では Record<string, unknown> 。
たぶん { [name: string]: any } のほうが正解そうですね。直します。

ちなみに実際に拡張名自体以外の情報量が無の拡張として KHR_materials_unlit があるが、こちらは値として {} を使っている模様。うーん、まあいいか。

修正しました @ 5cc0ba5
見直したら、他の拡張についても同様のシグネチャが紛れ込んでいるっぽいので、そちらも別PRで直します。

nyamadan · 2022-03-09T12:32:16Z

packages/three-vrm-node-constraint/src/utils/tests/traverseAncestorsFromRoot.test.ts

+     *     - objB
+     *       - objC <- this is gonna be given to the function
+     *         - objD <- this is a dummy
+     */


コメント助かります。

nyamadan · 2022-03-09T13:16:37Z

packages/three-vrm-node-constraint/src/tests/VRMRollConstraint.test.ts

+
+          describe('when the source is rotated around Z axis', () => {
+            beforeEach(() => {
+              source.quaternion.multiply(quatPZ90);


X軸周りに90度回転してるから、Z軸と重なる。

0b5vr added 6 commits March 1, 2022 17:50

change (1.0, schema): update constraint type definitions to the newer…

eb6a253

… one See: vrm-c/vrm-specification#355

change (1.0, schema): update constraint type definitions to the newer…

c387aa6

… one (2) forgot to apply changes on constraint

change (1.0, constraints): new implementation, test, examples of cons…

ba4eb6c

…traint See: vrm-c/vrm-specification#355

refactor (1.0, constraints): remove util functions that are no longer…

feda4f1

… necessary

fix (1.0, constraints): use quatInvertCompat

db7ad7c

performance (1.0, constraints): use decomposeRotation and decomposePo…

c469866

…sition

0b5vr added enhancement New feature or request VRM 1.0 labels Mar 4, 2022

0b5vr added this to the VRM 1.0 milestone Mar 4, 2022

0b5vr requested a review from nyamadan March 4, 2022 10:14

0b5vr self-assigned this Mar 4, 2022

refactor (1.0, constraints): populate with comments

fdc97cf

nyamadan approved these changes Mar 9, 2022

View reviewed changes

fix (1.0, schema): fix signatures of extensions for constraint

5cc0ba5

This was referenced Mar 10, 2022

[1.0] type packages, Tighten type definitions of oneOf objects #941

Open

fix (1.0, schema): fix signatures of extensions #942

Merged

0b5vr merged commit 9bf78dd into 1.0 Mar 17, 2022

0b5vr deleted the new-constraint branch March 17, 2022 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.0] Follow new constraint spec #934

[1.0] Follow new constraint spec #934

0b5vr commented Mar 4, 2022

nyamadan left a comment

nyamadan Mar 8, 2022

nyamadan Mar 8, 2022

nyamadan Mar 8, 2022

nyamadan Mar 8, 2022

nyamadan Mar 8, 2022

nyamadan Mar 9, 2022

nyamadan Mar 9, 2022

0b5vr Mar 10, 2022

nyamadan Mar 10, 2022 •

edited

Loading

0b5vr Mar 10, 2022

nyamadan Mar 10, 2022

nyamadan Mar 9, 2022

0b5vr Mar 10, 2022

0b5vr Mar 10, 2022

0b5vr Mar 10, 2022 •

edited

Loading

nyamadan Mar 9, 2022

nyamadan Mar 9, 2022

[1.0] Follow new constraint spec #934

[1.0] Follow new constraint spec #934

Conversation

0b5vr commented Mar 4, 2022

Description

Points need review

nyamadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyamadan Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0b5vr Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyamadan Mar 10, 2022 •

edited

Loading

0b5vr Mar 10, 2022 •

edited

Loading