Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

treat single values as greyscale RGB colour #446

Closed
nebulon42 opened this issue Apr 29, 2016 · 8 comments
Closed

treat single values as greyscale RGB colour #446

nebulon42 opened this issue Apr 29, 2016 · 8 comments

Comments

@nebulon42
Copy link
Collaborator

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 mapbox/mapbox-studio-classic#1558

@nebulon42 nebulon42 added this to the 0.16.3 milestone Apr 29, 2016
@nebulon42
Copy link
Collaborator Author

nebulon42 commented Apr 29, 2016

@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
Copy link

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
    1. 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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@springmeyer Could you please publish 0.16.3?

@nebulon42
Copy link
Collaborator Author

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

@nyurik
Copy link

nyurik commented Jul 6, 2016

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

@nebulon42
Copy link
Collaborator Author

nebulon42 commented Jul 6, 2016

@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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants