[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-kimageshop
Subject:    Re: Patch: Many composite/blend modes mostly compatible to Adobe
From:       Cyrille Berger Skott <cberger () cberger ! net>
Date:       2011-02-06 8:46:31
Message-ID: 201102060946.32245.cberger () cberger ! net
[Download RAW message or body]

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

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

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

* have you cheked if compositeFunc is inlined inside KoCompositeOpGenericSC ?

-- 
Cyrille Berger Skott
_______________________________________________
kimageshop mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop
[prev in list] [next in list] [prev in thread] [next in thread]