Convert to ThreeJS.Vector2 internally in public api. #4473

diegoteran · 2023-09-21T05:17:40Z

elalish · 2023-09-21T15:48:17Z

packages/model-viewer/src/features/scene-graph/sampler.ts

@@ -18,7 +18,7 @@ import {Texture as ThreeTexture, Vector2} from 'three';
 import {Filter, MagFilter, MinFilter, Wrap, WrapMode} from '../../three-components/gltf-instance/gltf-2.0.js';
 import {Sampler as DefaultedSampler} from '../../three-components/gltf-instance/gltf-defaulted.js';

-import {Sampler as SamplerInterface} from './api.js';
+import {Sampler as SamplerInterface, Vector2 as Vector2Interface} from './api.js';


Maybe we should just rename Vector2 to avoid this confusion with three.js? Also, we have a Vector2D and Vector3D in model-viewer-base.js - can we be consistent with those (accept them as valid input)?

I initially tried to use Vector2D from model-viewer-base in the api.js file and it wouldn't work. I can give it another go at trying out deleting the api.js Vector2 interface.

rename Vector2 to avoid this confusion with three.js?
You mean rename the Vector2 in api.js?

I think the interface is necessary so that users don't have to include a toString(). And yes, I mean renaming in api.js. Also check the members for compatibility with Vector2D.

Yeah Vector2D from base.js requires additional imports. Just uploaded a new commit

elalish · 2023-09-21T15:50:52Z

packages/model-viewer/src/features/scene-graph/sampler.ts

    if (scale == null) {
      // Reset scale.
      scale = new Vector2(1, 1);
    }
-    this[$setProperty]('repeat', scale);
+    this[$setProperty]('repeat', new Vector2(scale.x, scale.y));


Do we have an example that tests this? Did it work before?

Yes we have a transform Textures example. It did work before on android and Mac, still works. I need to test with iOS.

Wow, I guess three.js must automatically turn a scalar into a vector?

elalish · 2023-09-21T20:32:07Z

packages/model-viewer/src/features/scene-graph/api.ts

@@ -32,7 +32,7 @@ export declare interface ThreeDOMElementMap {
 }

 /** A 2D Cartesian coordinate */
-export interface Vector2 {
+export interface Vector2D {


Okay, so now instead of shadowing three's construct, you're shadowing our existing public class from model-viewer-base. Any reason not to use Vector2DInterface like you did before?

Not really, I think this is trying to be an interface for threejs's vector2, so maybe Vector2Interface is better?

elalish · 2023-09-21T20:32:54Z

packages/model-viewer/src/features/scene-graph/api.ts

@@ -32,7 +32,7 @@ export declare interface ThreeDOMElementMap {
 }

 /** A 2D Cartesian coordinate */
-export interface Vector2 {
+export interface Vector2D {


Also, I think you may have missed this one:

Also check the members for compatibility with Vector2D.

I believe they're u and v, not x and y?

Do we want that? I though tthe purpose of this Vector Interface was to not expose threejs' vector2, and ThreeJs Vector2 uses x and y. It seems to me that this interface and Vector2D in base have different purposes. wdyt?

Exactly, we have zero interest in an API consistent with three.js (we're hiding their API). We want an API that is self-consistent and consistent with glTF. We already have an API called Vector2D that uses u and v, so using x and y here would be confusing. Besides, this feature is about textures, which are mapped in a u-v space.

got it, sgtm

elalish · 2023-09-21T20:46:31Z

packages/model-viewer/src/features/scene-graph/sampler.ts

    return this[$threeTexture].repeat;
  }

-  get offset(): Vector2|null {
+  get offset(): Vector2D|null {


I just realized this also needs to change - you're returning a pointer to an internal three.js construct, which means they could then use it to change out internals (in JS every object is pass by reference). You need to use our toVector2D() to make a proper copy.

elalish · 2023-09-21T21:58:59Z

packages/model-viewer/src/test/features/scene-graph/texture-info-spec.ts

-              .to.be.eql(new Vector2(0.2, 0.3), 'offset');
-          expect(exported_sampler.scale)
-              .to.be.eql(new Vector2(0.4, 0.5), 'scale');
+          expect(exported_sampler.offset).to.be.eql({u: 0.2, v: 0.3}, 'offset');


I think the trouble is you're actually returning a Vector2D which has a toString method. I would just separate these lines to check the u and v separately.

elalish · 2023-09-21T21:59:37Z

packages/model-viewer/src/features/scene-graph/sampler.ts

  }

-  get offset(): Vector2|null {
-    return this[$threeTexture].offset;
+  get offset(): Vector2DInterface|null {


nit: unlike set, here you actually get a real Vector2D.

do we want the set and get to have different parameter/return value type?

Technically that's the most accurate - we give a specific object back, but accept a broader interface class as input.

elalish

Thanks! Mind poking around other code you've worked on just to see if you notice any similar issues?

* Convert to ThreeJS.Vector2 internally in public api. Fixes google#4403 * Rename vector2D api interface * Vector2DInterface consistency with Vector2D * Update example to use u,v

Convert to ThreeJS.Vector2 internally in public api.

206d221

Fixes #4403

elalish requested changes Sep 21, 2023

View reviewed changes

Rename vector2D api interface

618e7bb

diegoteran requested a review from elalish September 21, 2023 19:16

elalish requested changes Sep 21, 2023

View reviewed changes

elalish reviewed Sep 21, 2023

View reviewed changes

diegoteran added 2 commits September 21, 2023 13:52

Vector2DInterface consistency with Vector2D

3200979

Fix test

bddf7e0

elalish reviewed Sep 21, 2023

View reviewed changes

Fix test #2

b9fbc82

diegoteran requested a review from elalish September 21, 2023 23:13

Update example to use u,v

824547d

elalish approved these changes Sep 22, 2023

View reviewed changes

diegoteran merged commit bd4a846 into master Sep 22, 2023
5 checks passed

diegoteran mentioned this pull request Sep 22, 2023

sampler.setOffset doesn't move my texture #4428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to ThreeJS.Vector2 internally in public api. #4473

Convert to ThreeJS.Vector2 internally in public api. #4473

diegoteran commented Sep 21, 2023

elalish Sep 21, 2023

diegoteran Sep 21, 2023

elalish Sep 21, 2023

diegoteran Sep 21, 2023

elalish Sep 21, 2023

diegoteran Sep 21, 2023

elalish Sep 21, 2023

elalish Sep 21, 2023

diegoteran Sep 21, 2023

elalish Sep 21, 2023

elalish Sep 21, 2023

diegoteran Sep 21, 2023 •

edited

Loading

elalish Sep 21, 2023

diegoteran Sep 21, 2023

elalish Sep 21, 2023

elalish Sep 21, 2023

elalish Sep 21, 2023

diegoteran Sep 21, 2023

elalish Sep 21, 2023

elalish left a comment

Convert to ThreeJS.Vector2 internally in public api. #4473

Convert to ThreeJS.Vector2 internally in public api. #4473

Conversation

diegoteran commented Sep 21, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoteran Sep 21, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish left a comment

Choose a reason for hiding this comment

diegoteran Sep 21, 2023 •

edited

Loading