AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.87k stars 352 forks source link

Unable to connect layerable BSDFs indirectly as top layer. #627

Closed khang closed 3 years ago

khang commented 3 years ago

Hi,

I've come across what appears to be a limitation in the code generator, at least with GLSL in MaterialXView.

When layering with <layer>, the top layer's BSDF input is strictly limited to only <dielectric_bsdf>, <sheen_bsdf>, and <thin_film_bsdf>. Connecting any other type of node results in an error: Node connected as top layer '...' is not a layerable BSDF.

There are a number of valid use cases where we need to connect these layerable node types indirectly, but currently this isn't allowed.

One example is connecting a <dielectric_bsdf> to a <multiply> to scale the BSDF by some amount, and then layering that on top of another BSDF:

<?xml version="1.0"?>
<materialx version="1.38">
    <dielectric_bsdf name="dielectric" type="BSDF">
        <parameter name="scatter_mode" type="string" value="T" />
    </dielectric_bsdf>

    <multiply name="multiply" type="BSDF">
        <input name="in1" type="BSDF" nodename="dielectric" />
        <input name="in2" type="float" value="0.5" />
    </multiply>    

    <conductor_bsdf name="conductor" type="BSDF">
    </conductor_bsdf>

    <layer name="layer" type="BSDF">
        <input name="top" type="BSDF" nodename="multiply" />
        <input name="base" type="BSDF" nodename="conductor" />
    </layer>

    <surface name="test_surface" type="surfaceshader">
        <input name="bsdf" type="BSDF" nodename="layer" />
    </surface>

    <surfacematerial name="test_material" type="material">
        <input name="surfaceshader" type="surfaceshader" nodename="test_surface" />
    </surfacematerial>
</materialx>

Another example is when the <layer> is contained inside a custom node's nodegraph, and the layerable BSDF (e.g. <dielectric_bsdf>) is connected to it from outside through that node's interface:

<?xml version="1.0"?>
<materialx version="1.38">
    <nodedef name="ND_custom_layer" node="custom_layer">
        <input name="top" type="BSDF" />
        <input name="base" type="BSDF" />
        <output name="out" type="BSDF" />
    </nodedef>
    <nodegraph name="NG_custom_layer" nodedef="ND_custom_layer">
        <layer name="layer" type="BSDF">
            <input name="top" type="BSDF" interfacename="top" />
            <input name="base" type="BSDF" interfacename="base" />
        </layer>
        <output name="out" type="BSDF" nodename="layer" />
    </nodegraph>

    <dielectric_bsdf name="dielectric" type="BSDF">
        <parameter name="scatter_mode" type="string" value="T" />
    </dielectric_bsdf>

    <conductor_bsdf name="conductor" type="BSDF">
    </conductor_bsdf>

    <custom_layer name="custom_layer" type="BSDF">
        <input name="top" type="BSDF" nodename="dielectric" />
        <input name="base" type="BSDF" nodename="conductor" />
    </custom_layer>

    <surface name="test_surface" type="surfaceshader">
        <input name="bsdf" type="BSDF" nodename="custom_layer" />
    </surface>

    <surfacematerial name="test_material" type="material">
        <input name="surfaceshader" type="surfaceshader" nodename="test_surface" />
    </surfacematerial>
</materialx>

Both of these examples will fail with the error mentioned above when opened in MaterialXView. It would be extremely useful if these kinds of layering setups could be made possible!

Thanks, Khang

niklasharrysson commented 3 years ago

Hi @khang,

These are great observations, and your examples actually show two different limitations, where the first one is a fundamental limitation with vertical layering and the second one in more of a codegen limitation.

1: Vertical layering is special in the sense that it's not the user-facing layer node that handles the calculation. Instead the top BSDF is responsible for how to attenuate the light passed down to the base BSDF below, and this is done differently for different BSDFs (a nonlinear operation depending on top BSDF albedo, roughness, IOR, etc.)

To handle this the user-facing BSDF layer stack is transformed into a BSDF nesting internally in the codegen process. As illustrated in this image, the user-facing network A will be transformed into the internal network B:

vertical_layering_transform

As you can see it's impossible the represent other BSDF operators in-between the top BSDF and the layer operator when the network has been transformed.

This is only a limitation with vertical layering (layer), with horizontal layering (mix) your example is fully supported.

For this example you can work around it using pre- or post-multiplication instead. Connect a multiplier to the top BSDF's weight input if it is only the top BSDF you want to attenuate, or connect a multiplier after the layer operator if it's the combined result you want to attenuate.

2: In the second example it's a codegen limitation. Creation of nodegraphs is not as generally supported for BSDFs as with ordinary texture nodes. BSDF nodes has an intricate relationship with the surface shader node they connect to downstream, different for different code generators, to produce the final light integrating shader. And in addition the issues with vertical layering mentioned above. So more work is required in this area.

One solution might be to introduce flattening of all BSDF-type nodegraphs as part of code generation. This would solve the problem with your second example.

Cheers, /Niklas

khang commented 3 years ago

Thank you for the detailed explanation, @niklasharrysson.

While your suggested work around of using the "weight" input of <dielectric_bsdf> is well taken, we'd like a more general solution. Consider the case where we want to vertically layer two dielectric_bsdfs, which are then layered over a diffuse_bsdf. E.g.: image

To attentuate the top dielectric, we could pre-multiply the top dielectric_bsdf via its weight input as suggested. E.g.: image

But what if we wanted to similarly attentuate these layered results before the second layering over the diffuse_bsdf? We wouldn't be able to because there's no such "weight" input on the second <layer> node to pre-multiply. E.g.: image

Hence I'd like to propose two potential changes here:

  1. Add a "weight" input to <layer> or
  2. Allow <multiply> nodes to be layered over BSDFs (ideally, we'd want to be even more general and allow as many of the other nodes to be layered as possible, too).

Another change I'd like to propose is to allow the layering of <mix> nodes over BSDFs, a use case that's not possible right now but is something we see ourselves needing to express. For example, mixing the dielectric_bsdfs in the example above instead of vertical layering, and then layering those results over a diffuse_bsdf or conductor_bsdf.

Looking forward to your thoughts on this, @niklasharrysson.

Thanks, Khang

niklasharrysson commented 3 years ago

Hi @khang,

Your proposal makes perfect sense conceptually. I would love to have support for such a general layer stack where mixtures of BSDFs can be used with vertical layering. But the problem is I'm not sure how we would solve this in practice.

Our current method for vertical layering is to use albedo scaling, where the top layer's directional albedo is estimated and used to scale the light reaching the base layer. We currently support this for a single GGX lobe (and a single Sheen lobe), where we use importance sampled Monte Carlo integration, or curve fits or LUTs to make it more efficient for real-time. If the top layer is no longer a single lobe but a general BSDF mixture we have no good method to estimate the directional albedo of this. We could turn to Monte Carlo integration of the whole mixture, but that's not suitable for real-time targets.

If we could estimate each lobe's directional albedo separately still and mix afterwards it would of course be much easier, but as far as I know it's not a linear operation, so:

dir_albedo( mix(bsdf1, bsdf2) ) != mix( dir_albedo(bsdf1), dir_albedo(bsdf2) )

We could investigate further how large these errors would be in practice though.

It would be interesting to hear if you have more thoughts on the actual implementation of this?

A final side note: We should also consider that we might have more correct methods for vertical layering in the future, that is not only using directional albedo at the surface, but also thickness of the layer and interreflections within. If we allow the layer stack to be too general we might prevent such methods to be supported in the future.

niklasharrysson commented 3 years ago

Hello again,

I need to recant my statement from yesterday about the linearity of this problem. Following the sum rule of integrals we should actually have linearity with directional albedo over a mixture of BSDFs. Sorry I didn't do my homework before making this statement yesterday.

This is great news because then the problem is not so hard to solve after all. Jonathan and I have discussed some changes we can make to how BSDF layering is handled by the code generators, and with this we would be in much better shape to solve all of the points in your proposal. We have to try it out, but if it works out well in practice any BSDF node (including multiply and mix) would be layerable. BSDFs that are not layerable from a physical point of view, for example a conductor, would just be fully opaque if used as top layer.

I will start looking at this change as soon as I get a chance.

Cheers, /Niklas

khang commented 3 years ago

Thanks for the update, Niklas. That's great news indeed, glad to hear that there's a way to solve it. Looking forward to testing the changes whenever they're ready!

niklasharrysson commented 3 years ago

@khang here's an update on this topic.

I have implemented a proof-of-concept of this general layering now in our GLSL code generator. And it works well for all your use-cases above. It's available in my fork of MaterialX if you want to try it out: https://github.com/niklasharrysson/MaterialX/tree/bsdf_refactor.

So that's the good news.

However, in order to move forward with this proposal for MaterialX we need to make sure it's possible to represent in our other back-ends as well, or we loose portability. OSL and MDL are the standard back-ends we have for offline/production rendering. Being closure based (where BSDF evaluation and sampling is handled internally by the renderer) we don't have as much control from the shading language side as with GLSL, so we're more dependent on what is supported in the specification of these languages.

On OSL side there is a project started on updating the set of closures that comes with vanilla OSL, so this is a good timing to look at what layering support will be possible to do there. On MDL side we have started a discussion with the MDL team at Nvidia what is possible to do there.

Cheers, /Niklas

khang commented 3 years ago

@niklasharrysson awesome, I'll check out the fork as soon as I can. Thank you so much!

khang commented 3 years ago

@niklasharrysson it appears that MaterialXGenShader/Nodes/ThinFilmNode.h is missing in the bsdf_refactor branch.

niklasharrysson commented 3 years ago

@khang The thin-film node has been removed since thin-film handling is now built into the layer node.

Make sure the build options for OSL and MDL generation is disabled (MATERIALX_BUILD_GEN_OSL MATERIALX_BUILD_GEN_MDL). These libraries have not been updated in the refactoring yet as there are still questions around if and how they could handle this generalization to layering. Sorry I didn't explain this more clearly above.

Cheers / Niklas

niklasharrysson commented 3 years ago

@khang The BSDF refactoring work has been finalized and is now merged into Lucasfilm main through this PR: https://github.com/materialx/MaterialX/pull/747

This should address the problems raised in this issue, so I think we can close this now. Please let us know of any concerns.

jstone-lucasfilm commented 3 years ago

This looks like a great fix, so I'll close out the original issue. Thanks to @khang for the report and to @niklasharrysson for the fix!

khang commented 3 years ago

Fantastic news. Thank you @niklasharrysson!