Added MATRIX33 aggregate to TypeDesc. Used in OpenEXR input/output#1265
Added MATRIX33 aggregate to TypeDesc. Used in OpenEXR input/output#1265Shootfast wants to merge 3 commits intoAcademySoftwareFoundation:masterfrom Shootfast:openexr_3x3_matrix
Conversation
There was a problem hiding this comment.
I'm certain that this change is going to break OSL.
There was a problem hiding this comment.
Yeah, I left the fromstring method so that "matrix" still converts to matrix44, but felt that it was better to be specific for the c_str method. There was also the special case for "float4" which, although I left alone, I was curious as to why we have the special cases at all (though I assume it's also an OSL thing).
There was a problem hiding this comment.
OIIO is a dependency for OSL, not the other way around, so a few classes that are needed for both live in OIIO but may have some particular design quirks that came directly from imagining how it would be used in a shading system or renderer.
For c_str(), I would like to make a pitch for keeping "matrix" to mean the 'usual' case of 4x4 transformation matrices used in computer graphics, and only need the numbers for other cases (3x3 is the only other one we have planned at the moment). I think that's similar in spirit to how "vector" is understood to be the usual case of vector3, but we use "vector2" or "vector4"
There was a problem hiding this comment.
You know, now that I'm looking at TypeDesc::from_string versus TypeDesc::c_str(), I'm seeing some inconsistencies. Let me take a closer look at this issue and see what can be changed without breaking much in OSL. Give me a little time and I'll report back here. We may be ripe for some cleanup, so I'm taking a hard line on anything quite yet.
|
This all LGTM except for my reservations about apps that assume that TypeMatrix.c_str() returns "matrix". I need to see how much needs to be fixed on the OSL side, and consider if anybody else would be affected. |
|
See #1267 for my alternate proposal, which is mostly @Shootfast's work, with just a couple minor additions and reverting the "matrix" vs "matrix44" behavior change. |
|
Mark, do you have opinions on #1267? Is that ok with you? |
|
Subsumed by #1267 |
* // WIP convert header docs to doxygen style Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Adjust reference syntax From ":cpp:func:`Foo::bar`" to "\ref Foo::bar". This seems to pacify some build warnings and clean up the C++ docs a little, but it doesn't seem to improve issue AcademySoftwareFoundation#1265 Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Expose documentation for FileRules, Config, MatrixTransform::setOffset Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Expose more documentation: Looks, Context Variables Minor cleanup and adjustments Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Expose GpuShaderCreator usage example Promote usage example up to the class-wide documentation Change numbered list to simple bullets -- there seems to be a problem with orderedlists...? Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Missed a change Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Expose `GpuShaderDesc` fragment shader example code Not sure why the topmost "border" forward slashes are not rendering... Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Tidy up conversion to doxygen style Specify language for code demonstrations Improve readability of header comments Clean up "TODO" comments re: moving to rst Convert remaining comments to RST syntax Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Rebuild frozen docs Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> * Adjust line wrapping in one particularly egregious instance Signed-off-by: Zach Lewis <zachcanbereached@gmail.com> Co-authored-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>
2 participants
Added a new
MATRIX33toTypeDesc, and made the existingTypeMatrixan alias forTypeMatrix44.Also changed the c_str representation of matrix to always return the explicit
matrix33ormatrix44, though left the fromstring behaviour the same.