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

Color operations #479

Closed
wants to merge 2 commits into from
Closed

Color operations #479

wants to merge 2 commits into from

Conversation

edenh
Copy link
Contributor

@edenh edenh commented Jun 21, 2014

ref mapbox/mapbox-gl-style-spec#38

Replaces https://github.com/deanm/css-color-parser-js with https://github.com/harthur/color to allow for color operations:

  • greyscale
  • lighten (%)
  • darken (%)
  • saturate (%)
  • desaturate (%)
  • fadein (%)
  • fadeout (%)
  • spin (degrees)
  • mix (second color) - this does not take a value to mix by, always 50%

Syntax example:

{
    "[prop]-color": {
        "value": "#0000FF",
        "operation": [["lighten", 10], ["spin", 35]]
    },
    "[prop]-color": {
        "value": "#0000FF",
        "operation": ["mix", "red"]
    },
    "[prop]-color": {
        "value": "#0000FF",
        "operation": "greyscale"
    }
}

Note: values cannot yet be constants

@ansis @mourner @kkaefer can someone please review?

@mourner
Copy link
Member

mourner commented Jun 21, 2014

With this color library, we're back where we were in the beginning, with color parsing taking the biggest part of the whole bundled code (about 38kb uncompressed). This is up from 10 lines of code with a canvas hack @tmcw.

If we're implementing color functions and keeping a relatively lightweight color-parser-js, let's at least implement functions ourselves, because harthur/color is really too bloated to be used in a browser library.

@tmcw
Copy link
Contributor

tmcw commented Jun 21, 2014

@mourner - I'm not following how this is related to replacing the Canvas hack - was there some way the Canvas hack would solve color operations other than the 'mix' operation'? You could possibly try to use it for fade, but canvas's behavior with low opacity values is very irregular.

Let's port carto's simple operations first - this will cover everything but interpolating in different spaces, and for that we would probably use something like d3's color spaces or my color-system project, that just does color systems.

@edenh
Copy link
Contributor Author

edenh commented Jun 21, 2014

@mourner @tmcw sounds good. Would someone explain the canvas hack?

@tmcw
Copy link
Contributor

tmcw commented Jun 21, 2014

@edenh #426 - originally we were using a Canvas element to parse color formats, instead of css-color-parser-js. This was very simple code-wise but not uniform between browsers, couldn't be tested in node-land, and 50x slower than parsing in js.

@mourner
Copy link
Member

mourner commented Jun 21, 2014

@tmcw no, I'm just explaining why I was so compelled to replace the color
module with Canvas color parsing in the first place — it meant shaving off
something like 20-30% of the total library size at the time. Using
css-color-parser is acceptable because it's many times smaller. I agree
with what you suggest.

суббота, 21 июня 2014 г. пользователь edenh написал:

@mourner https://github.com/mourner @tmcw https://github.com/tmcw
sounds good. Would someone explain the canvas hack?


Reply to this email directly or view it on GitHub
#479 (comment).

Vladimir Agafonkin
http://agafonkin.com/en
+380 (93) 745 44 61

@yhahn
Copy link
Member

yhahn commented Jun 23, 2014

Hrm, I'm curious if we really want to support this as part of the core style spec. It puts a pretty high demand on implementers (of course only JS/native atm) and no other formats I know of support this kind of thing natively.

This seems more like preprocessor/editor territory to me.

@mourner
Copy link
Member

mourner commented Jun 23, 2014

This seems more like preprocessor/editor territory to me.

Good point, I agree. This is only useful when writing most styles by hand.

@edenh
Copy link
Contributor Author

edenh commented Jun 23, 2014

Closing for now.

@edenh edenh closed this Jun 23, 2014
@yhahn yhahn deleted the color-ops branch July 3, 2014 11:12
tmcw added a commit to cutting-room-floor/mapbox-gl-style-lint that referenced this pull request Nov 19, 2014
This is a failing test that demonstrates the issue: it would need
a fix - specifically a removal of the unknown key behavior at the root
object - to pass.

I think we should tolerate extra keys in style objects because they're
very useful for the editor use case, like if you want to store extra
data for color transforms etc., that is not handled by the gl renderer
directly (see mapbox/mapbox-gl-js#479 )

The only potential drawback I see here is if gl-native's parsing
behavior would have problems with additional data, but it seems
very unlikely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants