Patch: Many composite/blend modes mostly compatible to Adobe Photoshop (c)

[Cyrille Berger Skott]

On Sunday 06 February 2011, Silvio Heinrich wrote:
> On 02/06/2011 09:46 AM, Cyrille Berger Skott wrote:
> > Hi,
> > 
> > Here is some code review:
> > 
> > * I did some more testing and it is indeed a regression for this bug
> > https://bugs.kde.org/show_bug.cgi?id=176536 , since I am assuming that
> > you wanted it that way, having transparent pixel being erased.
> > 
> > For instance look at the result of the following file:
> > 
> > http://cyrille.diwi.org/tmp/krita/colored_leaves.kra
> > Before patch:
> > http://cyrille.diwi.org/tmp/krita/colored_leaves_before.png
> > After patch:
> > http://cyrille.diwi.org/tmp/krita/colored_leaves_after.png
> > 
> > Since I am not convinced that "alpha lock" should be used for that, since
> > it is there to prevent editing of the alpha channel, if the having
> > transparent pixel erased is a wanted behaviour we should probably
> > introduce a "keep transparency when compositing" and have it on by
> > default
> 
> Oh, yes you are right. This is a problem for layer compositing.
> I don't know how easily it can be done but I would like to have a
> button, to keep
> transparency when compositing layers, for every layer next to the alpha
> lock button
> and of course turned on by default if you want it that way.
> I have the rough guess that the layer compositing simply works with the
> CompositeOps alpha
> locking turned off by default. So turning it on by default and adding an
> option to turn
> it off (per layer) should hopefully solve the problem :D if you agree.
Yes, sounds good.

> > * the optimized integers operations do provide a significant boost in
> > speed, about 10% on the composite operations, that translate to about 5%
> > when drawing. I don't know at what size of brush you tried, but even on
> > high-end hardware it makes a noticeable difference when using large size
> > brushes. On the other hand, it does provide an unnoticeable difference
> > in the results. This is why those optimizwd integers operations are used
> > in Gimp, imagemagick and many other graphics applications. So I am
> > against dropping them. If it is really a problem for you, I would
> > suggest the use of a compile switch,
> > ENABLE_EXACT_PRECISION_COMPOSITE_OPS , defaulted to off.
> 
> Ok, my biggest brush only has a diameter of 300px.
> I didn't meant to drop those optimized functions. I just had some
> problems with the
> result and wanted to get everything working right before i start
> optimizing anything :D
Fair enough.
 
> > * KoCompositeOpFunction.h should not duplicate the content of
> > KoColorSpaceMath.h (especially lerp, mul) and the functions that does not
> > exist in KoColorSpaceMath should be added (like inv) Similary use "clamp"
> > from KoColorSpaceMath, since your version clamp values for floating
> > points as well, we should not happen.
> 
> Would you agree if I add those functions as aliases to KoColorSpaceMath.h
> because i think in my opinion it makes the code more readable to have a
> simple function mul(..., ...) then to specify every time
> KoColorSpaceMaths<...>::multiply(..., ...).

The problem is that those are headers, that might even become public at some 
point, and "mul" is extremely generic. So it should either be put in a 
namespace/class, or #defined / #undefined.

> And I would like to have two versions of the multiplication functions
> (optimized and unoptimized) like:
> 
> // fast version
> template<class T>
> inline T mul(T a, T b) { T(KoColorSpaceMaths<T>::multiply(a, b)); }
> 
> // accurate version
> template<class T>
> inline T mulac(T a, T b) {
>      typedef typename KoColorSpaceMathsTraits<T>::compositetype
> composite_type;
>      return T(composite_type(a) * b /
> KoColorSpaceMathsTraits<T>::unitValue);
> }
> 
> So i can add the fast version everywhere where it works and the accurate
> version everywhere else.

I would like to know about the case where it does not work. Since in 95% of 
the case, the fast version gives the correct result, and in the remaining 5% 
it has an error of +1 or -1.

> About the clamp function. I thought even the FP versions need to be
> clamped to the range
> between 0.0 and 1.0 (I used the camp function only where the values
> could exceed this bound e.g. after divisions)
> because without clamping the FP versions would behave different to the
> integer versions or do
> I understand something wrong here?
Yes. The point of using floating point is to allow values above 1.0. It used 
mainly for High dynamic range, see 
http://en.wikipedia.org/wiki/High_dynamic_range_imaging . Usually they use a 
linear light model. And a value above 1.0 only means a lot of light. Since 
most display have a low dynamic range (and I am even unsure that there are 
drivers support for HDR on linux), on display, the data is clamped to a given 
range, that depends on the exposure selected by the user.

> Ah, yes something else. I've got a question about the composite type for
> float data types.
> The KoColorSpaceMathsTraits<float>::compositetype is double. is this
> necessary?
> wouldn't float be sufficient here?
The compositetype can be used to sum a lot of values together, this is why we 
always use a much higher bit size.

> And the KoColorSpaceMaths::divide method takes values of type T and
> returns a value of type T.
> But i think it should return a value of type T->compositetype. For
> example dividing 1.0 by 0.5
> will result in 2.0 what would be 510 in 8bit per channel mode and we
> have a data loss here because it will be
> truncated to T.
Right, then lets use T->compositetype.
 
> > * have you cheked if compositeFunc is inlined inside
> > KoCompositeOpGenericSC ?
> 
> Well, yes I'm pretty sure. I compiled a little test program:
Good, I just wanted to be sure :)

-- 
Cyrille Berger Skott