Use Imath bindings from PyIlmBase #672
Conversation
This is consistent with all the Algo namespaces in Gaffer, and is what we intend to do for all Algo.h files in Cortex too.
Note that these duplicate functionality in BoxOps.h. Our intention is to remove BoxTraits/BoxOps in the future. This commit also adds python bindings and tests for BoxAlgo.
Also add bindings and unit tests.
We don't have any need for them, and they would be unusable with the imath module because it doesn't provide bindings for Color3d/Color4d.
This is used to locate python modules for use when running the tests.
At the time these were added to Cortex, there were no official bindings for Imath. But now there are well established standard bindings, and the IECore module is imcompatible with them because it binds the same classes. We will use the standard bindings from now on. I've kept the overloads for IECorePython::str and IECorePython::repr for now (moving them into IECore.cpp), as they are used by the Data bindings.
This was broken due to the imath bindings omitting `dimensions()` methods on some types. It was also of dubious utility andsemantics, and I can't find a single reference to it being used anywhere.
This highlights the differences between the IECore and Imath bindings. Non-standard bindings added by Cortex ===================================== These no longer exist. - Box.contains - Box.split - Box.transform (can use Box * Matrix instead) - M44f.create* (can use M44f().translate() etc instead) - Rand32/Rand48 utility methods (can use RandomAlgo instead for some of these) Standard bindings that are missing in Imath =========================================== - Euler : - angleMapping - simpleXYZRotation - legal - nearestRotation - Order.default - Constructor taking layout argument - M44 : - Constructor taking M33 and Vec3 - dimensions - Quat indexing - ImathRoots Bindings that are broken in Imath ================================= - +=, /= etc do not operate in place, but instead create a new object. - Properties are masked by non-standard accessor methods, so for example `box.min = box.max` is now `box.setMin( box.max() )`. Examining the bindings shows that these properties are bound correctly with `def_readwrite`, but the correct bindings are then immediately masked by the accessor methods. This applies to the following : - Box.min, Box.max - Plane.normal, Plane.distance - Quat.v, Quat.r - Quat.identity is not bound as a static method, making it unusable Differences =========== - `str()` and `repr()` now don't include the module prefix. - `Euler.angleOrder()` now returns `V3i` not tuple. - `Matrix.inverse()` is not giving identical results. - `slerp()` is now a Quat member function, not a free function. - Matrix constructors don't take a list
This isn't quick enough to be viable any more - these days we would use OSLObject instead.
We use TypedDataFromType to register converters from T to TypedData<T>, and the order of registration affects the order in which boost::python tries the converters. The Cortex bindings for Imath::Color3 didn't declare Imath::V3f as a base class, but the PyImath bindings do. That means we need to register the color converters first, so that Color3f is converted to Color3fData by preference, and not V3fData. This fixes a test failure in test/IECoreGL/ShadingTest.py, whereby testCsParameterTrumpsColorAttribute was declaring a "color" attribute using "Color3f", but it was appearing as V3fData on the C++ side.
Also add missing license and `unittest.main()`
This provides the imath python module that we now require.
This works around the following error : ``` > eval( repr( imath.Box2f() ) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<string>", line 1, in <module> OverflowError: bad numeric conversion: positive overflow ``` The problem occurred with both the Cortex and imath forms of the repr serialisation, and seems to be caused by imperfect round tripping for the FLT_MAX and FLT_MIN values used by an empty box. IECore uses `lexical_cast<string>( floatValue )` for this, and imath uses `stringstream << floatValue`, but both result in the exact same serialisation of "3.40282347e+38". This does provide accurate round tripping when parsed again as a float as follows : ``` const float f = FLT_MAX; const string s = lexical_cast<string>( f ); const float f2 = lexical_cast<float>( s ); assert( f == f2 ); // OK! ``` But that's not what happens when we parse in Python, because python doesn't use floats - everything is a double instead. That puts us on a code path somewhat equivalent to this : ``` const float f = FLT_MAX; const string s = lexical_cast<string>( f ); const double d = lexical_cast<double>( s ); // Equivalent to Python's parsing const float f3 = numeric_cast<float>( d ); // Throws, because we're now a _tiny_ bit bigger than FLT_MAX ``` This problem didn't occur in the IECore bindings because we did a straight cast rather than using `numeric_cast`, and also doesn't occur in the imath bindings for Box3f (rather than Box2f) because Box3f doesn't use `numeric_cast` for some reason. It would be nice to fix the imath bindings to be consistent, but the chances of getting anything merged into the OpenEXR repo are almost nil at this point - PRs are just accumulating unanswered. Since the solution here also results in more readable serialisations, I think it's a decent enough work around.
| @@ -249,6 +249,11 @@ o.Add( | |||
| "/usr/lib", | |||
| ) | |||
|
|
|||
| o.Add( | |||
| "PYTHONPATH", | |||
There was a problem hiding this comment.
A bit funny that it's named generically but only used for tests. Don't we add TEST_ to other variables like that. Are you thinking of using it outside the tests as well?
There was a problem hiding this comment.
I wasn't sure which way to go, but it seemed plausible that we'd use it for other things, and I thought having a matching set of CPPPATH, LIBPATH and PYTHONPATH made sense. Not particularly attached to it though if you feel strongly about it.
|
I've just been fixing up some USD tests to use the imath bindings as required by this PR and noticed the following which got me chasing my own tail for a bit. results in Whereas reordering the imports works as expected: results in Thought I should mention it here incase someone else encounters this issue. |
+ left the color3d / color4d functions commented out as I think it's useful to document they're not supported. + added a dodgy import into the test entry file All.py as we have to import IECore *before* imath to avoid type errors as mentioned in ImageEngine#672
|
Thanks for bringing that up Don, I should have mentioned it myself. The issue is that The boost docs seem to suggest that the BOOST_PYTHON_TYPE_ID_NAME macro can be defined to work around this, but I think we'd need to use that in both the imath build and the cortex one. Our solution is to use RTLD_GLOBAL so that the symbols are shared between the modules. This is done automatically in IECore/init.py because we need it for sharing types between say IECore and IECoreScene. So, if you import IECore before imath, you also get this fix applied to the imath problem. In other words, you don't have to import IECore first, but you do have to make sure RTLD_GLOBAL is in use at the point you import imath. I couldn't think of a clean way of making this "just work" though... |
+ left the color3d / color4d functions commented out as I think it's useful to document they're not supported. + added a dodgy import into the test entry file All.py as we have to import IECore *before* imath to avoid type errors as mentioned in ImageEngine#672
At the inception of the Cortex project, there were no publicly available Python bindings for the Imath library, so we rolled our own. Since then the OpenEXR project has added PyIlmBase, which provides an
imathpython module, but importing both IECore and imath at the same time results in clashes, making it impossible to use IECore in codebases with existing dependencies on the imath module.This PR resolves the situation by removing the Imath bindings from IECore, and updating all python code to use the imath module instead. This is in keeping with our general aim of slimming Cortex down by removing non-core functionality that has since been superceded by other projects.
Unfortunately the imath module is not a direct drop-in replacement for our original bindings so updating the code has been somewhat laborious, and on the whole I don't think the new bindings are an improvement (aside from the obvious interoperability benefits and some nice features like numpy support). Hopefully at some point we can contribute some improvements back to the OpenEXR project, but currently it doesn't seem to be responding to pull requests. Anyhow, here's a summary of the main differences :
Non-standard bindings added by Cortex
Standard bindings that are missing in Imath
len( matrix )andlen( matrix[0] )insteadBindings that are broken in Imath
box.min = box.maxis nowbox.setMin( box.max() ). Examining the bindingsshows that these properties are bound correctly with
def_readwrite, butthe correct bindings are then immediately masked by the accessor methods.
This applies to the following :
Differences
str()andrepr()now don't include the module prefix (useIECore.repr()if you need this).Euler.angleOrder()now returnsV3inot tuple.Matrix.inverse()is not giving identical results.slerp()is now a Quat member function, not a free function.This is a mammoth PR to review. To aid the process I've put the vast volume of fairly mechanical changes into various "Use Imath" commits - I don't expect anyone to review these line by line, but dipping in to get a flavour of the changes and their scale should be useful. Anything that I consider worth closer scrutiny or discussion is in its own commit.
I've also updated the 9to10 module to do some basic automatic IECore->imath conversion, but this only deals with getting the module right, not any of the other changes. It should go a long way towards getting the internal IE codebase updated though.
I'll follow up with the equivalent PR for Gaffer shortly.