Re: Review: YAML serialization


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

Hi,

Comments below.

Only addressing the GroupTransform issue...

I am in favor of keeping GroupTransform as is. Having a group of
transforms (at the API level) be interchangeable with a single
transform is a very powerful concept. The allows the API to be
pretty flexible, as class methods can seamlessly accept either a
single transform, or a group of transforms, using the same interface.

A good example of this is config.applyTransform(Transform *), which
can take either a single transform or a group of transforms. If we
were to create a separate object - TransformVec - we would need to
have 2 overloaded calls to expose the same functionality.

Another example of this is DisplayTransform.setDisplayCC(Transform *),
which controls the 'post display transform' color correction.
(Think the gamma slider in the nuke monitor).

In our internal compositing package, we actually let the user adjust
the white point, black point, and gamma. This is internally handled by
creating a group transform (at runtime), adding the appropriate
children, and then calling setDisplayCC. There is public code
analogous to this in nuke/Display/Display.cpp, Display::_validate.

In fact, I'd probably want to even go a step further and have the
ColorSpace.setTransform call take a Transform * rather than a
GroupTransform *. (If you want it to be a group, great! Otherwise,
why force it?) There's no reason to force a grouptransform anywhere
in the public interface, as far as I can tell.)
+1 this would clean the whole thing up for me.

The potentially nested nature of groupTransforms / subgroups does not
bother me. If a group - containing a group - happens to be the
natural way to express a particular transform to the user, why not allow it?
Yeah thats sounds cool, ColorSpace.setTransform(Transform *) clearly makes a GroupTransform just another Transform which I like a lot more than it being special in the ColorSpace() context.

.malcolm


-- Jeremy

* 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()

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