AcademySoftwareFoundation
Optimizations to `heighttonormal`
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, and it was additionally inspired by recent discussions with Stephen Hill and Ron Radeztsky at the Lucasfilm ADG.
Below are reference renders of our heighttonormal example material, before and after the optimizations:
Before Optimization:
After Optimization:
Any thoughts on this change from our reviewers, @niklasharrysson, @kwokcb, and @ld-kerley?
Although I don't have any concerns about the math in this change, and our render test suite provides good evidence that it introduces no visual artifacts, I'm hoping to improve our approach for code reviews going forward, where any change of this level of complexity has at least one approval from a MaterialX maintainer.
Don't feel compelled to take a deep dive into the math (unless you want to!), and a basic approval of the high-level approach in this change should be fine.
The question that immediately popped in to my head when I read the code - was that it was very similar, if not the same as the code @bernardkwok suggested in an earlier PR that rendered very differently when you tested it. I was curious what is different this time.
Any then curious if that same difference might apply to the calculatenormal() approach suggested for OSL - which I still think could be a simpler, more efficient, and easier to read implementation - if we can validate they create the same results. I believe now we're passing in an explicit texture coordinate space - the conversion from world space to tangent space could be equally principled as it is here.
The question that immediately popped in to my head when I read the code - was that it was very similar, if not the same as the code @bernardkwok suggested in an earlier PR that rendered very differently when you tested it. I was curious what is different this time.
I think this change handles some edge cases mentioned in previous discussion -- where the gradient is changing rapidly. You don't need the Sobel scaling anymore as well from what I recall. So it seems to add more robustness. Not sure if it's any faster performance wise.
Looks like a good improvement from a high level.
One side note: It would be good to get some "real" geometry other than a sphere which doesn't test boundary cases. e.g. Would be nice to get the shader ball being used instead if testrender could get the chagnes in to handle explicit geometry.
@ld-kerley @kwokcb Yes, Bernard's answer is exactly right, in that his original proposal had the spirit of the right solution, but didn't yet handle mirrored texture coordinates or global scale. This new PR takes these additional steps, giving us a visual match with the original gradient-based logic, without any division instructions being required at runtime.
The calculatenormal approach is interesting as well, and I'd love to learn more about the math that this function performs internally. One potential concern is that the calculatenormal approach needs to transform the heightfield into the world-space coordinates of the geometry, and then transform it back to the desired texture-space coordinates. If we can learn more about the internal math of calculatenormal, we can then potentially modify it to avoid this double transform, and compare the underlying math of these two approaches more directly.