Re: Review: YAML serialization
Jeremy Selan <jeremy...@...>
Only addressing the GroupTransform issue...toggle quoted messageShow quoted text
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.)
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?
* You mention in the code, "GroupTransform seems to be a mixedWith the current structure it would be possible to have GroupTransform()s containing GroupTransform()s and possibly more GroupTransform()s and so on..