Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Test model for KHR_materials_specular and KHR_materials_ior #268

Closed
wants to merge 8 commits into from

Conversation

proog128
Copy link
Contributor

@proog128 proog128 commented Aug 3, 2020

This is a sample model for KHR_materials_specular (KhronosGroup/glTF#1719) and KHR_materials_ior (KhronosGroup/glTF#1718). It also makes use of KHR_materials_clearcoat and KHR_materials_transmission.

@cx20
Copy link
Contributor

cx20 commented Aug 4, 2020

I experimentally added this model to gltf-test and tested it. The display results of some WebGL libraries are as follows.
I don't understand the glTF extension of this model correctly, so I don't know which library shows the correct result.

Library Result:
three.js image
Babylon.js image
Filament image
PlayCanvas image

@proog128
Copy link
Contributor Author

proog128 commented Aug 5, 2020

Thanks! Babylon.js looks very close to our results. This is our path-traced result:

stellarium_image_43

@donmccurdy
Copy link
Contributor

three.js hasn't implemented these extensions yet, so I doubt we're correct. 😇

@cx20
Copy link
Contributor

cx20 commented Aug 5, 2020

@proog128 Babylon.js is close to a path-traced result, but the windshield is not transparent, is that correct? Is transparent the correct answer?

@bghgary
Copy link
Contributor

bghgary commented Aug 6, 2020

There is no alphaMode specified in the glTF so opaque should be correct.

@proog128
Copy link
Contributor Author

proog128 commented Aug 6, 2020

The transparency on the windshield comes from KHR_materials_transmission (KhronosGroup/glTF#1698). The surface is opaque, but it has a transparent material. See Section about blend modes in KHR_materials_transmission. Another test model for transmission is provided in #265.

@bghgary
Copy link
Contributor

bghgary commented Aug 6, 2020

Yup, totally missed that extension.

@sebavan
Copy link

sebavan commented Aug 6, 2020

But the transmissionFactor is undefined so defaulting to 0 according to the spec. @proog128 shouldn t it kind of disable the extension or "kind of" ?

Also I am seeing 6 in the ior extension which sounds a bit high ?

@emadurandal
Copy link

Hi, there. I'm called by my friend cx20.

@sebavan Hi, IOR 6 is not so unusual in metals.
Some car paint contains metallic composition.

@flashk
Copy link

flashk commented Aug 7, 2020

Shouldn't the clearcoatFactor be set to 1? The clearcoatFactor is currently unspecified, which means it will be disabled according to the spec.

@proog128
Copy link
Contributor Author

proog128 commented Aug 7, 2020

Oh, you are right, the factors are missing. I'll fix that. Thanks!

The car paint is a special case. Usually, an IOR of 6 is too high for dielectrics, and for metals you should use metalness=1. But in the car paint we have a layered mix of dielectric and metal with different colors for each of them. We use an unusually high IOR to increase the contribution of the metal (top layer) over diffuse (base layer).

@cx20
Copy link
Contributor

cx20 commented Aug 7, 2020

I updated the ToyCar model to the latest version.

Model Result
Before image
After image

The windshield has become transparent, but the background behind can be seen through.

Expected Current
image image

@sebavan
Copy link

sebavan commented Aug 7, 2020

Perfect this i all expected as to completely work we also need to render to texture the none transparent parts which is what we have as a work in progress here: BabylonJS/Babylon.js#8623

@MiiBond is taking care of enabling it in our loader, so we are at the correct state at this point :-)

@sebavan
Copy link

sebavan commented Aug 7, 2020

The only strange part is the clear coat ior on the path tracer.

@proog128 it looks like on the path traced model the ior of 6 is applied to the top layer ??? not the metallic one, I kind of assume looking at the render.

Should it be ?

@proog128
Copy link
Contributor Author

proog128 commented Aug 7, 2020

@sebavan With top layer you mean clear coat? No, the clear coat IOR is 1.5. Semantically, the following happens:

fresnel_blend(
  bottom = fresnel_blend(
    bottom = diffuse,
    top = specular,
    ior = 6)
  top = clearcoat,
  ior = 1.5)

(There is no "IOR stacking", the IORs do not influence each other).

A few more images, maybe helpful for debugging:

IOR CC off CC on
1.5 toycar_ior_1 5_nocc toycar_ior_1 5_cc
6 toycar_ior_6_nocc toycar_ior_6_cc

@sebavan
Copy link

sebavan commented Aug 7, 2020

@proog128

(There is no "IOR stacking", the IORs do not influence each other).

This would explain why in Babylon we see such a difference then :-)

Is it part of the spec that they do not influence each other (I might have missed this part) ? so that I ll modify our material accordingly.

Thanks a lot again for your reactivity.

@proog128
Copy link
Contributor Author

proog128 commented Aug 7, 2020

@sebavan Ah ok, that explains why the base layer is so much brighter in Babylon. This is what you mean?

The spec doesn't mention IOR stacking, the example code in the implementation sections doesn't use it. Although I like the idea (after all it's more physically-plausible), it will require the material to be set up with "valid" values for the IOR. And this will break use-cases such as the car paint, where IOR is used in a kind-of spec-gloss workflow. I think it will also reduce compatibility to material models in other renderers, especially if they come from spec-gloss or high IOR for metal etc. For this reason I think we should keep IOR stacking for later, when we have a node-based material system with layering in glTF :-)

@romainguy
Copy link

romainguy commented Aug 21, 2020

Filament is actually closer to Babylon.js for the windows:

Screen Shot 2020-08-21 at 2 23 35 PM

Note that the transmission extension is currently not supported in ubershader mode when too many other extensions are also in use (clear coat in this case). We run out of samplers… We haven't implemented _specular and _ior yet.

I'm surprised the stripes don't show through the front window. Are the stripes an opaque material?

@romainguy
Copy link

Ah I see, the whole car is a single material, which prevents our real-time implementation from working properly. The way it works is:

  • Render all opaque surfaces
  • Capture color buffer, build roughness mip chain
  • Render transmission surfaces, applying transmission using the previous roughness mip chain
  • Render transparent surfaces

Is there any chance we could change the reference model to draw the windows separately? It would make the model correct in real-time renderers.

@sebavan
Copy link

sebavan commented Aug 21, 2020

Yup did not catch this part at first, but we will have the exact same in Babylon or most of the real time renderers.

@emackey
Copy link
Member

emackey commented Aug 22, 2020

@proog128 can you make the change @romainguy mentions? For the sake of realtime renderers, the non-transmissive portions of the car will need to be a separate material without any transmission applied. This allows these parts to be rendered separately as needed.

@proog128
Copy link
Contributor Author

Yes, I will separate the materials.

@proog128 proog128 force-pushed the KHR_materials_specular branch from f550a20 to 5348631 Compare September 21, 2020 09:54
@proog128
Copy link
Contributor Author

Non-transmissive parts are now separated and branch is rebased (merge conflicts resolved).

@cx20
Copy link
Contributor

cx20 commented Sep 21, 2020

I updated the ToyCar model to the latest version.

Model Result
Before image
After image

I think the look of the windshield has been improved.

Expected Current
image image

@UX3D-nopper
Copy link
Contributor

I noticed that a glTF editor called Gestaltor supports this extension, so I displayed the model to try it out.

Library Result
Gestaltor image
@UX3D-nopper The model is up to date, but the windshield doesn't seem to look as expected. Do you have any advice?

Expected Current
image image

My team will have a look at it @UX3D-becher

@romainguy
Copy link

What is the IBL used in all the reference images btw? I'd like to replicate the same lighting.

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

I think it's this?
http://www.hdrlabs.com/test2/store/archive/PaperMill_Ruins_E
https://github.com/cx20/gltf-test/blob/master/textures/hdr/papermill.hdr

@romainguy
Copy link

I realized we were still using our option to change the base layer's color when clear coat is enabled (since enabling clear coat creates a transition from IOR=1.5 instead of IOR=1). With this fixed we're closer to the references (still missing support for the IOR/specular extensions though):

Screen Shot 2020-09-21 at 5 07 44 PM

@sebavan
Copy link

sebavan commented Sep 22, 2020

Actually, we still do have this option turned on as well :-) @Popov72 Could you see if it is possible to add an option to disable it for GLTF ?

@cx20
Copy link
Contributor

cx20 commented May 29, 2021

@bghgary @sebavan It seems that Babylon.js v5.0.0 alpha.18 and 20 changed the display result.
Is this an intended change? I think the previous result was closer to the path-traced result #268 (comment).

Babylon.js + ToyCar(Specular) model result
image
image

@Popov72
Copy link

Popov72 commented May 29, 2021

What is the mode file used in the screenshot?

It seems it is not the ToyCar from the glTF sample repo as the latter one has no stripes.

@cx20
Copy link
Contributor

cx20 commented May 29, 2021

@Popov72

It seems it is not the ToyCar from the glTF sample repo as the latter one has no stripes.

The ToyCar model used for testing is the one included in this PR.
https://github.com/proog128/glTF-Sample-Models/tree/KHR_materials_specular/2.0/ToyCar
-> for KHR_materials_specular and KHR_materials_ior extension

This is a different version from the following model, although the name is the same and therefore confusing.
https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/ToyCar
-> for KHR_materials_sheen, KHR_materials_transmission and KHR_materials_clearcoat extension

Perhaps ToyCar in this PR is confusing and needs to be named differently (e.g. ToyCarSpecular).

@Popov72
Copy link

Popov72 commented May 29, 2021

Between alpha 18 and alpha 20 we have implemented the KHR_materials_specular extension as described here.

That means specularTexture is now giving only the strength of the specular reflection (in the alpha component) and not the color, which is given by specularColorTexture. You will need to modify the KHR_materials_specular entries of the glTF file this way to get the same result than before:

Car:

                "KHR_materials_specular" : {
                    "specularTexture" : {
                        "index": 4,
                        "texCoord": 0
                    },
                    "specularColorTexture" : {
                        "index": 4,
                        "texCoord": 0
                    }
                },

Fabric:

                "KHR_materials_specular" : {
                    "specularColorFactor": [0.729, 0.071, 0.078],
                    "specularTexture" : {
                        "index": 8,
                        "texCoord": 0
                    },
                    "specularColorTexture" : {
                        "index": 8,
                        "texCoord": 0
                    }
                }

@cx20
Copy link
Contributor

cx20 commented May 29, 2021

@Popov72 Thank you for confirming this. I modified the model manually as you said and got the same display results as before.

@proog128 What do you think is the right thing to do with this model?

@proog128
Copy link
Contributor Author

proog128 commented May 31, 2021

@cx20 That's a good question. Although not as polished as the other version with sheen, I think it's still a nice test model to check the specular extension and its interaction with the IOR and clearcoat extensions. In particular the combination of specular and clearcoat is not covered by any other test as far as I know.

Meanwhile I renamed the model from ToyCar to ToyCarSpecular to distinguish the two models.

@cx20
Copy link
Contributor

cx20 commented Jun 1, 2021

@proog128 Thanks for fixing the file name.
However, it seems that the specularTexture is set in duplicate. Could you please check? Probably one of them should be specularColorTexture.

https://github.com/proog128/glTF-Sample-Models/blob/c698c733045c203a3f0a7783677b218bedf56bd9/2.0/ToyCarSpecular/glTF/ToyCarSpecular.gltf#L111-L118

@proog128
Copy link
Contributor Author

proog128 commented Jun 1, 2021

@cx20 Oops, you are right, thanks. Fixed it.

@emackey
Copy link
Member

emackey commented Mar 15, 2022

@proog128 This is an old one, but given that we ended up using a different version of the ToyCar (here), would it be OK to close this version?

Do we still need a test model for metallic IOR colors or anything else from this version that isn't in the other version?

@proog128
Copy link
Contributor Author

The ToyCar model in this PR is a better test-case for the interaction of the specular, ior and clearcoat extensions. I am a bit hesitant to abandon it, because it already found bugs in implementations that are not covered by other test models (as far as I know). Some ideas:

  • Merge PR as it is, so that we have two versions of the ToyCar model (ToyCar and ToyCarSpecular).
  • Use KHR_materials_variants to add an alternative material to the other ToyCar model.
  • Close this PR and create a new test model that checks a couple of corner cases with regard to specular, ior and clearcoat extensions. Maybe with simple geometry like spheres and boxes.

Any opinions?

@emackey
Copy link
Member

emackey commented Mar 22, 2022

  • Close this PR and create a new test model that checks a couple of corner cases with regard to specular, ior and clearcoat extensions. Maybe with simple geometry like spheres and boxes.

I think this might be the best choice, although I'm open to other opinions (@cx20?). I think having a few sample shapes alongside some samples of the correct answers (where possible) can allow folks to more easily spot problems without a trained eye. Also it avoids the "car is different colors in different engines" issue seen in the first few shots on this thread. It would be better for the sample model to highlight an implementation getting the wrong answer, rather than showing an assortment of different interpretations.

Can we start by making a detailed list of which features / interactions need testing?

@cx20
Copy link
Contributor

cx20 commented Mar 22, 2022

I agree with @emackey.
It would be nice to have a simple test case to test this extension.
I think switching materials between ToyCar and ToyCarSpecular with KHR_materials_variants makes the model a bit too complicated.

@proog128
Copy link
Contributor Author

proog128 commented Apr 4, 2022

Can we start by making a detailed list of which features / interactions need testing?

  • Verify that IORs of clearcoat and base layer are independent: same IOR for clearcoat and base does not make the base reflection invisible. Test with something like roughness = 0.2, ior = 1.5, clearcoatIOR = 1.5. Maybe additionally vary the base IOR, like it is done in the (old) ToyCar. Alternatively, use the (old) ToyCar configuration (which should result in a red appearance instead of green).
  • Verify that the min in min(f0_from_ior * specularColor) * specular is correctly applied (only to f0_from_ior * specularColor, not to specular). Could be something like ior = 5, specularColor = [2, 2, 2], specular = 1.
  • Verify that renderer correctly applies the maxValue function. Test with something like roughness = 0.4, baseColor = [1, 1, 1], specularColor = [0, 1, 0]. In the bad case, the higher the IOR, the more purple the base layer becomes. In the good case, the base layer stays gray. The same test case could be run with transmission = 1.

@DRx3D
Copy link
Contributor

DRx3D commented May 11, 2023

@proog128 : This PR uses a branch that has conflicts. We are trying to resolve all PRs in this repo in the next week. If it can be resolved in that time, please do so.

@proog128
Copy link
Contributor Author

Based on the discussion above, I'd suggest to close this pull request. I will come up with a simple test model based on this list.

@proog128 proog128 closed this May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.