Add reflect node and toon shading example

[kwokcb]

Changes

• Add in new <reflect> node to compute reflection across a planes normal
• Add in new "toon" shading example which uses the <reflect> node.

Results

Graphs

New <reflect> graph

Updated <gooch_shade> graph

[jstone-lucasfilm]

Thanks for putting this together, @kwokcb!

Since this proposal includes a reflect node, I'd like to make sure we're including @ld-kerley in the conversation, and comparing the proposed interface against the existing node in Apple's shader graph system:
https://developer.apple.com/documentation/shadergraph/geometric/reflect-(realitykit)

[ld-kerley]

As is "reflected" (pun intended) in the shadergraph documentation Jonathan links above, we would prefer to propose

    <nodedef name="ND_reflect_vector3" node="reflect">
        <input name="in" type="vector3" value="1.0,0.0,0.0" />
        <input name="normal" type="vector3" defaultgeomprop="Nworld" />
        <output name="out" type="vector3" />
    </nodedef>

I think using normal as the input name makes it clearer which of the two inputs is which. In Bernards PR the uiname is called "Normal Vector", so it doesn't seem too much of a stretch to rename the actual input port.

Internally we don't actually specify the default for in as 1.0,0.0,0.0, but I think thats something we'd be open to migrating to whatever everyone thought was reasonable. unit X vector seems as good a suggestion as any - unless anyone thinks otherwise.

While we're here, and because it's also public in the ShaderGraph docs (https://developer.apple.com/documentation/shadergraph/geometric/refract-(realitykit)). The refract node definition we would propose would look like this.

    <nodedef name="ND_refract_vector3" node="refract">
        <input name="in" type="vector3" value="1.0,0.0,0.0" />
        <input name="normal" type="vector3" defaultgeomprop="Nworld" />
        <input name="eta" type="float" value="1.0" />
        <output name="out" type="vector3" />
    </nodedef>

Thats basically what we've already started using internally. I believe that this is inline with what others think this should be - then we could also share the nodegraph implementation of this node as well.

[kwokcb]

  <nodedef name="ND_reflect_vector3" node="reflect">
        <input name="in" type="vector3" value="1.0,0.0,0.0" />
        <input name="normal" type="vector3" defaultgeomprop="Nworld" />
        <output name="out" type="vector3" />
    </nodedef>

Sounds good to me. I originally had "viewdirection" and "normal" as inputs but changed them when I realized Vworld (viewdirection) is only specified in the npr library so can't really be exposed here.

BTW @ld-kerley, someone should probably update the docs for normal. It reads :)

Normal
The vector that the In vector will be reflected about.“Reflected about”? Not sure what that means. Can you clarify?

[jstone-lucasfilm]

I'd recommend "reflected against" if that phrase works for you both, as in the following:

Compute the reflection of an input vector against a normal vector.

[jstone-lucasfilm]

@kwokcb Should we take the additional step of using this new reflect node in the implementation of gooch_shade?

[kwokcb]

Hi @jstone-lucasfilm,
Yes that was what we discussed offline. Wanted to make sure that the definition was agreed upon first.

• The proposal to change the doc phrasing to Compute the reflection of an input vector against a normal vector. sounds okay to me and this does not affect the interface.
• So, if we're good with in and normal, I'll make the change to gooch_shade.

[jstone-lucasfilm]

For the reflect node, one important remaining question is whether the caller or the node itself is responsible for vector normalization.

For the in vector, it seems clear that the caller should be responsible for taking this step, as there are valid use cases for reflecting a non-unit vector against the normal.

For the normal vector, we can use the implementations of reflect in GLSL and OSL as reference points, and both languages leave this as a step for the caller to take.

Here is the language describing the behavior of the reflect function in GLSL:

For a given incident vector I and surface normal N reflect returns the reflection direction calculated as I - 2.0 * dot(N, I) * N.

N should be normalized in order to achieve the desired result.

And here is the corresponding language in OSL:

For incident vector I and surface orientation N, returns the reflection direction R = I
- 2*(N.I)*N. Note that N must be normalized (unit length) for this formula to work
properly

[kwokcb]

This is from MSL:

Seems like GLSL, MSL, OSL indicate N must be normalized within reflect to get a correct result so what's there should remain. Normalization of I should not be within reflect -- so this should change.

refract requires N and I to be normalized so I assume this is done within refract.

[jstone-lucasfilm]

That's an interesting interpretation of the text, @kwokcb, though it's exactly the opposite of my own!

Taking the related snippet from your MSL documentation, they state "the surface normal N needs to already be normalized to get the desired results".

I take that to mean that the caller is responsible for normalization, which increases the possibilities for optimization and numerical stability in MSL logic that would not be possible if the reflect method automatically normalized its input.

[dbsmythe]

I would agree- it’s better to state in the Spec that the input vectors are expected to be normalized if that’s required for the math to work (and so the caller can insert a <normalize> node in the node graph before it if needed), than to add an internal normalization that in many cases would be superfluous and redundant.-DougOn Feb 4, 2024, at 1:31 PM, Jonathan Stone ***@***.***> wrote: That's an interesting interpretation of the text, @kwokcb, though it's exactly the opposite of my own! Taking the related snippet from your MSL documentation, they state "the surface normal N needs to already be normalized to get the desired results". I take that to mean that the caller is responsible for normalization, which increases the possibilities for optimization and numerical stability in MSL logic that would not be possible if the reflect method automatically normalized its input. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[kwokcb]

It was the reflect statement for MSL that threw me off as it says it takes N and computes NN. I'll make the change to normalize outside.

[jstone-lucasfilm]

Sounds good, thanks @kwokcb!

[ld-kerley]

@kwokcb - thanks for flagging the docs - I had also noticed that on Friday and flagged it internally.

+1 for normalize outside the node - Here is the OSL code direct from the repo - it clearly expects things to be normalized already.

If I have cycles this week I'll see if I can put up a complementary refract PR to start the discussion there.

[jstone-lucasfilm]

This looks good to me, thanks @kwokcb!