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

Add support for viewdirection space in hardware shading #2036

Conversation

edobrowo
Copy link
Contributor

@edobrowo edobrowo commented Sep 28, 2024

This addresses issue #1656.

Previously, attribute space in node viewdirection_vector3 was unused by GLSL and MSL implementations. This change correctly transforms the view direction from world space to the specified space.

Copy link

linux-foundation-easycla bot commented Sep 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@jstone-lucasfilm
Copy link
Member

@edobrowo This looks really promising, and to resolve the EasyCLA issue, I'd recommend following up through the support link that the Linux Foundation provides:

https://jira.linuxfoundation.org/servicedesk/customer/portal/4

@edobrowo
Copy link
Contributor Author

I've made the ticket, thank you!

I'm not entirely sure why the view direction is not properly transformed in the object /model case. Also, it seems to be unaffected by any transformation, not just the world inverse transpose.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks like a good start, though I think it's a larger set of changes than you need, and I'd recommend the following simplifications:

  • Instead of replacing the computation of world-space view direction with new vertex shader logic, I'd leave it as a pixel-shader subtraction of view position from vertex position, as this should provide the most accurate per-pixel values.
  • Then, for your new object-space view direction, you can simply apply the T_WORLD_INVERSE_TRANSPOSE_MATRIX transform to the world-space view direction that you compute in the pixel shader, just before the vector has been normalized.
  • Finally, you can normalize your world/object view direction, making it a unit-length vector that can be returned as a result.

@edobrowo
Copy link
Contributor Author

Thank you for the feedback! I made the requested changes, though the issue still persists. It seems like the transformation isn't being applied; why could this be the case? Thank you!

@jstone-lucasfilm
Copy link
Member

@edobrowo This looks like a good step forward, and here are three suggestions for debugging the math of the new logic:

  • One useful debugging tool that you can use here is the G key in the MaterialX Viewer, which writes the generated GLSL code out to a file on disk, where you can read it directly and compare it against what you expected to see.
  • For additional detail, I'll sometimes place breakpoints in the new code path that I've written, allowing me to validate that my new code is executed, and that the inputs to the function match my expectations.
  • As an external reference point, you might compare your new logic against the equivalent code in https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenShader/Nodes/HwNormalNode.cpp, just to make sure you're not missing any important steps in your object-space code path.

@edobrowo
Copy link
Contributor Author

edobrowo commented Oct 3, 2024

@jstone-lucasfilm Thanks for the tips. I've been mainly testing in the graph editor and the viewer. I realized that the world transform, inverse transpose, etc. are all the identity matrices, since presumably object space is equivalent to world space for viewing single objects. This also explains why the output to normal_vector3 doesn't vary with the input space either.

I tested by hardcoding arbitrary mat4 transformations into the object-space path, and confirmed that when the node input is set to object or model space, the transformation is in fact applied. What would be an effective way to create a test case where the object is offset from world space? Thank you again.

@jstone-lucasfilm jstone-lucasfilm changed the title Update viewdirection_vector3 to correctly utilize attribute space Add GLSL support for the viewdirection space attribute Oct 3, 2024
@jstone-lucasfilm jstone-lucasfilm changed the title Add GLSL support for the viewdirection space attribute Add GLSL support for viewdirection space attribute Oct 3, 2024
@jstone-lucasfilm
Copy link
Member

@edobrowo Good catch, and that makes sense. We don't yet have an effective way to test non-identity world-space transforms in the viewer and graph editor, and this would likely be a good candidate for a future improvement to MaterialX!

Given that context, this proposed change looks good to me, and I'll go ahead and run a few validations before merging.

@jstone-lucasfilm jstone-lucasfilm changed the title Add GLSL support for viewdirection space attribute Add support for viewdirection space in hardware shading Oct 3, 2024
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Great work on this project, thanks @edobrowo!

@jstone-lucasfilm jstone-lucasfilm merged commit 860b8d4 into AcademySoftwareFoundation:main Oct 3, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants