Skip to content
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

StainedGlassLamp model #292

Merged
merged 10 commits into from Nov 3, 2021
Merged

StainedGlassLamp model #292

merged 10 commits into from Nov 3, 2021

Conversation

echadwick-wayfair
Copy link
Contributor

@echadwick-wayfair echadwick-wayfair commented Jan 29, 2021

An example model to test and demonstrate KHR_materials_clearcoat, KHR_materials_ior, KHR_materials_translucency, KHR_materials_transmission, KHR_materials_variants, KHR_materials_volume, and KHR_texture_basisu.

Re-posting #288 into its own branch, with slight updates, and including the source model.

screenshot_large

An example model to test and demonstrate KHR_materials_clearcoat,  KHR_materials_ior, KHR_materials_translucency, KHR_materials_transmission, KHR_materials_variants, KHR_materials_volume, and KHR_texture_basisu.
@cx20
Copy link
Contributor

@cx20 cx20 commented Jan 29, 2021

I think this model is very nice. However, I am worried about the bloated repository size.
The average size of the folders in the other sample models is 12-13MB, so this model is much larger than that.

Folder Name Folder Size
2.0/StainedGlassLamp/glTF 29.3 MB
2.0/StainedGlassLamp/glTF-Binary 29.3 MB
2.0/StainedGlassLamp/glTF-KTX 13.2 MB
2.0/StainedGlassLamp/glTF-Translucency 27.5 MB
2.0/StainedGlassLamp/glTF-Variants 29.4 MB
2.0/StainedGlassLamp/screenshot 3.4 MB
sourceModels/StainedGlassLamp 60.5 MB
Total 192.6 MB

For reference, here is the folder size information for other samples.
image

@echadwick-wayfair
Copy link
Contributor Author

@echadwick-wayfair echadwick-wayfair commented Feb 1, 2021

Thanks for this. A few ideas:

  1. Should I compress each folder with KTX, then remove the redundant \glTF-KTX ?
  2. Should we keep the textures in sourceModels as PNG files?
  3. Should I replace the (larger) .Max model in sourceModels with FBX or OBJ (smaller file)?
  4. Should down-sample the PNGs in sourceModels to match the image sizes used in the glTFs? These are mostly 2048 PNGs.
  5. Should I remove any of the model versions?

@lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Feb 1, 2021

Should I compress each folder with KTX, then remove the redundant \glTF-KTX ?

No, since people implementing new PBR extensions may lack KTX support.

Should we keep the textures in sourceModels as PNG files?

Yes.

Should I replace the (larger) .Max model in sourceModels with FBX or OBJ (smaller file)?

The sourceModels asset should contain proper materials, so OBJ is probably not an option.

Should down-sample the PNGs in sourceModels to match the image sizes used in the glTFs? These are mostly 2048 PNGs.

This repo has lots of 2048 PNGs, so it should be fine. In fact, it may be even better to use the same PNGs (if possible) for source models and glTFs so that git could deduplicate them internally.

Should I remove any of the model versions?

In my opinion, the GLB version doesn't add much value to this sample model.


Also please remove the validation info from the readme since that'll change eventually.

@cx20
Copy link
Contributor

@cx20 cx20 commented Feb 2, 2021

  1. Should I remove any of the model versions?

I compared each model with Babylon.js. It looks the same as glTF / glTF-Binary / glTF-KTX / glTF-Variants (Lamp on).
What about the idea of removing the same looking glTF / glTF-Binary and promoting glTF-Variants to glTF?
If we remove the two variations, we save 60 MB.

image image image
image image image

I will link to the following issue as a related topic.
#272

* Redundant GLB removed.
* Variant promoted to \glTF.
* 2048x2048 textures duplicated from sourceModel, to enable git to deduplicate them internally.
* Renamed assets to use "StainedGlassModel" prefix instead of "FDLV3095" SKU prefix.
* glTF-KTX recompressed using ETC1S, to get the asset closer to the file size recommended by 3D Commerce guidelines.
* DS-PBR render updated.
* Removed validation info from README as it will be outdated soon.
* Various small README improvements.
* Removed the model with KHR_materials_translucency, until it's been ratified.
* Changed the KTX model to use no extensions except KHR_textures_basisu.
* Added a model using JPG and PNG textures, with no extensions, to compare file sizes between it and the KTX version.
@cx20
Copy link
Contributor

@cx20 cx20 commented Feb 26, 2021

@echadwick-wayfair Thank you for organizing the folder of models.

Is it possible to change the folder name to glTF-BasisU instead of glTF-KTX?
The reason for this is that the folder name for the KHR_texture_basisu version of FlightHelmet is glTF-BasisU.
See: #285

And if possible, To be consistent with other samples, it is better to name the sample model file the same as the folder name is a good idea.

Folder Name File Name
Duck/glTF Duck.gltf
Duck/glTF-Binary Duck.glb
Folder Name File Name
StainedGlassLamp/glTF StainedGlassLamp.gltf
StainedGlassLamp/glTF-JPG-PNG StainedGlassLamp.gltf
StainedGlassLamp/glTF-BasisU StainedGlassLamp.gltf

@rsahlin
Copy link

@rsahlin rsahlin commented Mar 4, 2021

Is it possible to change the folder name to glTF-BasisU instead of glTF-KTX?
The reason for this is that the folder name for the KHR_texture_basisu version of FlightHelmet is glTF-BasisU.
See: #285

Hi @cx20 and thanks for your comments.
I think we should think about this for a moment.
As I understand we should start using KTX2 (or simply KTX) when referring to 'supercompressed' (BasisU) assets.
This is so that we have a consistent way of communicating.

I know that the extension that adds the support for supercompressed textures is called KHR_texture_basisu but the carrier is KTX (version 2)

@cx20
Copy link
Contributor

@cx20 cx20 commented Mar 4, 2021

@rsahlin Thanks for letting me know about your concerns.
I'm not too particular about the folder name glTF-BasisU. I simply suggested it as a way to make folders with similar meanings more consistent in the repository.

@bghgary Do you have any advice on folder names for this model?

@bghgary
Copy link
Contributor

@bghgary bghgary commented Mar 5, 2021

As I understand we should start using KTX2 (or simply KTX) when referring to 'supercompressed' (BasisU) assets.

Hmm, I haven't heard this. KTX files are not necessarily supercompressed BasisU files.

I would recommend calling it glTF-BasisU.

@cx20
Copy link
Contributor

@cx20 cx20 commented Mar 5, 2021

For reference, I have listed the names currently used for glTF folders. So far, glTF-JPG-PNG and glTF-KTX have not been used.

glTF
glTF-BasisU
glTF-Binary
glTF-Draco
glTF-Embedded
glTF-Embedded-buffer
glTF-IBL
glTF-pbrSpecularGlossiness
glTF-Quantized

(I don't think there will be any significant impact by increasing the folder name. The only impact would be that I would have to add a few more if statements when I use gltf-test to display it.)

image

@rsahlin
Copy link

@rsahlin rsahlin commented Mar 5, 2021

I would recommend calling it glTF-BasisU.

What would the drawback be with calling it glTF-KTX2?

From a broader ecosystem perspective I think it makes sense to try to unify how we name our features.
I know that historically we usually use the technical name of the feature.
In this case, to make it easier for the rest of the ecosystem to understand I think we should consider using KTX2 instead of BasisU.

@lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Mar 5, 2021

  • The glTF extension is called KHR_texture_basisu.
  • KTX v2 can handle much more payload formats than just Basis Universal supercompression.
  • There will be other KTX use cases within glTF.

That said, we could use glTF-KTX-BasisU to give the most precise context possible.

@emackey
Copy link
Member

@emackey emackey commented Mar 7, 2021

Once the folder name is settled (and it looks like it is, from @lexaknyazev's comment above), we should update the files here and in other PRs that offer KTX, such as #285.

* Copyright section added, Generator section shortened.
* glTF files renamed to match root folder.
* JPG-PNG version recompressed to use mostly JPG files.
* KTX replaced with high-quality version created by @rsahlin .
* KTX folder renamed to match convention.
* Readme updated with new screenshots and information.
echadwick-wayfair added a commit to KhronosGroup/3D-Formats-Guidelines that referenced this issue Mar 8, 2021
For details, see readme.md in push 8bee860 in pull request KhronosGroup/glTF-Sample-Models#292
Correcting the readme: only the baseColor textures with alpha were kept as PNG. Normal maps were not kept as PNG, they were converted to JPGs along with all the rest. Also added the reasoning behind keeping those PNGs.
Model proportions adjusted to match reference photo better. Stained-glass textures recreated to reduce stretching & aliasing. Stained-glass transmission roughness reduced to improve translucency. Normal map "scale" reset to 1 to improve bump quality. KTX vs JPG chart added to readme.
@echadwick-wayfair
Copy link
Contributor Author

@echadwick-wayfair echadwick-wayfair commented Mar 30, 2021

I noticed KTX2 model has some problems:

I'm re-compressing the textures to see if this fixes the problems.

@echadwick-wayfair
Copy link
Contributor Author

@echadwick-wayfair echadwick-wayfair commented Mar 30, 2021

Ah it was missing the extension definitions in the Textures section!
image

I'll submit a fix soon.

The KTX glTF was refusing the load in glTF Sample Viewer and <model-viewer>, so I added missing data to the glTF: extensionsUsed, extensionsRequired, extensions within Textures. Recompressed the textures too, just in case.
@bghgary
Copy link
Contributor

@bghgary bghgary commented Apr 7, 2021

Somewhat minor, but there is a seam on the lamp. It appears in at least in the bump normal, roughness, metallic maps. Not sure what's causing it.

image

* No emissive on gems, beads.
* Roughness adjusted for glass w/ transmission.
* Volume & IOR added to gems/beads.
* Various geometry fixes: Cord exit added under base, fixed interpenetrations between steel hangers and amber beads.
* Texture tiling fixes on stained glass. (thanks for the nudge Gary!)
* Brighter emissive for top part of base.
* Re-compress all KTX with latest RDO & Linear parameters.
* A/B/C versions created: no extensions, clearcoat/transmission/variants, clearcoat/transmission/variants/volume/IOR.
* File size/memory size data collected for all versions.
* Pathtracer version created for dsPBR use, with emissive textures disabled and bulb emissiveFactor increased to 100.
* Screenshots created with Babylon.js, using manual override to avoid transmission roughness bug (Volume IOR = 1.15).
* GLBs uploaded to 3D-Formats-Guidelines for demo use.
Copy link
Contributor Author

@echadwick-wayfair echadwick-wayfair left a comment

The pathtracer version created for dsPBR use is actually in a separate commit, see https://github.com/KhronosGroup/3D-Formats-Guidelines/tree/main/samples/StainedGlassLamp

@cx20
Copy link
Contributor

@cx20 cx20 commented Apr 10, 2021

@echadwick-wayfair

I tried the latest version of the model locally.
However, Lamp on is not displayed in Variants. Is this the intended change? I think it was displayed before.

"extensions": {
"KHR_materials_variants": {
    "variants": [
        {
            "name": "Lamp off"
        }
    ]
}

@echadwick-wayfair
Copy link
Contributor Author

@echadwick-wayfair echadwick-wayfair commented Apr 10, 2021

Thanks for the review. Yes that is intended. It seems to me the "lamp on" variant is redundant since it's a duplicate of the original state.

@cx20
Copy link
Contributor

@cx20 cx20 commented Apr 10, 2021

Thanks for the review. Yes that is intended. It seems to me the "lamp on" variant is redundant since it's a duplicate of the original state.

I see, it is the same idea as the default camera.
I will have to modify the example program because I have not taken into account the default state of variants in the gltf-test.
Currently, gltf-test just shows the list in the extension in variants.

image

[updated 2021.04.11] I added the default state to variants in gltf-test.
I have modified the gltf-test example to show the default state to variants in Babylon.js and Three.js. I don't know how to change the default material in RedCube.js, so I need to check with the library author.
image

* Model proportions adjusted to match reference photo better.
* Stained-glass textures recreated to reduce stretching & aliasing.
* Stained-glass transmission roughness reduced to improve translucency.
* No emissive on gems, beads.
* Volume & IOR added to gems/beads.
* Various geometry fixes: Cord exit added under base, fixed interpenetrations between steel hangers and amber beads.
* Texture tiling fixes on stained glass. (thanks for the nudge Gary!)
* Brighter emissive for top part of base.
@echadwick-wayfair
Copy link
Contributor Author

@echadwick-wayfair echadwick-wayfair commented Jun 7, 2021

Is there anything else that needs to be done with this asset to resolve the merge? Thanks.

@emackey
Copy link
Member

@emackey emackey commented Jun 7, 2021

The choice between "Lamp off" vs "default" seems far less intuitive than the old "Lamp off" vs "Lamp on". I think the KHR_material_variants spec itself needs a bit of clarification. Perhaps we can give the default state a name somehow, or ask clients to detect whether the default state is redundant with the named states and simply not show it when it's redundant. Or we could ask implementors of variants to always include the default state among the variants, and say that clients are not required to offer the default state, and should instead detect its name from matching it to the list of named states or something.

It seems like something that needs discussion at the spec level before we commit to a model like this one that forces clients to show a nameless state.

1. Variant added for the "on" state, so the model has a specific variant for the default state. See pull request 292 for discussion on default variant states.

2. Links to specific pages on Wayfair.com have been removed from the readme; this is no longer an active product so it no longer has a "live" webpage.
@emackey
Copy link
Member

@emackey emackey commented Nov 3, 2021

Thanks again @echadwick-wayfair for this amazing model!

@emackey emackey merged commit 7100cd5 into master Nov 3, 2021
2 checks passed
@emackey emackey deleted the stained-glass-lamp branch Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants