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

Metallic IBL Reflection #1356

Merged
merged 4 commits into from
Apr 27, 2021
Merged

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Apr 20, 2021

The USD Preview Surface shader setup only reflected IBLs (represented by an Arnold SkyDomeLight with a texture) if the specular color was not black. It also did this when the specular workflow was not enabled.

This PR fixes that (and addresses my needs in #933), so that metallic workflows also are capable of reflecting the IBL.

I also took the liberty of removing the autogeneration comments because, as far as we could discern, these are no longer direct results of the auto generation.

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 20, 2021

@mattyjams and @JGamache-autodesk , the logic I'm applying here is pretty simplistic. I'm not sure if the metallic+roughness logic that is in usdPreviewSurfaceLighting.xml should also be represented here or if this is sufficient, since it comes after that particular fragment.

For reference, this is the code from the lighting fragment.

if (!useSpecularWorkflow) {
            float R = (1.0 - ior) / (1.0 + ior);
            float3 specColor = mix(float3(1.0, 1.0, 1.0), diffuseColor, metallic);
            F0  = R * R * specColor;
            F90 = specColor;

            // For metallic workflows, pure metals have no diffuse
            d *= 1.0 - metallic;
        }

At the very least, it allows for a basic reflection estimation in the viewport now
image

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 22, 2021

We've tried to adapt the specular formula into the combiner with some modifications as we believe pertains to reflectivity.
@ysiewappl and I think this should be "close" but without a reference renderer to compare against exactly, we're unsure and would like someone more well versed in the UsdPreviewSurface like @mattyjams to verify when they can.
We are trying to make sure however that we do not modify the unlit mode in any undue way with our changes.

Below you can see a comparison of the before and after of the test sphere with our test IBL, with before and afters of this PR. I include the lit and unlit versions to discern how much things differ. Lit is forward/colorful, unlit is behind and greyscale since the sphere itself has no color. The shader setup in Maya is that the sphere is a light grey, with a black/white checkerboard in the specular/metalness parameters. The IBL is a validation room with a checkerboard pattern and bright text.

metallic

Similarly here is a comparison with the gramaphone asset in the Apple USDZ gallery. Again, each section includes the lit and unlit version of each asset. The IBL is a basic HDR room. I believe however the specularity of the metallic workflow should be tinted by the diffuse color, but I am not sure how to integrate that into this specific fragment.

metallic_gramaphone

In both images you can see that the unlit versions remain unaffected as we hope.
Prior to this PR, you can see that the metallic workflow has no IBL reflection since the shader fragments assumed only specular would reflect.
The luminance of non metallic parts remains consistent between the metallic and specular workflow as can be seen by the gramophone (Edit: the previous commit had a slight lift, but I fixed that and updated this comment and the images)

Note: I have not attempted to update the clear coat logic that @JGamache-autodesk mentioned yet. Perhaps that should be left to a secondary PR, once this one is sorted.

@dgovil dgovil force-pushed the metallic-ibl-reflection branch from 4bde3fa to 617f3c3 Compare April 22, 2021 01:36
Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already looks a lot better for metallic workflows. I have a small thing that might help if you want to try it, otherwise the code is a big step in the right direction.

@JGamache-autodesk
Copy link
Collaborator

There is a nice discussion on metallic looks here if you are interested: PixarAnimationStudios/OpenUSD#1174
Using the provided Damaged_Helmet_usda.zip asset in Maya with USD/pxr/imaging/hdx/textures/StinsonBeach.exr as the environment file should allow comparing the Maya shader against usdView.

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 22, 2021

Thanks for that reference and the pointers, they help clarify quite a bit. I'll iterate a bit more internally and update the PR accordingly.

@dgovil dgovil force-pushed the metallic-ibl-reflection branch from 617f3c3 to 8e3c485 Compare April 23, 2021 15:55
@dgovil dgovil force-pushed the metallic-ibl-reflection branch from 8e3c485 to 8801944 Compare April 23, 2021 17:52
@dgovil
Copy link
Collaborator Author

dgovil commented Apr 23, 2021

I think your suggestions really helped.

Here's a new before and after.
The specular workflow remains unchanged.
Metallic now modulates its specular color by F0/F90 as a function of diffuse color, which means that specular highlights appropriately pick up the nature of the base color for metallic elements.

Let me know if you think there's any other changes I should try here, but otherwise I feel like this is in a good state for me personally.

metallic_gramaphone
metallic

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Apr 23, 2021
@JGamache-autodesk
Copy link
Collaborator

Looks nice indeed. Thanks.
Does not require PF since all changes are in non-compiler VP2 fragment XML files.

@kxl-adsk
Copy link

Looking great indeed. Great job!
How about we get an image base test to make sure results of this hard work are not lost? Here is one example.

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 23, 2021

@kxl-adsk I'll look into that however I had some questions:

  1. The PF doesn't support Arnold which is required for the IBL map. I could spoof it with just colored lights though? Or I could write it to use the Arnold SkyDome and it could be disabled for the PF here.
  2. If we do use the skydome light for the test, is there a specific IBL/HDR etc that your project prefers to use? The gramophone asset is MIT, or the sphere would be a new contribution so either would be fine to add to the test data set. However I do not have a license compatible IBL to add here as part of the test.

@kxl-adsk
Copy link

We are adding support for interactive tests in PF, but we won't have the ability to add them with additional dependencies (like MtoA). So Arnold SkyDome is not something we can use in tests.

@JGamache-autodesk do you have any recommendations on what would make sense to do in a test without Arnold dependency?

@JGamache-autodesk
Copy link
Collaborator

JGamache-autodesk commented Apr 23, 2021

A double metallic/roughness ramp with two directional lights would probably be enough to make sure this does not regress:

from maya import cmds

for i in range(5):
	for j in range(5):
		sphere_xform = cmds.polySphere()[0]
		cmds.move( 2.5*(i-2), 2.5*(j-2), 0, sphere_xform, absolute=True )

		material_node = cmds.shadingNode("usdPreviewSurface", asShader=True)
		material_sg = cmds.sets(renderable=True, noSurfaceShader=True, empty=True, name=material_node+"SG")
		cmds.connectAttr(material_node+".outColor", material_sg+".surfaceShader", force=True)
		cmds.sets(sphere_xform, e=True, forceElement=material_sg)

		cmds.setAttr(material_node + ".diffuseColor", 0.944, 0.776, 0.373, type="double3")
		cmds.setAttr(material_node + ".useSpecularWorkflow", False)
		cmds.setAttr(material_node + ".metallic", i * 0.25)
		roughness = j * 0.25
		if j == 0:
			roughness = 0.001
		cmds.setAttr(material_node + ".roughness", roughness)

With a sky blue directional light rotated -45 in X and a yellow directional light rotated at +45 in X you get this incredibly ugly looking scene that yet makes sure all the metallic features are there. Of course it looks better with an IBL:
image
image
Without the IBL, there is no way to know that this is a gold material.

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 23, 2021

Thanks! I'll get that set up as part of this PR.

@kxl-adsk kxl-adsk added vp2renderdelegate Related to VP2RenderDelegate and removed ready-for-merge Development process is finished, PR is ready for merge labels Apr 23, 2021
@dgovil
Copy link
Collaborator Author

dgovil commented Apr 26, 2021

@kxl-adsk I've added a test to this PR. I also made some modifications to the utility libraries to handle changing cameras and differing color management environments (it was erroring in our environment where we had recently enabled color management workflows)

@kxl-adsk kxl-adsk merged commit acdc492 into Autodesk:dev Apr 27, 2021
JGamache-autodesk added a commit that referenced this pull request Sep 3, 2021
We recently debugged a scene that failed to render correctly with the V2
Light API. The main issue with shadows was fixed in another PR, but there
were still 3 other problems where the Light API V1 was not on-par with USD
and the V2 Light API.

1- Directional light affects IBL lighting

Create a test scene:

- 20x20 plane at the origin, usdPreviewSurface shading, 50% metalness
- aiSkyDomeLight with an environment map
- Directional light with *zero* intensity

Rotate the directional light, and watch how it affects the lighting of the plane. That light has zero intensity, so it should have no effect.

2- textured occlusion does not affect environment lighting

One of the assets in the scene had a textured occlusion map which
correctly worked with scene lights, but not with aiSkyDome lights.

3- specular highlights from regular Maya lights did not have enough base
color for metallic workflows.

That one was fixed for aiSkyDome lights in PR #1356 but direct lighting
still used the old formula.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants