Re: Review: YAML serialization


Jeremy Selan <jeremy...@...>
 

Nice work.

A few comments:

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.
* 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?

* 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?
* On the Imageworks linux system, there is a build error related to libyaml-cpp:

/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.

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.)

-- 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.