Conversation
|
Thank you for the pull request, @danielzhong! ✅ We can confirm we have a CLA on file for you. |
|
I did not yet thoroughly read the spec, but will have a closer look (and at least try out this PR) soon. I'm sure Sean can provide more profound feedback about things like best practices for this, and the implementation approach in general. |
javagl
left a comment
There was a problem hiding this comment.
Some "minor/small" things, and a few high-level comments for now:
The
Specs/Data/Models/glTF-2.0/ConstantLod/gltf/checker_txture.png
is actually a WEBP file (wrong file extension).
(This causes a validation error because of the mismatch between the MIME type and the actual type, and because WEBP would require an extension)
(There's an archive attached blow that contains the PNG version)
The ModelComponents constantLod is documented to be
@type {object}
but must be
@type {object|undefined}
More generally: Several functions in the MaterialPipelineStage now receive RenderResources as a parameter. This parameter is usually not documented. In the cases where it is documented (e.g. in processConstantLod), it is typed as
@param {PrimitiveRenderResources}
but should be
@param {PrimitiveRenderResources|undefined}
(Admittedly, some parameters (like the "default texture") are generally not documented, *sigh*... just mentioning it...)
While according to the spec the extension can be present on any
textureInfoproperty, this implementation only supports it onbaseColorTextureandnormalTextureto a limited extent.
I understand the reasoning behind that. Supporting it everywhere could be a lot of effort. Roughly speaking: Every call to processTexture would need the renderResources to be passed in (raising the question of whether they can still be undefined in the JSDoc). This would likely affect shader in many places as well....
Not in the scope of this PR, but ...
...out of curiosity, I created an example where both the base color and the emissive texture contain the extension:
ConstantLod_CheckerEmissive.zip
(This contains the checker texure as a PNG)
It works within the constraints that are established here (i.e. only for the base color):
(Why does the emissive texture appear to be rotated by -90 degrees? Haven't investigated that yet...)
The normal map texture limitation is ...[...]
[...] to match the iTwin.js implementation
Again, I see the reasoning behind that. How a "full, proper" support could be implemented is probably beyond the scope of this PR. But one could still consider to track the current limitations in an issue (maybe after/when this PR is merged)
I could imagine that there are some tricky considerations for the cases where this extension is supposed to be combined with KHR_texture_transform. If it is not allowed, then this should be mentioned in the specification. If it is allowed, I could create some test asset for that.
I might do another pass, depending on the responses and the desired level of nitpicking strictness for the review.
|
@javagl
I'm confused by this as well and took a look to see if the orientation of the emissive texture was changed when I removed the extension. Here's the model as-is vs. when I remove this extension from both textures (base color & emissive). I think this indicates a bug with the orientation of the UVs produced by the constant LOD shaders?
I don't in theory see a problem with them being combined... I just did a quick test where I added the following to the ConstantLod_Checker.gltf's "KHR_texture_transform": {
"offset": [0.1, 0.1],
"scale": [2.0, 2.0],
"rotation": 0.75
}and it seems like |
Co-authored-by: Mark Schlosser <47000437+markschlosseratbentley@users.noreply.github.com>
I probably should have said that more explicitly: I think that this is not related to this PR. (It may be worth examining that further, and maybe opening a dedicated issue - and it's quite odd that this seems to happen only for the emissive texture (until now) - I'll take a TODO on my list to check this, for emissive, occlusion, etc., and maybe open an issue (or PR) eventually)
I mentioned "tricky considerations", but the first one is relatively easy: Should it be possible to combine them (in a form where they both have the intended effect)? If the answer to that is "No", then one could simply say in the spec: "This extension cannot be combined with If the answer is "Yes, it should be possble to combine them", then the tricky considerations come in - namely, about the where and how. I have not yet thought that through. The second place that you linked to looks like one that could make sense - i.e. transforming the Maybe the answer to the first question is 'No'...? |
Based on the screenshots I included, is the fact that the base color texture is a different orientation w/ and w/o this new extension not a concern? that's what made me think it's related to this PR, but I'm not familiar with any existing problems with UVs so could be wrong. Here's a clearer comparison of what I mean:
(See the orientation of the numbers and letters in the texture)
I feel like the answer is "yes it should be possible, but out of the scope of this issue since it's not a common/high priority use case"-- I'm hesitant to explicitly say it's not allowed in the spec, but maybe we don't have to spend the time on implementing it yet since it hasn't come up until now when discussing design models that use @weegeekps do you think this is fair on the topic of combining |
|
Ahhh 💡 ... sorry, I completely missed that. (For some reason, I was only looking at that "Emissive" string and thought "Well, it's 'wrong' in both cases?!?") But now I see: Yes, apparently the extension "rotates" the texture coordinates, and that should not be the case. I said "rotates" in quotes, because it might simply be a mixup of Quickly skimming over the code: There's that |
|
After discussion with @markschlosseratbentley (and his discussion with @weegeekps), this is a summary of remaining issues on this PR that I am working on:
|
Also tested normal map support for both texture transform and CLOD. I added a normal map that matches the checkerboard color texture so it would be easy to see if they are aligned.
The difference between these images is a little hard to see, but the normal map makes the right image's letters and some square borders appear darker/more pronounced. You can tell both textures are transformed since they are aligned.
|
|
@javagl Adam gave the interaction with |












Description
This PR adds support for the new
EXT_textureInfo_constant_lodglTF extension. See the extension spec PR CesiumGS/glTF#92 which is in the final stages of review.Constant level-of-detail ("LOD") is a technique of texture coordinate generation which dynamically calculates texture coordinates to maintain a consistent texel-to-pixel ratio on screen, regardless of camera distance. As the camera zooms in or out, the texture coordinates are recalculated so that the texture pattern remains at approximately the same visual scale. The transition between scale levels is smoothly blended to avoid abrupt changes. The general use case of this is to create textures that look good at different zoom levels, commonly for surfaces like grass or gravel on a ground plane in large models.
See the spec README for detailed info about the formulas used.
Notes:
textureInfoproperty, this implementation only supports it onbaseColorTextureandnormalTextureto a limited extent.emissiveTexture,occlusionTexture, or any othertextureInfoModel with
EXT_textureInfo_constant_lodgrass texture very zoomed in:Same model zoomed out. Note how the texture remains at the same visual scale:
Gif from iTwin.js to illustrate the effect:
Issue number and link
#12892
Testing plan
I added a new dev Sandcastle to test this feature for two different base color textures, as well as a normal map. See the test models in
Specs/Data/Models/glTF-2.0/ConstantLod/gltf/. I also added some unit tests to GltfLoaderSpec.js.Test models free texture sources:
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change