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

Round-trip linear interpolators (release-boba) #11565

Merged
merged 2 commits into from
Mar 30, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Mar 30, 2018

Backport of #11564 and mapbox/mapbox-gl-js#6388 to the release-boba branch.

Unblocks #11389. Depends on mapbox/mapbox-gl-js#6427.

/cc @ChrisLoer @anandthakker

@1ec5 1ec5 added bug Core The cross-platform C++ core, aka mbgl labels Mar 30, 2018
@1ec5 1ec5 self-assigned this Mar 30, 2018
@1ec5 1ec5 requested a review from ChrisLoer March 30, 2018 02:26
@1ec5 1ec5 force-pushed the 1ec5-interpolate-linear-11562-boba branch from c9f1400 to e29f8c1 Compare March 30, 2018 02:36
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 30, 2018

This test is failing because mapbox/mapbox-gl-js@34902f8 includes mapbox/mapbox-gl-js#6388, which hasn’t been ported to gl-native yet:

  [
-   "to-rgba",
+   "literal",
    [
-     "rgba",
+     null,
    null,
    null,
      0,
-     0,
    0,
    0
    ]
]f([{},{}])
Expected: [0,0,0,0]
Actual: [null,null,null,0]f([{},{}])
Expected: [0,0,0,0]
Actual: [null,null,null,0]
* failed to-rgba zero

The division by zero is easy to address, but the test still fails because the to-rgba and rgba cancel each other out.

/cc @jfirebaugh

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 30, 2018

da95eca6356ec90260d7ac4205a06d04f9959c3d ports mapbox/mapbox-gl-js#6388 and depends on mapbox/mapbox-gl-js#6427, which corrects the relevant test fixture.

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple nitpicks inline. Also, since you'll be rebasing/squashing anyway, could we separate the color alpha 0 fix into a commit separate from the serialization fix?

@@ -106,12 +106,13 @@ Value ValueConverter<mbgl::Value>::toExpressionValue(const mbgl::Value& value) {
mbgl::Value ValueConverter<mbgl::Value>::fromExpressionValue(const Value& value) {
return value.match(
[&](const Color& color)->mbgl::Value {
auto array = color.toArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

s/auto/std::array<double, 4>/

@@ -23,11 +23,25 @@ optional<Color> Color::parse(const std::string& s) {
}

std::string Color::stringify() const {
auto array = toArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

s/auto/std::array<double, 4>/


std::array<double, 4> Color::toArray() const {
if (a == 0) {
return {{}};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for explicitly listing 0, 0, 0, 0 here

@1ec5 1ec5 force-pushed the 1ec5-interpolate-linear-11562-boba branch from da95eca to a88ed20 Compare March 30, 2018 22:34
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 30, 2018

Fixed (and squashed into two separate commits).

@1ec5 1ec5 force-pushed the 1ec5-interpolate-linear-11562-boba branch from a88ed20 to d066e94 Compare March 30, 2018 22:36
@1ec5 1ec5 merged commit d066e94 into release-boba Mar 30, 2018
@1ec5 1ec5 deleted the 1ec5-interpolate-linear-11562-boba branch March 30, 2018 23:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants