Add support for KHR_texture_transform #8266
Conversation
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
|
message above is due to a network error on crates.io |
|
Your screenshot doesn't look exactly like the expected (https:/KhronosGroup/glTF-Sample-Models/tree/master/2.0/TextureTransformTest), it seems the background for out of texture quads are not the same. Do you know why? How would it work with https:/KhronosGroup/glTF-Sample-Models/tree/master/2.0/TextureTransformMultiTest? |
The default camera and lighting in the scene viewer make it hard to see. The background in the arrow texture is there, just very faint, if that's what you mean.
This looks far from complete because the loader doesn't support texture coord sets. I see there's some TODOs in the source code. The renderer doesn't do clearcoat, and the gltf crate doesn't expose a texture transform for occlusion. |
nicopap
added
C-Feature
There was a problem hiding this comment.
Looks good to me. I'll point out this is an important change.
Need to be aware of costs though:
- This adds 36 bytes to the
StandardMaterialuniform. - This is an additional 3x3 matrix multiplication per standard material pixel.
With #9254, the cost of (1) is fairly low now though.
| let (base_color_texture, uv_transform) = pbr | ||
| .base_color_texture() | ||
| .map(|info| { | ||
| // TODO: handle info.tex_coord() (the *set* index for the right texcoords) | ||
| let label = texture_label(&info.texture()); | ||
| let path = AssetPath::new_ref(load_context.path(), Some(&label)); | ||
| ( | ||
| load_context.get_handle(path), | ||
| info.texture_transform().map(texture_transform_mat3), | ||
| ) | ||
| }) | ||
| .unzip(); | ||
| let uv_transform = uv_transform.flatten().unwrap_or_default(); |
There was a problem hiding this comment.
Nit: For code quality, I would prefer if the base_color_texture and uv_transform definitions were split. the map(…).unzip() is fairly hard to parse.
|
A |
|
Hi… Any update on this?? |
|
Closes #11869 |
alice-i-cecile
added
the
S-Adopt-Me
|
Adopted; I'm currently updating this PR to |
|
Done, just got some questions on the new PR |
|
Closing as adopted :) |
Adopted #8266, so copy-pasting the description from there: # Objective Support the KHR_texture_transform extension for the glTF loader. - Fixes #6335 - Fixes #11869 - Implements part of #11350 - Implements the GLTF part of #399 ## Solution As is, this only supports a single transform. Looking at Godot's source, they support one transform with an optional second one for detail, AO, and emission. glTF specifies one per texture. The public domain materials I looked at seem to share the same transform. So maybe having just one is acceptable for now. I tried to include a warning if multiple different transforms exist for the same material. Note the gltf crate doesn't expose the texture transform for the normal and occlusion textures, which it should, so I just ignored those for now. (note by @janhohenheim: this is still the case) Via `cargo run --release --example scene_viewer ~/src/clone/glTF-Sample-Models/2.0/TextureTransformTest/glTF/TextureTransformTest.gltf`:  ## Changelog Support for the [KHR_texture_transform](https:/KhronosGroup/glTF/tree/main/extensions/2.0/Khronos/KHR_texture_transform) extension added. Texture UVs that were scaled, rotated, or offset in a GLTF are now properly handled. --------- Co-authored-by: Al McElrath <[email protected]> Co-authored-by: Kanabenki <[email protected]>
Adopted bevyengine#8266, so copy-pasting the description from there: # Objective Support the KHR_texture_transform extension for the glTF loader. - Fixes bevyengine#6335 - Fixes bevyengine#11869 - Implements part of bevyengine#11350 - Implements the GLTF part of bevyengine#399 ## Solution As is, this only supports a single transform. Looking at Godot's source, they support one transform with an optional second one for detail, AO, and emission. glTF specifies one per texture. The public domain materials I looked at seem to share the same transform. So maybe having just one is acceptable for now. I tried to include a warning if multiple different transforms exist for the same material. Note the gltf crate doesn't expose the texture transform for the normal and occlusion textures, which it should, so I just ignored those for now. (note by @janhohenheim: this is still the case) Via `cargo run --release --example scene_viewer ~/src/clone/glTF-Sample-Models/2.0/TextureTransformTest/glTF/TextureTransformTest.gltf`:  ## Changelog Support for the [KHR_texture_transform](https:/KhronosGroup/glTF/tree/main/extensions/2.0/Khronos/KHR_texture_transform) extension added. Texture UVs that were scaled, rotated, or offset in a GLTF are now properly handled. --------- Co-authored-by: Al McElrath <[email protected]> Co-authored-by: Kanabenki <[email protected]>
Adopted bevyengine#8266, so copy-pasting the description from there: # Objective Support the KHR_texture_transform extension for the glTF loader. - Fixes bevyengine#6335 - Fixes bevyengine#11869 - Implements part of bevyengine#11350 - Implements the GLTF part of bevyengine#399 ## Solution As is, this only supports a single transform. Looking at Godot's source, they support one transform with an optional second one for detail, AO, and emission. glTF specifies one per texture. The public domain materials I looked at seem to share the same transform. So maybe having just one is acceptable for now. I tried to include a warning if multiple different transforms exist for the same material. Note the gltf crate doesn't expose the texture transform for the normal and occlusion textures, which it should, so I just ignored those for now. (note by @janhohenheim: this is still the case) Via `cargo run --release --example scene_viewer ~/src/clone/glTF-Sample-Models/2.0/TextureTransformTest/glTF/TextureTransformTest.gltf`:  ## Changelog Support for the [KHR_texture_transform](https:/KhronosGroup/glTF/tree/main/extensions/2.0/Khronos/KHR_texture_transform) extension added. Texture UVs that were scaled, rotated, or offset in a GLTF are now properly handled. --------- Co-authored-by: Al McElrath <[email protected]> Co-authored-by: Kanabenki <[email protected]>
Objective
Support the KHR_texture_transform extension for the glTF loader.
Solution
As is, this only supports a single transform. Looking at Godot's source, they support one transform with an optional second one for detail, AO, and emission. glTF specifies one per texture. The public domain materials I looked at seem to share the same transform. So maybe having just one is acceptable for now. I tried to include a warning if multiple different transforms exist for the same material.
Note the gltf crate doesn't expose the texture transform for the normal and occlusion textures, which it should, so I just ignored those for now.
Via
cargo run --release --example scene_viewer ~/src/clone/glTF-Sample-Models/2.0/TextureTransformTest/glTF/TextureTransformTest.gltf:Changelog
Support for the KHR_texture_transform extension added.