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