Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spectrum mixing speed up by direct mixing colours that are close enough #182

Open
ChengduLittleA opened this issue Sep 14, 2021 · 7 comments

Comments

@ChengduLittleA
Copy link
Contributor

ChengduLittleA commented Sep 14, 2021

Here I have a patch:

diff --git a/brushmodes.c b/brushmodes.c
index f0c68b6..eede454 100644
--- a/brushmodes.c
+++ b/brushmodes.c
@@ -70,6 +70,12 @@ void draw_dab_pixels_BlendMode_Normal (uint16_t * mask,
   }
 };
 
+inline int color_direct_mixable(uint16_t r, uint16_t g, uint16_t b, uint16_t* rgba){
+  return ((rgba[0]==r || r/abs(rgba[0]-r)>50)&&
+          (rgba[1]==g || g/abs(rgba[1]-g)>50)&&
+          (rgba[2]==b || b/abs(rgba[2]-b)>50));
+}
+
 void draw_dab_pixels_BlendMode_Normal_Paint (uint16_t * mask,
                                        uint16_t * rgba,
                                        uint16_t color_r,
@@ -91,7 +97,7 @@ void draw_dab_pixels_BlendMode_Normal_Paint (uint16_t * mask,
       uint32_t opa_b = (1<<15)-opa_a; // bottomAlpha
       // optimization- if background has 0 alpha we can just do normal additive
       // blending since there is nothing to mix with.
-      if (rgba[3] <= 0) {
+      if (rgba[3] <= 0 || opacity>30000 || color_direct_mixable(color_r, color_g, color_b, rgba)) {
         rgba[3] = opa_a + opa_b * rgba[3] / (1<<15);
         rgba[0] = (opa_a*color_r + opa_b*rgba[0])/(1<<15);
         rgba[1] = (opa_a*color_g + opa_b*rgba[1])/(1<<15);
@@ -351,6 +357,16 @@ void draw_dab_pixels_BlendMode_Normal_and_Eraser_Paint (uint16_t * mask,
     for (; mask[0]; mask++, rgba+=4) {
       const uint32_t opa_a = mask[0]*(uint32_t)opacity/(1<<15); // topAlpha
       const uint32_t opa_b = (1<<15)-opa_a; // bottomAlpha
+
+    if (rgba[3] <= 0 || opacity>30000 || color_direct_mixable(color_r, color_g, color_b, rgba)) {
+      const uint32_t opa_a1 = opa_a * color_a / (1<<15);
+      rgba[3] = opa_a1 + opa_b * rgba[3] / (1<<15);
+      rgba[0] = (opa_a1*color_r + opa_b*rgba[0])/(1<<15);
+      rgba[1] = (opa_a1*color_g + opa_b*rgba[1])/(1<<15);
+      rgba[2] = (opa_a1*color_b + opa_b*rgba[2])/(1<<15);
+      continue;
+    }
+
       const uint32_t opa_a2 = opa_a * color_a / (1<<15); // erase-adjusted alpha
       const uint32_t opa_out = opa_a2 + opa_b * rgba[3] / (1<<15);
 
@@ -458,7 +474,7 @@ void draw_dab_pixels_BlendMode_LockAlpha_Paint (uint16_t * mask,
       uint32_t opa_b = (1<<15)-opa_a; // bottomAlpha
       opa_a *= rgba[3];
       opa_a /= (1<<15);
-      if (rgba[3] <= 0) {
+      if (rgba[3] <= 0 || opacity>30000 || color_direct_mixable(color_r, color_g, color_b, rgba)) {
         rgba[0] = (opa_a*color_r + opa_b*rgba[0])/(1<<15);
         rgba[1] = (opa_a*color_g + opa_b*rgba[1])/(1<<15);
         rgba[2] = (opa_a*color_b + opa_b*rgba[2])/(1<<15);

Because most brushes are dabbed repeatedly around the same spot, so there are a lot overlapping, so if colours are close enough, we don't really need to do spectrum conversion, which greatly speeds up large brushes.

Results are still visually correct:

图片


My original description which I thought it was a bug: (wrong, don't look at it)

Because spectrum mixing is ridiculously slow even with fully opaque brushes, I am looking into the code and found the spectrum switch seems to be controlled by "alpha underneath" instead of "input alpha"? Is this a typo error or am I not understanding the algorithm correctly?

brushmodes.c line 375:

      if (spectral_factor && rgba[3] != 0) { // apparently it should be "color_a!=65535" here?
        // Convert straightened tile pixel color to a spectral
        float spectral_b[10] = {0};
        rgb_to_spectral(
          (float)rgba[0] / rgba[3],
          (float)rgba[1] / rgba[3],
          (float)rgba[2] / rgba[3],
          spectral_b
          );

I'm not sure I understood the code correctly, but I think for opaque brushes there should not be any spectrum mixing? Or if it's a different mindset I'd appreciate a more in-depth explanation.

Thanks and have a great day!

@ChengduLittleA
Copy link
Contributor Author

Also: I noticed in the forum that we might have a floating point back end that is supposed to be a bit faster? when is that gonna come?

@ChengduLittleA ChengduLittleA changed the title Spectrum mixing logic error in opaque condition? Spectrum mixing logic error? (My mistake) Sep 14, 2021
@ChengduLittleA
Copy link
Contributor Author

Hi I was mistaken in the original issue description... so I updated a patch to speed up the spectral mixing by directly mixing colous that are close enough.

@ChengduLittleA ChengduLittleA changed the title Spectrum mixing logic error? (My mistake) Spectrum mixing speed up by direct mixing colours that areclose enough Sep 14, 2021
@ChengduLittleA ChengduLittleA changed the title Spectrum mixing speed up by direct mixing colours that areclose enough Spectrum mixing speed up by direct mixing colours that are close enough Sep 14, 2021
@odysseywestra
Copy link
Member

@briend if you are still around, this would be up your alley to review.

@briend
Copy link
Contributor

briend commented Sep 15, 2021

I think it probably makes more sense to just flip pigment mode off if a brush is slow :-)

Maybe just make it exact and optimize when rgba[3] == 0 || opa_a == 1<<15. There's enough weird stuff in here that I don't really understand; I think it might be matching a lot more than you think it should and giving you the false impression that it's a lot faster (but it's acting a lot like just flipping pigment mode off :-). Using opacity instead of the applied dab mask opa_a is probably not good, and I don't know what this is doing, seems like the unsigned ints would wrap around when subtracting, and you have a div by zero potential there too:

r / abs( rgba[0] - r ) > 50

@ChengduLittleA
Copy link
Contributor Author

I think it might be matching a lot more than you think it should

Yeah that's how I expect it to be, in this case if the differential of colour and target rgb is within 1/50 of original colour, then it's off. It has to match a lot because a lot of brushes are just dabbling in the same places, like that acrylic brush, a lot of times things are just overlapping, mostly only dabs outside "dabbed" region would have meaningful pigment mixing. (If you apply it very lightly, the "inside" would also mix in pigment mode because the colour haven't been close enough). We can have a dynamic threshold as a setting as well.

and you have a div by zero potential there too

That's why I have that equal condition first.

@briend
Copy link
Contributor

briend commented Sep 19, 2021

Oh I didn’t know that the uint subtraction gets promoted to signed Int, so negatives are ok huh. I still wonder about comparing the alpha unassociated color (‘straight”) (r) with the associated alpha color (“premultiplied”) (rgba). Does that make sense? I also think || opacity>30000 is bound to cause issues. Imagine a soft airbrush that is fully opaque; it wont use spectrum mixing at all even though many dabs are < 30000 opacity as you move from the center and the dab mask is applied. But if you instead use the applied dab mask (opa_a > 30000) you’ll have a discontinuity when it jumps from additive to pigment somewhere in the middle of the gradient.

@ChengduLittleA
Copy link
Contributor Author

Ah I see... that could be an issue, but I don't think I fully understand the logic here... I probably should not be comparing with the premult value. What would you suggest is the best thing to do? After all the goal is to eliminate "a ton of" spectrum mixing for very opaque brushes.

I do have some colour differences when I apply airbrush pretty lightly and over a bit pressure, maybe that could be a case, but that could also be caused by the "very low alpha cut off" in the code, didn't check that carefully, however the current code works in my paintings pretty well, so I guess it's not that far off. Maybe just need some tweaks to those conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants