-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Add specular to MeshPhysicalMaterial and GLTFLoader KHR_materials_specular support #22156
Conversation
@takahirox Thanks for this! The
These properties are for artistic control, and are neither physically-based nor physically plausible. Consequently, I do not think they are appropriate as properties of I think they should only be used to extend |
The
You have renamed these terms to:
I do not think
The so-called |
Many thanks @takahirox! I agree with @WestLangley's suggestions on renaming to |
How about:
Just like |
One other thought on naming. The glTF extension defines
... we could certainly do that, and just require users to pack their textures accordingly. |
404bf7b
to
6c48a6b
Compare
Thanks for the renaming suggestion! I was very unsure if the naming is good, so the suggestion is very helpful. I don't have any big preference about the names but one thing I can say is it may be better to avoid to use the name |
I like even better // I would not add these as properties of the material directly -- only apply them as extensions. EDIT: On second thought, maybe it would be OK to add properties to // Then, in an example, have a GUI checkbox, that allows the user to toggle on various extensions -- in varying combinations. The GUI should also allow the user the alter the primary material parameters. For the geometry, use a sphere -- or polyhedron, if you need to demonstrate flat shading. That should be sufficient to demonstrate the code is working correctly. Anything more complex is a "showcase". |
I think this would be my preference, perhaps supporting only a packed RGBA texture if we are concerned about API complexity. As a bit of context, the spec/gloss PBR workflow is less preferred for glTF moving forward. There was some reasonable concern that the new PBR extensions (transmission, volume, IOR, sheen, ...) × (2x PBR workflows) would create double implementation complexity for little gain. With That being the case, we could actually remove the custom ShaderMaterial that GLTFLoader currently generates for spec/gloss PBR materials, and set up MeshPhysicalMaterial with appropriate specular and IOR properties instead. This comes at a cost (inverting a texture channel, and a more expensive material than the ShaderMaterial) but it would be a nice simplification of GLTFLoader, and help to steer users toward the core materials. |
…transmission example
6c48a6b
to
7457970
Compare
Unfortunately, this implementation is not working... It does not attenuate indirect specular lighting from environment maps properly -- or attenuate it all for that matter. This feature is for artist control, and is not physically-based. This is a reason not to add it to |
As spec/gloss is one of the two dominant PBR workflows, I'm not sure I understand this distinction with specular properties in the metal/rough workflow. The Dassault and Autodesk metal/rough workflows similarly provide specular intensity and tint in some form. My understanding is that it's possible to break the law of conservation with incorrect specular values, or to create physically-implausible materials. But the workflow is still physically-based, expands the range of real materials that the metal/rough workflow can represent, and certainly not all physical materials have dielectric specular F0 == 0.04. |
TDLR; Users can change any parameter in //
I would like to avoid a discussion about semantics, if possible. Unfortunately, that is partly what this is about. :-) The illumination model attempts to model physical phenomenon. Once you modify that model for artistic control, it no longer models physical phenomenon. That is why I prefer to add parameters for artistic controls via an "extension", rather than add them directly to the material.
There is a distinction between this PR and
Right. And Dassault makes clear it is for artistic control.
Right. With this PR it is possible -- but not without it. We try to be careful to ensure that.
I would say an illumination model can be physically-based. A workflow is just a reference to the model parameters. With this PR, the workflow remains the same, but the modified model can violate laws of physics.
I do not see the addition of these parameters as expanding the range of physical materials the model can represent.
We can already handle that.
Also, a glTF extension allows |
@WestLangley You make good points, and in fact these very points were debated at length in the glTF PBRNext working group (which I attend) as this extension was being hammered out. This extension grew out of compromise, which I will briefly summarize here (for the record, I mostly defended your argument during the proceedings).
If this is super important to you, I'd recommend you join the glTF Khronos group. There are a lot of smart people there, and once they set the standard, it becomes hard to push back against. |
Thank you, @elalish, for your comments and explanation. :-)
Ah, I was not aware of that. So we need to accommodate the specular-glossiness workflow in another way... // So it remains:
|
The renders are looking good; is there a reason @WestLangley thinks IBLs are not handled properly in this PR? edit: wait, I see the comment now. Yeah, worth looking into. All the other glTF PBR Next extensions seem to be added directly to |
The spec-gloss properties were not added. And for good reason: total user confusion. As I understand it, this PR is a replacement for that. Also see my comments above regarding |
@WestLangley said
I have fixes for this PR that appear to be working nicely. IBLs now attenuate. I have not addressed area lights, yet, nor clearcoat, so there is still more to do... |
Yes, that's the plan. Sorry I didn't clearly write but the PR comment implied that IBL and other parts needs to be updated later.
I'm happy if you make following up PRs after this PR is merged. |
We currently have So I think |
|
Yes, this would likely be confusing. I'm not sure the docs are really clear (even now) that |
When @bhouston and I discussed the implementation of the However, it is fine with me to deprecate
|
@WestLangley Just to clarify... The current names in this PR are these: this.specularStrength = 1.0;
this.specularStrengthMap = null;
this.specular = new Color( 1, 1, 1 );
this.specularMap = null; What names should we use instead? |
this.specularIntensity = 1.0;
this.specularIntensityMap = null;
this.specularTint = new Color( 1, 1, 1 );
this.specularTintMap = null; |
Yeah, ior is better than reflectivity because it can be used for
transmission/refraction and other effects.
…On Tue, Jul 27, 2021 at 12:57 PM WestLangley ***@***.***> wrote:
so (IMO) the way to improve this would be to deprecate either ior or
reflectivity,
When @bhouston <https://github.com/bhouston> and I discussed the
implementation of the Physical material when it was introduced, we wanted
all parameters to take [0, 1] values. Hence, reflectivity is a
re-parameterization of ior.
However, it is fine with me to deprecate reflectivity.
ior is nominally in [1, 2+].
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22156 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEPV7O3EQWCJRSSUT5JAWLTZ3QQFANCNFSM5AVHUDEA>
.
--
Ben Houston, CTO https://threekit.com
613-762-4113 / http://calendly.com/benhouston3d
|
…rIntensity and specularTint
I have renamed Regarding the |
Thanks! |
Looks like the documentation of |
@chubei-oppen Please do. :-) |
Thanks for your upgrade! |
This PR adds
shininess
andspecular
toMeshPhysicalMaterial
andKHR_materials_specular
extension support toGLTFLoader
.Description
KHR_materials_specular
is a glTF extension which enables to control specular of PBR material.Link:
KHR_materials_specular
extension specificationThe extension adds four specular related properties to PBR material
The Fresnel term
F
will be calculated as the following formulas.This PR adds
shininess
,shininessMap
,specular
, andspecularMap
properties corresponding to the fourKHR_materials_specular
extension properties toMeshPhysicalMaterial
and updates the shader to follow the above formulas.Screenshot
Left: With this PR, Right without this PR. The left specular is a bit brighter because the model has specularColor [2,2,2] or [3,3,3] (default is [1,1,1]).
Model: glTF IridescentDishWithOlives KhronosGroup/glTF-Sample-Models#319
Changes
shininess
,shininessMap
,specular
, andspecularMap
properties corresponding to the fourKHR_materials_specular
extension properties toMeshPhysicalMaterial
KHR_materials_specular
extension support toGLTFLoader
shininess
andspecular
to GUI in thewebgl_materials_physical_transmission
example to check the specularwebgl_loader_gltf_transmission
example with another one because the glTF IridescentDishWithOlives will look nicerNotes
F_Schlick()
and other functions in the shader code are optimized by assuming F90 is 1.0. In this PR I updated onlyF_Schlick()
for the parameterized F90 so far. Perhaps I should have updated other functions, e.g.F_Schlick_RoughnessDependent()
, for the parameterized F90, too, but I was unsure how to apply F90 to them so I didn't touch them so far. (I'm still new to PBR shader code). Following up is very welcome.