MaterialX
MaterialX copied to clipboard
AcademySoftwareFoundation
Incorrect implementation of Convert_Vector2_Vector3
Hello, while using 'convert_vector2_vector3' node in MaterialX Graph Editor and QuiltiX I seem to have stumbled on a real issue where the glsl and OSL code is incorrect.
The specifiction says:
vector2 to vector3, or vector3 to vector4: copy incoming channels and append an additional channel with value 1.0 (e.g. convert from non-homogeneous to homogeneous vector)
And yet the glsl implemenation seem touse a 0.0 value insted of 1.0:
The same can be seen for OSL using QuiltiX and Arnold:
Good catch, @HardCoreCodin, and here is the line of code that ought to be modified to address this in MaterialX shader generation:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenShader/Nodes/ConvertNode.cpp#L31
Good catch, @HardCoreCodin, and here is the line of code that ought to be modified to address this in MaterialX shader generation:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenShader/Nodes/ConvertNode.cpp#L31
Thanks @jstone-lucasfilm , will have a look at building with the fix. Could you also have a look at my other recent issue? If affects a few nodes.
I believe this issue will be addressed by upcoming PR #1905, though the resolution in this PR actually updates the specification to match the current behavior of the MaterialX nodes.
@jstone-lucasfilm is correct #1905 is not going to change any render behavior - but instead conform the specification to the code, and also add some additional convert nodes. If we think its desirable to follow the convention of the original specification we should discuss that, and I can amend the PR - it would be a trivial change.
@ld-kerley In this case, I think it's better to follow your current approach, where the codebase behavior is considered canonical, and the specification is updated to match. Both interpretations seem logical, and we just need to align our specification and codebase to a single interpretation.
Thanks for considering this issue, though - I've had a quick look at the new documentation changes, and they still don't really align with what the code was doing. I can't find where the new code is as the link here is now broken. I think because it changed into being a sub-graph? At any rate, I'm not sure what the rational would be for the new convention of always setting just the 4th component to 1, even when converting from vector2 to vector3 (where does that 1 even supposed to go?). If the idea of making homogenous coordinates was dropped for the 2D-3D case, why was it kept for the 3D-4D case? That feels inconsistent. In my opinion, if the fix is to change the documentation instead of the code, then it should always be zero-padded all the way for when vectors go up a dimension (like the original code was doing). The only place it makes sense to pad with a 1 is when going from a color3 to a color4, making the default alpha value opaque. Also, if reaching consensus about what should happen is challenging, then how about exposing what the appended component value should be as an optional input to the convert nodes that go up a dimension? At least the contention would be kept to "what the default should be" and users would still be able to change it if they like, as opposed to having to resort to using a combine node instead.
@HardCoreCodin Here's the code that handled these conversions in the previous release of MaterialX, if you'd like to compare @ld-kerley's latest implementation against the historical behavior of the codebase:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/v1.38.10/source/MaterialXGenShader/Nodes/ConvertNode.cpp#L34
As far as I can tell, Lee's current code matches the historical behavior, but please let us know if we're missing a subtle edge case.