Conversation
diegoteran
commented
Apr 10, 2023
diegoteran
commented
- Adds Feature Request: Get and Set texture transform info #3368
- Adds modelviewer.dev example
* Adds feature #3368 * Adds modelviewer.dev example
Missing automated test
| await waitForEvent(element, 'load'); | ||
|
|
||
| // Transform the textures. | ||
| const sampler = element.model?.materials[0].pbrMetallicRoughness['baseColorTexture'].texture?.sampler; |
There was a problem hiding this comment.
In tests you can use ! liberally, since if you're wrong, the test will fail.
There was a problem hiding this comment.
Then you won't have to keep doing it over and over down below.
There was a problem hiding this comment.
You marked this as resolved, but I don't think you made the change?
packages/model-viewer/src/test/features/scene-graph/texture-info-spec.ts
Show resolved
Hide resolved
packages/model-viewer/src/three-components/gltf-instance/gltf-2.0.ts
Outdated
Show resolved
Hide resolved
elalish
left a comment
elalish
left a comment
There was a problem hiding this comment.
Looking good, just some coding style left.
elalish
left a comment
elalish
left a comment
There was a problem hiding this comment.
Sorry, I missed some things last time.
| await waitForEvent(element, 'load'); | ||
|
|
||
| // Transform the textures. | ||
| const sampler = element.model?.materials[0].pbrMetallicRoughness['baseColorTexture'].texture?.sampler; |
There was a problem hiding this comment.
You marked this as resolved, but I don't think you made the change?
| <p>Scale: <span id="texture-scale"></span></p> | ||
| <input type="range" min="0.5" max="1.5" value="1" step="0.01" id="scaleSlider"> | ||
| <p>Offset</p> | ||
| <input type="range" min="0.5" max="1.5" value="1" step="0.01" id="offsetSlider"> |
There was a problem hiding this comment.
check alt text and ranges - e.g. offset should start at 0, rotation to go to pi, etc.
There was a problem hiding this comment.
What do you think of using 3.14 as PI here?
| const material = modelViewerTexture2.model.materials[0]; | ||
|
|
||
| let rotationDisplay = document.querySelector('#texture-rotation'); | ||
| let scaleDisplay = document.querySelector('#texture-scale'); |
| private[$setProperty]<P extends 'minFilter'|'magFilter'|'wrapS'|'wrapT'>( | ||
| property: P, value: MinFilter|MagFilter|WrapMode) { | ||
| setRotation(rotation: number|null): void { | ||
| if(!rotation) { |
There was a problem hiding this comment.
prefer == null for null checks, since truthiness can be surprising.
| setWrapS(mode: WrapMode): void; | ||
| setWrapT(mode: WrapMode): void; | ||
| setRotation(rotation: number|null): void; | ||
| setScale(scale: Vector2|null): void; |
There was a problem hiding this comment.
You need to define Vector2 in the docs.
| scaleDisplay.textContent = scaleSlider.value; | ||
|
|
||
| // Texture wrap is often used as REPEAT. | ||
| material.pbrMetallicRoughness['baseColorTexture'].texture.sampler.setWrapS(10497); //WrapMode.Repeat |
There was a problem hiding this comment.
Check if this is still necessary with the textureCube (since this is the glTF default already).
There was a problem hiding this comment.
It does work without it, but might be a good idea to leave it as an example
There was a problem hiding this comment.
It's important for the examples to be as simple as possible, and enum numbers like that are confusing. We can answer something like this in a discussion if it comes up.
| x: offsetSlider.value, | ||
| y: -offsetSlider.value | ||
| }; | ||
| material.pbrMetallicRoughness['baseColorTexture'].texture.sampler.setOffset(offset); |
There was a problem hiding this comment.
Let's save this sampler as a const up above so we can simplify these example functions.
* Get and Set texture transform * Adds feature google#3368 * Adds modelviewer.dev example * Texture Transform in Sampler * constructor fix * Sampler API working Missing automated test * Add test * Document and clean gltf file. * Revoke url from test * Remove test.only * Fix coding style. * Clean index and test file
2 participants
