treat single values as greyscale RGB colour

mapbox / carto

fast CSS-like map stylesheets

https://cartocss.readthedocs.io/

Apache License 2.0

652 stars 129 forks source link

treat single values as greyscale RGB colour #446

Closed nebulon42 closed 8 years ago

nebulon42 commented 8 years ago

Currently e.g. 0.8 converts to HSL(0.8, 0.8, 0.8), which is quite different from RGB(0.8, 0.8, 0.8) of v0.15.3.

ref https://github.com/mapbox/mapbox-studio-classic/issues/1558

nebulon42 commented 8 years ago

@springmeyer I'm running into problems here and I might not be able to provide full backwards-compatibility. The problem is that I have to put 0.8 through the conversion functions and at the end depending on rounding errors something like rgb(0,0,0) or rgb(1,1,1) comes out.

For me color * 0.8 is some kind of hack instead of using the darken function, but since it is also in the osm-bright.tm2 style this is a problem. How should we proceed here? If you want full backwards-compatibility we can only revert the colour changes.

Apart from that there is clearly the bug that 0.8 is currently interpreted as HSL but was treated as RGB before 0.16.0.

springmeyer commented 8 years ago

My preference would be to aim for full back compatibility for v0.16.x, and so reverting sounds good. This is for 2 main reasons from my perspective:

1) I don't have the energy right now to fully understand the nature of the changes/breakage
2) There are a lot of other great changes and fixes that you landed in v0.16.x. Keeping those but reverting the color changes to maintain back compatibility seems wise. They could then be re-integrated carefully in v0.17.x?

nebulon42 commented 8 years ago

Ok, the one reason for me against reverting is that I currently see no way how to re-integrate the colour changes without running into this problem again.

Since the above mentioned line is a hacky way to deal with colours (it assumes that internally colours are represented in RGB) there cannot be a guarantee that this could not break sometimes. So it may be possible to require a change downstream in this case.

But I see that this might have bigger implications for dependent projects so rolling back might be a sensible solution. IMO it would hinder future changes/improvements in colour handling.

So I'm asking bluntly: Shall I roll back the whole change?

nebulon42 commented 8 years ago

It now seems that I'm able to achieve full backwards compatibility. So I guess reverting is not necessary. There have been a few regressions lately for which I apologize. Hopefully this was it.

nebulon42 commented 8 years ago

@springmeyer Could you please publish 0.16.3?

nebulon42 commented 8 years ago

@nyurik Would you be able to test if 0.16.3 fixes the issues with your OSM Bright fork?

nyurik commented 8 years ago

@nebulon42 mapbox-studio-linux-x64-v0.3.8 seems to work ok, thx!

nebulon42 commented 8 years ago

@nyurik Mapbox Studio Classic 0.3.8 seems to use carto 0.15.3. Have you upgraded carto by hand and tried it? If not, could you please try to bump carto to 0.16.3 in package.json, run npm install and try again?