Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Color interpolation with configurable color spaces #6442

Closed
wants to merge 7 commits into from
Closed

Color interpolation with configurable color spaces #6442

wants to merge 7 commits into from

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Sep 23, 2016

This is just a start so I can get an idea from the C++ illuminati
about whether it's the right direction. Approach here is

  • Since colors are arrays in JS and objects in C++, instead of a
    rgbToLab() method, there's a to_lab() method on the color object

Connected to mapbox/mapbox-gl-js#3245 - the GL JS side

@mention-bot
Copy link

@tmcw, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh and @yhahn to be potential reviewers.

@tmcw tmcw mentioned this pull request Sep 23, 2016
Merged
6 tasks
@jfirebaugh
Copy link
Contributor

I think eventually we'll need a set of color-space-specific classes -- class RGBColor, class LABColor, class HSLColor -- and a variant type to unify them. The latter will probably want to be defined as:

using ColorBase = variant<RBGColor, LABColor, HSLColor>;
class Color : public ColorBase {
public:
    using ColorBase::ColorBase;
    // now it's possible to define methods such as:
    RGBColor asRGB() const;
};

@tmcw
Copy link
Contributor Author

tmcw commented Oct 12, 2016

@jfirebaugh want to take a look at the code and the approach? This now passes unit tests locally, but there are at least three ways I could implement the to&from RGB/LCH conversion, and I chose the one I could get to work, not necessarily the most idiomatic.

jfirebaugh
jfirebaugh suggested changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types look good, just a few small nits.

PropertyEvaluator<Color>::operator() duplicates a lot of PropertyEvaluator<T>::operator() but I imagine that's something you're still working on.

include/mbgl/style/function.hpp
@@ -3,6 +3,14 @@
#include <vector>
#include <utility>

using EnumType = uint32_t;

enum class ColorSpace : EnumType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete EnumType; let the compiler choose.

include/mbgl/style/function.hpp
@@ -15,19 +23,26 @@ class Function {
explicit Function(Stops stops_, float base_)
: base(base_), stops(std::move(stops_)) {}

explicit Function(Stops stops_, float base_, ColorSpace colorSpace_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine with the previous constructor using a default parameter (ColorSpace colorSpace_ = ColorSpace::RGB).

include/mbgl/util/color.hpp
@@ -14,9 +16,14 @@ class Color {
float b = 0.0f;
float a = 0.0f;

ColorLAB to_lab();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toLAB

include/mbgl/util/color.hpp
@@ -14,9 +16,14 @@ class Color {
float b = 0.0f;
float a = 0.0f;

ColorLAB to_lab();
ColorHCL to_hcl();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toHCL

include/mbgl/util/color.hpp
static constexpr Color black() { return { 0.0f, 0.0f, 0.0f, 1.0f }; };
static constexpr Color white() { return { 1.0f, 1.0f, 1.0f, 1.0f }; };

static Color from_lab(const ColorLAB);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fromLAB/fromHCL

Parameter type should be const ColorLAB&

@1ec5 1ec5 mentioned this pull request Oct 17, 2016
Closed
@1ec5
Copy link
Contributor

1ec5 commented Oct 17, 2016

Tracking an iOS/macOS SDK implementation for the runtime styling API in #6721.

/cc @zugaldia

@tmcw
Native side of color interpolation
c7f954a 
This is just a start so I can get an idea from the C++ illuminati
about whether it's the right direction. Approach here is

* Since colors are arrays in JS and objects in C++, instead of a
  rgbToLab() method, there's a to_lab() method on the color object
@tmcw
Add ColorHCL type
63cb91c
@tmcw
Start colorspace interpolation
6b0daf6
@tmcw
from methods
712e5f8
@tmcw
Convert back out of LAB and HCL
0b43776
@tmcw
Test colorSpace-dependent functions
b7ffe67
@tmcw
Code review fixes
7e264d1 
* Default parameter for colorSpace in Function
* to_lab -> toLab, to_hcl -> toHCL, from_lab -> fromLAB, from_hcl ->
  fromHCL
* Pass colors by reference, not value
* Don't explicitly choose a type for the ColorSpace enum
@zugaldia zugaldia mentioned this pull request Oct 17, 2016
Closed
@jfirebaugh jfirebaugh mentioned this pull request Oct 22, 2016
Closed
@1ec5 1ec5 mentioned this pull request Jan 13, 2017
Merged
@1ec5 1ec5 mentioned this pull request Jan 26, 2017
Merged
This was referenced Feb 10, 2017
Merged
Closed
@1ec5 1ec5 added GL JS parity For feature parity with Mapbox GL JS runtime styling Core The cross-platform C++ core, aka mbgl labels Mar 4, 2017
@1ec5
Copy link
Contributor

1ec5 commented Mar 4, 2017

Looks like the changes requested in #6442 (review) have been implemented. @tmcw, would you mind rebasing this PR and resolving the conflicts?

@1ec5 1ec5 changed the title Native side of color interpolation Color interpolation Mar 4, 2017
@1ec5 1ec5 changed the title Color interpolation Color interpolation with configurable color spaces Mar 4, 2017
@jfirebaugh
Copy link
Contributor

This requires some non-trival rework now that data-driven properties have landed.

@tmcw tmcw closed this Apr 12, 2017
@jfirebaugh jfirebaugh mentioned this pull request Apr 12, 2017
Closed
@jfirebaugh jfirebaugh deleted the color-interpolation branch April 12, 2017 03:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jfirebaugh jfirebaugh jfirebaugh requested changes

Assignees
No one assigned
Labels
Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS runtime styling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmcw @mention-bot @jfirebaugh @1ec5