Re: Review: YAML serialization


Malcolm Humphreys <malcolmh...@...>
 

Hi,

OCIOYaml.cpp
* There was a missing #include <cstring>
* For the Enums, (line 512+) this could could be greatly simplified by
relying on the helpers in OpenColorTypes.h.
I think you just fixed this, but yeah there was a bit of code dup.

* You mention in the code, "GroupTransform seems to be a mixed
concept". And, you dont allow for it to be serialized. Could you
elaborate your thinking behind this?
With the current structure it would be possible to have GroupTransform()s containing GroupTransform()s and possibly more GroupTransform()s and so on..

On a colorspace you can set ColorSpace::setTransform(GroupTransform, dir), to me this seems ok and I do serialize it in this case, but if I support the GroupTransform where I have support for all the other Transform()s then you could build multiple layer deep GroupTransform()s which doesn't seem necessary.

I would prefer to create a new type TransformList eg. ColorSpace::setTransform(TransformList, dir) which would replace GroupTransform and it would different than the base Transform()


* The ExternalProject_Add stuff is slick, but appears to required
cmake 2.8. We're currently only requiring 2.6. Is this update
something we'd like to do?
Did you try and copy the ExternalProject.cmake into share/cmake to see if that works. I have attached it, can you check to see if it imports ok as I'm not running 2.6.

I have been using 2.8 for about 6 months with no issues, so I would say it's safe to update esp if it helps to reduce the code base.

http://www.kitware.com/products/html/BuildingExternalProjectsWithCMake2.8.html

* On the Imageworks linux system, there is a build error related to libyaml-cpp:
I think I was getting a similar error when putting yaml-cpp into the OpenColorIO namespace, I was leaving this to fix later. Did you find a fix for this?

/usr/bin/ld: ../../ext/dist/lib/libyaml-cpp.a(node.cpp.o): relocation
R_X86_64_32S against `std::basic_string<char, std::char_traits<char>,
std::allocator<char> >::_Rep::_S_empty_rep_storage' can not be used
when making a shared object; recompile with -fPIC

I've addressed most of these in my local git repo in a low tech
manner, by manually copying yaml into src/core/yaml.
I've also patched yaml to put it into the OCIO namespace.

see http://github.com/jeremyselan/OpenColorIO/tree/yaml

While this approach isn't clean as your .tgz "ExternalProject_Add"
solution, it's potentially less prone to build errors, and addresses
all namespace issues. And, seeing as I imagine we wont be needing to
update yaml versions on a regular basis, the maintenance implications
aren't particularly troublesome either.
Umm I would prefer if this was a patch for the reasons you have stated that it isn't going to change all that much so having it all checked in seems like a waste, while also making it harder to absorb any changes when yaml-cpp changes. The dev on yaml-cpp is pretty active (http://code.google.com/p/yaml-cpp/updates/list)

If we were choosing to fork yaml-cpp (which I don't think we are) or other ext dependants, checking them in would be fine and would help to indicate a fork.

I was also hoping to look at using http://gcc.gnu.org/wiki/Visibility as well to help reduce symbol clashes, have you had any luck/experience with this?

Of course, I'm open to your approach of ExternalProject_Add assuming
we actively agree to require cmake 2.8 (and address the build
problems.)
+1

Let me know if the ExternalProject.cmake works in 2.6, I'm happy to clean up the other externals the same way while I'm still motivated to do so. :)

.malcolm



-- Jeremy

On Sun, Oct 17, 2010 at 5:09 PM, Malcolm Humphreys
<malcolmh...@mac.com> wrote:
- Initial checkin of YAML serialization
- Added ext/yaml-cpp-0.2.5.tar.gz with a patch to build it static
- Added yaml-cpp to the LICENCE file
- CDLTransform should be the only thing depending on tinyxml
- PyOCIO_Config_getXML now returns YAML not XML, this should be fixed soon. Possibly this python method should be removed and we use the config.__str__() instead, this would make it slightly more pythonic

http://github.com/malcolmhumphreys/OpenColorIO/commit/ef6cddf742b961e89061ddd9a68f968eb0d4e5fd

.malcolm

Join ocio-dev@lists.aswf.io to automatically receive all group messages.