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

Improvements to heighttonormal node #2165

Merged

Conversation

jstone-lucasfilm
Copy link
Member

  • Improve the specification of the heighttonormal node, clarifying that its output contains tangent-space vectors encoded in the [0-1] range.
  • Improve the OSL implementation of the heighttonormal node, leveraging the built-in partial derivative instructions to compute the gradient at each surface point.
  • Simplify example material filenames to improve layout in render tests.
  • Add convolution nodes to the list of standard render tests.

- Improve the specification of the `heighttonormal` node, clarifying that its output contains tangent-space vectors encoded in the [0-1] range.
- Improve the OSL implementation of the `heighttonormal` node, leveraging the built-in partial derivative instructions to compute the gradient at each surface point.
- Simplify example material filenames to improve layout in render tests.
- Add convolution nodes to the list of standard render tests.
@jstone-lucasfilm
Copy link
Member Author

Attaching a reference render comparison between GLSL and OSL using the latest code in this PR:

MaterialXRenderTests_01_02_2025_GitHub.pdf

@jstone-lucasfilm
Copy link
Member Author

Here's a close-up of the latest render comparison for the heighttonormal node. There's still room for further improvement, but this is significantly closer to the mark than our previous results:

HeightToNormal_GLSL_vs_OSL

@jstone-lucasfilm
Copy link
Member Author

I'm CC'ing @dbsmythe, @anderslanglands, @ld-kerley, and @niklasharrysson for their thoughts and guidance, in case they spot improvements that we could make to this changelist.

@jstone-lucasfilm jstone-lucasfilm merged commit d114ab7 into AcademySoftwareFoundation:main Jan 3, 2025
34 checks passed
@@ -1776,7 +1776,7 @@ Convolution nodes have one input named "in", and apply a defined convolution fun

<a id="node-heighttonormal"> </a>

* **`heighttonormal`**: convert a scalar height map to a normal map of type vector3.
* **`heighttonormal`**: convert a scalar height map to a tangent-space normal map of type vector3. The output normal map is encoded with all channels in the [0-1] range, enabling its storage in unsigned image formats.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be explicit here about what each component of the output tangent-space vector3 correspond to?

ie. is it (normal, tangent, bitangent) (N,T,B) or (T,B,N) or something else etc.... the code is already making an explicit choice here - would be good to have the language in the spec reflect that choice.

I believe Ole Gulbrandsen was asking for this language to be explicit somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great suggestion, @ld-kerley, and feel free to propose this improvement in a new issue or PR!

Our implementations all use (T, B, N) as the output order, and it sounds wise to state this in our specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants