Skip to content

comp-op=saturation does not work as expected #1493

Closed
ajashton opened this Issue Sep 19, 2012 · 8 comments

3 participants

@ajashton
Mapnik member

A saturation comp op should result in an image with the saturation of the source and the hue and lightness of the destination. It looks like Mapnik's saturation comp of is taking the saturation of the source and the lightness of the destination correctly, but the final hue value is not correct.

For example, here is a polygon-fill of #ace (blue) with comp-op=saturation overlaid on another polygon-fill of the exact same color. The resulting circle should be the same solid blue because the saturation of the source and destination are the same. But something weird is going on:

The same thing happens whether the comp op is applied to the style or the symbolizer.

@lightmare

I'm able to replicate with comp-op on the symbolizer, but not on the style.

Adding some debug output shows RGB->HSV conversion is broken (I scaled the numbers so that H=(0..360), S,V=(0..100)). Your #ace color's hue should be somewhere between 180 and 240.

rgb(85,102,119;128) -> hsv(330,28,46)
hsv(330,28,46) -> rgb(118,84,101)

rgb(170,204,238;255) -> hsv(330,28,93)
hsv(330,28,93) -> rgb(238,170,204)
@lightmare

Here's my proposed fix. I'll comment on it when I get home, now I need to catch a bus. You can test in the meantime ;)

edit: Just one quick note: the evil thing that broke it was boost::numeric_cast<int>, everything else is just comments and minor optimization.

diff --git a/boost/gil/extension/toolbox/hsv.hpp b/boost/gil/extension/toolbox/hsv.hpp
index afaf1ff..1eb2e33 100644
--- a/boost/gil/extension/toolbox/hsv.hpp
+++ b/boost/gil/extension/toolbox/hsv.hpp
@@ -60,9 +60,16 @@ struct default_color_converter_impl< rgb_t, hsv_t >
       bits32f temp_blue  = channel_convert<bits32f>( get_color( src, blue_t()  ));

       bits32f hue, saturation, value;
+      bits32f min_color, max_color;

-      bits32f min_color = std::min(temp_red,std::min(temp_green, temp_blue));
-      bits32f max_color = std::max(temp_red,std::max(temp_green, temp_blue));
+      if( temp_red < temp_green ) {
+          min_color = std::min( temp_blue, temp_red );
+          max_color = std::max( temp_blue, temp_green );
+      }
+      else {
+          min_color = std::min( temp_blue, temp_green );
+          max_color = std::max( temp_blue, temp_red );
+      }

       value = max_color;

@@ -85,7 +92,7 @@ struct default_color_converter_impl< rgb_t, hsv_t >
       }   
       else
       { 
-         if( std::abs( boost::numeric_cast<int>(temp_red - max_color) ) < 0.0001f )
+         if( temp_red == max_color )
          {
             hue = ( temp_green - temp_blue )
                 / diff;
@@ -149,18 +156,23 @@ struct default_color_converter_impl<hsv_t,rgb_t>

          frac = h - i;

+         // p = value * (1 - saturation)
          p = get_color( src, value_t() ) 
            * ( 1.f - get_color( src, saturation_t() ));

+         // q = value * (1 - saturation * hue_frac)
+         // it drops with increasing distance from floor(hue)
          q = get_color( src, value_t() )
            * ( 1.f - ( get_color( src, saturation_t() ) * frac ));

+         // t = value * (1 - (saturation * (1 - hue_frac))
+         // it grows with increasing distance from floor(hue)
          t = get_color( src, value_t() ) 
            * ( 1.f - ( get_color( src, saturation_t() ) * ( 1.f - frac )));

-         switch( i )
+         switch( i % 6 )
          {         
-            case 0: 
+            case 0: // red to yellow
             {
                red   = get_color( src, value_t() );
                green = t;
@@ -169,7 +181,7 @@ struct default_color_converter_impl<hsv_t,rgb_t>
                break;
             }

-            case 1: 
+            case 1: // yellow to green
             {
                red   = q;
                green = get_color( src, value_t() );
@@ -178,7 +190,7 @@ struct default_color_converter_impl<hsv_t,rgb_t>
                break;
             }

-            case 2: 
+            case 2: // green to cyan
             {
                red   = p;
                green = get_color( src, value_t() );
@@ -187,7 +199,7 @@ struct default_color_converter_impl<hsv_t,rgb_t>
                break;
             }

-            case 3: 
+            case 3: // cyan to blue
             {
                red   = p;
                green = q;
@@ -196,7 +208,7 @@ struct default_color_converter_impl<hsv_t,rgb_t>
                break;
             }

-            case 4: 
+            case 4: // blue to magenta
             {
                red   = t;
                green = p;
@@ -205,7 +217,7 @@ struct default_color_converter_impl<hsv_t,rgb_t>
                break;
             }

-            case 5: 
+            case 5: // magenta to red
             {
                red   = get_color( src, value_t() );
                green = p; 
@springmeyer
Mapnik member

an an aside we do have some mods compared to upstream, but they seem functionally the same:


$ wget http://gil-contributions.googlecode.com/svn/sandbox/boost/gil/extension/toolbox/hsv.hpp
--2012-09-20 11:03:52--  http://gil-contributions.googlecode.com/svn/sandbox/boost/gil/extension/toolbox/hsv.hpp
Resolving gil-contributions.googlecode.com... 74.125.129.82, 2607:f8b0:400e:c02::52
Connecting to gil-contributions.googlecode.com|74.125.129.82|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 5792 (5.7K) [text/plain]
Saving to: `hsv.hpp'

100%[=================================================================================>] 5,792       --.-K/s   in 0.007s  

2012-09-20 11:03:53 (798 KB/s) - `hsv.hpp' saved [5792/5792]

~/projects/mapnik5[master]$ diff -u hsv.hpp boost/gil/extension/toolbox/hsv.hpp 
--- hsv.hpp     2011-12-04 17:39:06.000000000 -0800
+++ boost/gil/extension/toolbox/hsv.hpp 2012-07-31 22:14:26.000000000 -0700
@@ -61,8 +61,8 @@

       bits32f hue, saturation, value;

-      bits32f min_color = (std::min)( temp_red, (std::min)( temp_green, temp_blue ));
-      bits32f max_color = (std::max)( temp_red, (std::max)( temp_green, temp_blue ));
+      bits32f min_color = std::min(temp_red,std::min(temp_green, temp_blue));
+      bits32f max_color = std::max(temp_red,std::max(temp_green, temp_blue));

       value = max_color;

@@ -85,7 +85,7 @@
       }   
       else
       { 
-         if( (std::abs)( boost::numeric_cast<int>(temp_red - max_color) ) < 0.0001f )
+         if( std::abs( boost::numeric_cast<int>(temp_red - max_color) ) < 0.0001f )
          {
             hue = ( temp_green - temp_blue )
                 / diff;
@@ -129,7 +129,7 @@
       bits32f red, green, blue;

       //If saturation is 0, the color is a shade of gray
-      if( abs( get_color( src, saturation_t() )) < 0.0001f  )
+      if( std::abs( get_color( src, saturation_t() )) < 0.0001f  )
       {
          // If saturation is 0, the color is a shade of gray
          red   = get_color( src, value_t() );

@lightmare

Well, since temp_red and max_color are both in the range [0;1], that numeric_cast will yield 0 unless one of them is 0.0 and the other 1.0 -- but what you need to know is whether red is the prevailing component. And since max_color was previously set to either temp_red, temp_green, or temp_blue, I believe you can safely just compare the values without precision-handling tricks.

Further below, I changed switch(i) to switch(i % 6) -- assuming h could originally be equal to 1.0, so int(floor(h*6)) would be 6 and no case would match.

As for the comments, I added them while trying to understand how it works, sorry they take most of the patch :)

@springmeyer springmeyer referenced this issue in mapbox/tilemill Sep 21, 2012
Open

Colorize-alpha #1715

@springmeyer springmeyer added a commit that referenced this issue Oct 2, 2012
@springmeyer springmeyer expose more compositing options in python (the non-agg custom ones) t…
…o set up for testing as per #1493 and #1369
7866cc3
@springmeyer
Mapnik member

applied patch, no major changes seen in the python compositing tests. Will look for an opportunity to test further - so leaving open.

@lightmare

That could be because test images commited before the patch were already using it. I'm getting different (wrong) results for "saturation" and "value" without the patch. Anyway, let's see whether it solves the original issue.

@springmeyer
Mapnik member

yeah, that seems likely, I think there was a bug whereby things were not getting rebuilt which I think I fixed in 1468133

@springmeyer
Mapnik member

closing, I think this was resolved by the original patch.

@PetrDlouhy PetrDlouhy added a commit to PetrDlouhy/mapnik that referenced this issue Aug 22, 2013
@springmeyer springmeyer expose more compositing options in python (the non-agg custom ones) t…
…o set up for testing as per #1493 and #1369
7052bca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.