-
Notifications
You must be signed in to change notification settings - Fork 393
Unify heighttonormal
node implementations
#2424
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
Unify heighttonormal
node implementations
#2424
Conversation
This changelist unifies the GLSL and OSL implementations of the `heighttonormal` node, using a simplified approach based on the gradient of the heightfield signal with respect to the input texture coordinates. This approach makes the output normal map independent of screen-space size and orientation, and allows the `heighttonormal` node to be applied to procedural heightfields of arbitrary complexity, rather than restricting it to scalar images. The specification for `heighttonormal` has been updated to clarify the role of the `texcoords` input and provide more guidance to renderer authors, though we should ultimately provide complete equations for the behavior of the node in our normative specification.
Attaching a new set of render comparisons between GLSL and OSL 1.12 using the code in this PR: |
How does this render compare to the more straight forward use of |
@ld-kerley I've just posted render comparisons for #2416, so that you can get a sense of how the two approaches stack up visually. |
This changelist implements minor optimizations to the math in GLSL and OSL, without changing the resulting visuals in either language.
This changelist makes adjustments to shader constants in GLSL and OSL, providing better visual parity with traditional Sobel sample filtering.
This changelist adds a guard against missing texture coordinates, falling back to a computation that uses screen-space gradients alone.
Here's a short video captured in the MaterialX Viewer, to provide some visual intuition for how this approach looks in real-time tools: MaterialXView_GradientBasedHeightToNormal.mp4 |
In general, I'm happy with the visual result of this approach, and I believe this usage of gradients harmonizes well with the usage of differentiable shading languages in the future, but I'm very interested in further discussion. In particular, I'm CC'ing @kwokcb, who was one of the main developers of our convolution nodes in the earliest days of MaterialX ShaderGen, and @pablode, who has provided great insights about the generality of our normal mapping approach in recent years. Also, major thanks to @ld-kerley and @HardCoreCodin, who brought up the need to improve this node on Slack, and I'd love their thoughts as well. |
This changelist implements fixes and clarifications to the math of the GLSL and OSL logic, including a more robust implementation of the chain rule.
- Flip dVdS for visual consistency. - Fix OSL syntax error.
This changelist fixes the fallback for missing texture coordinates, which should be zero instead of one.
This changelist refactors the use of negations for clarity, without making any changes to the visual output.
This changelist adds documentation for the origins of the Sobel scale factor, since it may be unknown to many readers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if using a straight cross-product vs Jacobian might be more stable -- depending on the amount of UV distortion.
e.g: for things like: poles of a lat-long sphere,, seams,
small uv area stretched to large screen-space area (or reverse), mirror or wrapping (the example height map does not go across uv boundaries), perspective distortion etc.
vec3 tangent = normalize(vec3(dUdS.x, dVdS.x, dHdS.x));
vec3 bitangent = normalize(vec3(dUdS.y, dVdS.y, dHdS.y));
vec3 n = normalize(cross(tangent, bitangent));
I assume we don't want to branch implementations but a possible choice for real-time ?
dHdT *= SOBEL_SCALE_FACTOR; | ||
|
||
// Convert the gradient to a normal and encode for storage. | ||
vec3 n = normalize(vec3(dHdT.x, dHdT.y, 1.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the normal is inverted (pointing inward) and you may have issues with numeric instability if dHdT is very large (Z map become very small). Suggested:
float z_scale = max(1.0, length(dHdT));
vec3 n = normalize(vec3(-dHdT.x, -dHdT.y, z_scale));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwokcb I'll run the full Render Test Suite with latest code, just to make sure, as there have been lots of minor improvements in recent days.
I'm not sure running MaterialX View is exactly equivalent to the Render Test Suite, as in the latter case we're making adjustments to our conventions to match OSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my eye, this confirms that the orientation of the normal map is the preserved by the new implementation, and it seems to provide better handling of edge cases where the tiling image is not aligned with the screen plane.
In this same render test run, I'm seeing some stretching issues in OSL that are likely caused by one of my recent optimizations, so I'll fix this up before posting a full PDF of the render comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What confused me visually to make me "see" the normals as facing "in" vs "out" was that "up" orientation differs for the MaterialXViewer vs unit tests. Sorry for the mistaken "noise".
It would be good to address the image flip inconsistency at some point.
} | ||
else | ||
{ | ||
dHdT = vector2(0.0, 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems you might get discontinuities when the determinant is close to 0 but not 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking here is that a determinant with an absolute value less than 1e-8
represents a shading point with degenerate texture coordinates, so falling back to a flat normal seems like the best we can offer.
Let me know, though, if you have other suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I can think about here is to use the original geometries normal when the determinant is very small. I guess it would mostly occur at discontinuities in UV space so it may not match anyways if you use the original normal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a "flat" normal map should be exactly equivalent to using the original normal of the geometry at that point, as if the normal map was not present at all.
@kwokcb I tried out your alternative implementation, using the following code block, but I don't believe it captures the subtle gradations of the normal map as effectively as using the full chain rule:
Here's a render of the full chain rule implementation in GLSL: And here's a render of the alternative implementation in GLSL: |
Agreed the result you have is much better. Just a suggestion for a simpler formulation. |
Ok, here's a full PDF for the Render Test Suite, using the changes in this PR: |
To my eye, the new approach preserves the orientation of normals from the original Sobel, but with more visually consistent results at grazing angles, and a measurably closer match between GLSL and OSL. |
So far, all of the feedback on this change has been positive, and I wanted to check whether anyone would object to our moving forward with this approach. |
Thanks to everyone for the discussion and math suggestions, and let's move forward with this change! |
dfc2fe5
into
AcademySoftwareFoundation:main
This changelist implements optimizations to our gradient-based implementation of `heighttonormal` in GLSL and OSL, using a cross product in place of the inverse Jacobian. The new version requires no division instructions, and generates virtually identical results. Credit for this idea is due to @kwokcb, who originally suggested it in a [comment](AcademySoftwareFoundation#2424 (review)), and it was also inspired by recent discussions with Stephen Hill and Ron Radetsky at Lucasfilm.
This changelist implements optimizations to our gradient-based implementation of `heighttonormal` in GLSL and OSL, using a cross product in place of the inverse Jacobian. The new version requires no expensive division operations, and generates virtually identical results. Credit for this idea is due to @kwokcb, who originally suggested it in a [comment](AcademySoftwareFoundation#2424 (review)), and it was also inspired by recent discussions with Stephen Hill and Ron Radeztsky at the Lucasfilm ADG.
This changelist implements optimizations to our gradient-based implementation of `heighttonormal` in GLSL and OSL, using a cross product in place of the inverse Jacobian. The new version requires no division instructions at runtime, and generates virtually identical results. Credit for this idea is due to @kwokcb, who originally suggested it in a [comment](AcademySoftwareFoundation#2424 (review)), and it was also inspired by recent discussions with Stephen Hill and Ron Radeztsky at the Lucasfilm ADG.
This changelist implements optimizations to our gradient-based implementation of `heighttonormal` in GLSL and OSL, using a cross product in place of the inverse Jacobian. The new version requires no division instructions at runtime, and generates virtually identical results. Credit for this idea is due to @kwokcb, who originally suggested it in a [comment](AcademySoftwareFoundation#2424 (review)), and it was additionally inspired by recent discussions with Stephen Hill and Ron Radeztsky at the Lucasfilm ADG.
This changelist implements optimizations to our gradient-based implementation of `heighttonormal` in GLSL and OSL, using a cross product in place of the inverse Jacobian. The new version requires no division instructions at runtime, and generates virtually identical results. Credit for this idea is due to @kwokcb, who originally suggested it in a [comment](#2424 (review)), and it was additionally inspired by recent discussions with Stephen Hill and Ron Radeztsky at the Lucasfilm ADG.
This changelist unifies the GLSL and OSL implementations of the
heighttonormal
node, using a simplified approach based on the gradient of the heightfield signal with respect to the input texture coordinates.This approach makes the output normal map independent of screen-space size and orientation, and allows the
heighttonormal
node to be applied to procedural heightfields of arbitrary complexity, rather than restricting it to scalar images.The specification for
heighttonormal
has been updated to clarify the role of thetexcoords
input and provide more guidance to renderer authors, though we should ultimately provide complete equations for the behavior of the node in our normative specification.