Fix OSL transformmatrix vector3M4 translation - #2843
Conversation
|
|
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
This looks very promising, @aviralgarg05, and I had just one suggested improvement for the new test that you've added.
| TEST_CASE("GenShader: OSL TransformMatrix Point Semantics", "[genosl]") | ||
| { | ||
| const std::string expectedSourceCode = "transform({{mat}}, point({{in}}))"; | ||
| const std::string testDocumentString = R"(<materialx version="1.39"> |
There was a problem hiding this comment.
One downside of inlining an example file is that it won't be included when we perform syntax upgrades and reformatting on our example data, and a better approach would be to leverage our standard test suite, which includes MaterialX documents that leverage transformmatrix:
If there's already an appropriate example there, feel free to load and test it here; if not, then you can add another example to the same document in the test suite.
This change removes the "OSL TransformMatrix Point Semantics" unit test from this PR, narrowing it to the one-line library fix for `IM_transformmatrix_vector3M4_genosl`. Both assertions in the test were string matches, comparing the `sourcecode` attribute against the literal text of the fix and searching the generated shader for the `point(` substring, so neither could detect a regression in the translation behavior itself. The `sourcecode` comparison would also break if this implementation were later moved into an OSL source file, as is already done for the `vector2M3` variant, even though its behavior would be unchanged. I think the fix is well justified without a unit test, as it aligns the OSL implementation with the existing GLSL, MDL, and OSL `vector2M3` implementations, each of which appends a homogeneous coordinate of one before applying the matrix. Coverage across all shading language targets will be better provided by our render test suite, and going forward it would be valuable to add an updated example to `transform.mtlx` to handle this new case.
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
Thanks for the fix, @aviralgarg05!
6236d02
into
AcademySoftwareFoundation:main
2 participants
Title
Fix OSL
transformmatrixtranslation forvector3+matrix44. #2833Description
Summary
This fixes an OSL backend bug in
transformmatrixfor thevector3+matrix44variant.In OSL,
ND_transformmatrix_vector3M4was using:That treats the input as a direction-like vector, so matrix translation is ignored.
The fix changes the OSL implementation to:
which preserves translation and brings the behavior in line with the existing GLSL and MDL implementations, as well as the OSL
vector2M3path.What changed
IM_transformmatrix_vector3M4_genoslpoint({{in}})Why this is needed
The issue was that a
matrix44transform with translation would not move avector3input in generated OSL, even though the same MaterialX node behaves correctly in other backends.That made OSL inconsistent and caused translation to be silently dropped.
Files changed
libraries/stdlib/genosl/stdlib_genosl_impl.mtlxsource/MaterialXTest/MaterialXGenOsl/GenOsl.cppValidation
Ran the new regression test:
build/bin/MaterialXTest "GenShader: OSL TransformMatrix Point Semantics"Ran targeted OSL checks:
Ran the full locally configured test suite:
Result:
79/79tests passedgit diff --checkpassedNotes
This change is intentionally narrow and only touches the OSL implementation path that was dropping translation, plus the regression coverage needed to keep it from coming back.