Flow Update
ClosedPublic

Authored by wbrown on Wed, Jan 23, 1:00 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Maniphest Tasks
T8576: Improve Brush Opacity handling.
Commits
R37:d4a0c1f9fb52: Flow Update
Summary

Update Alpha Darken Composite Operation to use flow more reasonably

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
wbrown created this revision.Wed, Jan 23, 1:00 PM
Restricted Application added a project: Krita. · View Herald TranscriptWed, Jan 23, 1:00 PM

Okay, this builds properly for me, it'll require a look over by @dkazakov to check if the maths is ok.

@Deevad, @kamathraghavendra, @ramonmiranda will need to test if it is desirable behaviour.

Pro: it would make Krita's behaviour much more consistent with other applications.
Con: We'll need to adjust the presets that depended on the old behaviour.

libs/pigment/compositeops/KoCompositeOpAlphaDarken.h
106

please don't add newlines :)

Note: deevad and raghukamath have answered in T8576

Hey William Brown, I am going to push this patch temporarily so artists can test, could you provide an email address so I can credit you properly?

Okay, I've temporarily pushed it in https://phabricator.kde.org/R37:e3c2f5bed0c83462f893919098dad16d37263698 , we'll revert it on Monday 28th.

sure thing, my email's will.brown039@gmail.com

I need to check the maths of it

rempt added a subscriber: rempt.Mon, Jan 28, 1:06 PM

Here are the first survey results: https://www.dropbox.com/s/8ispua7oo6er1z4/survey_2019-1.pdf?dl=0 -- people are _very_ happy :-)

dkazakov requested changes to this revision.Thu, Jan 31, 8:12 PM

Hi, all!

I have checked the maths of the new algorithm. It looks logical and should work fine. I have also added a few comments to the code to make it easier to decrypt later. See patch snippet below.

The only problems I see with the patch are 1) the transition process for existing presets and 2) the old effect is not available in the new version. I have a feeling that we have people who like the old effect, so it would be better to keep the old version as well.

I guess it is not very difficult to make both composite ops available. We just need to do the following:

  1. Implement a new composite op, say, "alpha-darken-creamy".
  2. Modify KisPaintOpSettings::indirectPaintingCompositeOp() to fetch the selected composite op from the preset itself
  3. Add a switch to Flow section to select between "sharp" and "creamy" variants of flow and save it into the preset.
  4. Modify all our default presets to use the new "creamy" version of flow :)

If needed, I can help with points 1 and 2.

1diff --git a/libs/pigment/compositeops/KoCompositeOpAlphaDarken.h b/libs/pigment/compositeops/KoCompositeOpAlphaDarken.h
2index e51e1a6..c05668e 100644
3--- a/libs/pigment/compositeops/KoCompositeOpAlphaDarken.h
4+++ b/libs/pigment/compositeops/KoCompositeOpAlphaDarken.h
5@@ -91,7 +91,24 @@ public:
6 channels_type fullFlowAlpha;
7 channels_type averageOpacity = scale<channels_type>(*params.lastOpacity);
8
9+ /**
10+ * Here we calculate fullFlowAlpha, which shuold strive either to
11+ * averageOpacity or opacity (whichever is the greater) or just keep old dstAlpha
12+ * value, if both opacity values are not bit enough
13+ */
14 if (averageOpacity > opacity) {
15+ /**
16+ * This crypty code is basically an optimized version of the folowing:
17+ * fullFlowAlpha = averageOpacity *
18+ * unionShapeOpacity(srcAlpha / averageOpacity,
19+ * dstAlpha / averageOpacity);
20+ *
21+ * The main idea is: fullFlowAlpha should be as near to averageOpacity as
22+ * maximum of srcAlpha and dstAlpha and a bit more. So that in consequent
23+ * applications of the blending operation alpha channel would aim to
24+ * averageOpacity.
25+ */
26+
27 channels_type reverseBlend = KoColorSpaceMaths<channels_type>::divide(dstAlpha, averageOpacity);
28 fullFlowAlpha = averageOpacity > dstAlpha ? lerp(srcAlpha, averageOpacity, reverseBlend) : dstAlpha;
29 } else {
30@@ -103,8 +120,6 @@ public:
31 } else {
32 channels_type zeroFlowAlpha = dstAlpha;
33 dstAlpha = lerp(zeroFlowAlpha, fullFlowAlpha, flow);
34-
35-
36 }
37
38 dst[alpha_pos] = dstAlpha;

This revision now requires changes to proceed.Thu, Jan 31, 8:12 PM
rempt added a comment.Thu, Jan 31, 8:16 PM
The only problems I see with the patch are 1) the transition process for

existing presets and

That's what Deevad was worried about, too, but it turned out not to be a
problem.

  1. the old effect is not available in the new version. I have a feeling that we have people who like the old effect, so it would be better to keep the old version as well.

That is not a problem either. As Deevad has shown in his video, the old effect
is still achievable.

Note that only one person in the survey, who had not seen Deevad's video,
asked for the old behaviour to be retained as an option; everyone else wanted
the new effect, and couldn't care less about the old effect.

That is not a problem either. As Deevad has shown in his video, the old effect
is still achievable.

Okay, now I agree with the point that the old effect is achievable. With breaking old presets, I'm not sure. Perhaps, we could make some compatibility mode, that is, if you activate some global config option (which is off by default), the presets will behave as before? And the option will be removed in, e.g. Krita 5?

Implementing one more composite op and doing a switch based on KConfigGroup is not a big task (we already do that for AVX optimizations). And this way people, who rely on their custom presets in production work will not suffer much :)

rempt added a comment.Thu, Jan 31, 8:47 PM

< Implementing one more composite op and doing a switch based on KConfigGroup is not a big task (we already do that for AVX optimizations). And this way people, who rely on their custom presets in production work will not suffer much :)

I'm sure it could be done -- but I don't think there's a huge need. I'd like this patch to be pushed asap, so our git master users have the new behaviour.

This revision was not accepted when it landed; it landed in state Needs Revision.Fri, Feb 1, 10:35 AM
Closed by commit R37:d4a0c1f9fb52: Flow Update (authored by wbrown, committed by dkazakov). · Explain Why
This revision was automatically updated to reflect the committed changes.