-
Notifications
You must be signed in to change notification settings - Fork 819
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
Get and Set texture transform #4209
Conversation
diegoteran
commented
Apr 10, 2023
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In tests you can use !
liberally, since if you're wrong, the test will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you won't have to keep doing it over and over down below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just some coding style left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer const
to let
.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer == null
for null checks, since truthiness can be surprising.
@@ -700,6 +775,9 @@ <h2 class="demo-title">Materials API</h2> | |||
setMagFilter(filter: MagFilter): void; | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if this is still necessary with the textureCube (since this is the glTF default already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work without it, but might be a good idea to leave it as an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's save this sampler as a const up above so we can simplify these example functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* 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