From kde-kimageshop Sun Feb 06 08:46:31 2011 From: Cyrille Berger Skott Date: Sun, 06 Feb 2011 08:46:31 +0000 To: kde-kimageshop Subject: Re: Patch: Many composite/blend modes mostly compatible to Adobe Message-Id: <201102060946.32245.cberger () cberger ! net> X-MARC-Message: https://marc.info/?l=kde-kimageshop&m=129698198913281 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