search

mapbox / carto

fast CSS-like map stylesheets
https://cartocss.readthedocs.io/
Apache License 2.0
652 stars 129 forks source link

mix weight parameter has been switched #442

Closed jakepruitt closed 8 years ago

jakepruitt commented 8 years ago

So it looks like the recent fix to mix in the RGB color space switched the functionality of the mix function's weight parameter.

Functionality change:

old: mix(blue,purple,100) = blue
new: mix(blue,purple,100) = purple

This is not detectable with 50% mixes, so it was pretty easy to slip through the cracks of the tests. Also, the documentation doesn't explicitly say how this functionality should work. It is breaking people's styles though, like @xrwang's style here: https://github.com/mapbox/mapbox-studio-classic/issues/1553.

@springmeyer @nebulon42 - would love to hear your opinions :)

pnorman commented 8 years ago

Right now there's unquestionably a bug in that the alpha is alpha1*p + alpha2*(1-p) while the color is color1*(1-p)+color2*p (Assuming you define addition and multiplication of colours sensibly)

I checked with with less and it just defines it as "a percentage balance point between the two colors, defaults to 50%" (less/less-docs#409)

However, less does define tint(color,weight) = mix(white, color, weight). Assuming tint(color, 0%) = color, then mix with weight=0 is 0% of the first colour and 100% of the second colour, which is the reverse of chroma.js's chroma.mix.

The relevant line should be something like chroma.mix(color2.toString(), color1.toString(), p, 'rgb'); or chroma.mix(color1.toString(), color2.toString(), 1-p, 'rgb'); with a comment that chroma.mix is the opposite order from less and cartocss. Right now it's var mix = chroma.mix(color1.toString(), color2.toString(), p, 'rgb');

nebulon42 commented 8 years ago

@jakepruitt thanks for reporting and @pnorman thanks for the background research. I had to get rid of the chroma.mix function since it behaved differently than LESS/SASS regarding mixing RGBA colours. I have now expanded the test cases and the mix function should work like in carto 0.15.3. You're welcome to verify that this is the case.

@tmcw @springmeyer I have prepared everything for a 0.16.1 release. Please sign off on it and tag/publish, thanks.

springmeyer commented 8 years ago

Thank you @nebulon42! @xrwang just tested and confirmed this is fixed. So I just tagged and published v0.16.1.