fix: Bugs in vector2.h, vector4.h, color2.h, color4.h, docs #1892
Conversation
Fix a variety of typos in the 2- and 4-component color and vector helper structs. Very embarrassing! Also embarrassing, a typo in the docs had said that the way to overload the `!=` operator was with a function called `__operator__ne__` while actually, all along it had been `__operator__neq__`. Comprehensive test of color2, color4, vector2, vector4. New testing header: testsuite/common/shaders/osl-unittest.h This has utilities to make it easier to make unit tests for shader functions in our testsuite. Various other minor fixes to these headers. Signed-off-by: Larry Gritz <[email protected]>
|
This looks like a great improvement, thanks @lgritz, though it seems worthwhile to consider whether the The https://materialx.org/assets/MaterialX.v1.38.Changelist.pdf
I'm not aware of any teams that are still building projects with pre-2021 versions of MaterialX, and legacy MaterialX documents are automatically upgraded in modern versions, so we may want to consider whether the |
|
Yes, I had to do it anyway because (a) I wanted to backport the fixes to the 1.12 branch, where we can't remove functionality within a supported release family, and also (b) because of the high symmetry between color2 and vector2, it wouldn't really have saved me any work to fix only one. I think we should integrate this now, for the reasons stated above, but then remove the color2 from main before we beta 1.13. I feel like my procrastination on the types I didn't like has been 25% successful so far. I guess I can't convince you of the error of color4, if not vector2/4? :-) But seriously, I feel like OSL has to support these types to properly live in an ecosystem where MaterialX is important (which it is), but while my mild annoyance at vector2 and vector4 being built-in types is merely that they are unnecessary, I fear we are all going to regret color4 if spectral rendering ever catches on more widely. |
|
Got it, and that approach makes sense to me. On the value of the That would be challenging without the ability to bind Although we'll learn more about the implications of spectral rendering as it becomes more popular in open-source renderers, it's my early sense that we won't associate spectral color values with alpha channels at all, since there's no history of combining these two concepts in rendering applications. That would allow us to have a future It's worthwhile to consider how this would be integrated in OSL, and whether |
|
Ha, that's where I was going with my thinking as well. I agree that the way it will eventually shake out is that we'll need to introduce a |
|
|
||
| // Check that two expressions are equal. For non-built-in types, this | ||
| // requires that a tostring() function is defined for that type. | ||
| #define OSL_CHECK_EQUAL(x,y) \ |
There was a problem hiding this comment.
@lgritz do we need some fuzzy comparison here, allowing for some amount of epsilon or other floating point differences? Exact compare sounds like a setup for issues with different hardware/compilers especially with trigonometric functions
There was a problem hiding this comment.
I think when we run across that problem, we'll add a separate fuzzy one.
|
Anybody have remaining concerns about this PR? |
|
LGTM! |
…oftwareFoundation#1892) Fix a variety of typos in the 2- and 4-component color and vector helper structs. Very embarrassing! Also embarrassing, a typo in the docs had said that the way to overload the `!=` operator was with a function called `__operator__ne__` while actually, all along it had been `__operator__neq__`. Comprehensive test of color2, color4, vector2, vector4. New testing header: testsuite/common/shaders/osl-unittest.h This has utilities to make it easier to make unit tests for shader functions in our testsuite. Various other minor fixes to these headers. Signed-off-by: Larry Gritz <[email protected]>
Fix a variety of typos in the 2- and 4-component color and vector helper structs. Very embarrassing!
Also embarrassing, a typo in the docs had said that the way to overload the
!=operator was with a function called__operator__ne__while actually, all along it had been__operator__neq__.Comprehensive test of color2, color4, vector2, vector4.
New testing header: testsuite/common/shaders/osl-unittest.h This has utilities to make it easier to make unit tests for shader functions in our testsuite.
Various other minor fixes to these headers.
Fixes #1889