Eviscerate mbgl expression to Foundation JSON object conversion

[1ec5]

Piggy-back on #11156 to more robustly and succintly convert mbgl::style::expression::Expressions to JSON-style Foundation objects. Before merging this PR, I might consider merging MGLJSONObjectFromMBGLValue() with ValueEvaluator.

Fixes #11254. Depends on #11391 and #11565.

/cc @anandthakker

[1ec5]

Tests are currently failing because of an incomplete implementation of the number operator.

[anandthakker]

Is this requiring the components of an RGB(A) expression to be literals? If so, note that that isn't required by the core "rgb" / "rgba" expressions.

[1ec5]

This method is a convenience. If it returns nil, then the caller will synthesize a colorWithRed:green:blue:alpha: function expression instead, which can take arbitrary expressions as the components.

[fabian-guerra]

This PR is looking good.

This is what I have reviewed so far and needs to address the missing tests.

Documentation

- [ ] Public class/methods/properties/structs are documented.
- [ ] Documentation has Jazzy friendly links.

Code readability.

- [ ] Header imports are not duplicated.

• Method names has clear purpose.
• Usage of appropriate data types.
  - [ ] Use of auto for C++ objects.
  - [ ] Pointer returns.

Code functionality.

- [ ] Check project files are included on both platforms (ios, macos).

• Shared functionality is implemented on both platforms.
• Has no retain cycles.
• Has no side effects.
• Test cases test each scenario.

[fabian-guerra]

LGTM 👍🏼(just need to add the tests)

[1ec5]

@anandthakker @ChrisLoer, 02de0ce adds this special case so that ["linear"] round-trips as ["linear"] and not ["exponential", 1]. This balances out the existing special case in parseInterpolate() that converts ["linear"] to ExponentialInterpolator(1.0).

[1ec5]

So this is a problem, actually:

  [
  "interpolate",
  [
-     "exponential",
    1
+     "linear"
    ],
  [
    "heatmap-density"
  ],
  0,
  [
    "rgba",
    0,
    0,
    0,
    1
  ],
  1,
  [
    "rgba",
    255,
    0,
    0,
    1
  ]
]
* failed heatmap-density basic

This is caused by fixtures in GL JS:

https://github.com/mapbox/mapbox-gl-js/blob/7bc23b9b9df28afc54252a2d68d450aa24580c0b/test/integration/expression-tests/heatmap-density/basic/test.json#L37

We can adjust those fixtures, but I’m not sure how feasible it is to get that into the release-boba branch in time for a beta. For now, I may need to revert 02de0ce and work around this round-tripping issue in the SDK’s unit tests.

[ChrisLoer]

Updating the fixtures shouldn't take too long even though there's the annoying JS/native PR dance:

• Checkout master in the gl-js submodule
• run UPDATE=1 yarn run test-expressions from the native side
• Go go back into the submodule, verify the diff is just the exponential->linear change
• Create a GL JS PR, merge to master
• Update the pin on native

release-boba is already on a pretty recent pin of GL JS, I don't think you should have too much trouble updating the pin.

Too bad, I thought about putting in a "linear" special case in the first place but I think we decided we didn't have a need for it.

[1ec5]

No worries, I initially thought it would be OK because we weren’t trying to serialize to literally the same thing, only semantically the same thing. But this turned out to be one of the trickier things to work around in the SDK’s roundtripping unit tests.

[1ec5]

Thanks for the step-by-step explanation: #11564.

[anandthakker]

Does solving this cause the opposite problem, where ["exponential", 1.0] roundtrips as ["linear"] rather than ["exponential", 1.0]?

[1ec5]

It does, but it’s semantically equivalent, correct? Just as ["to-rgba", ["rgba", …]] gets collapsed down to ["literal", …], I think it’d be slightly less surprising for roundtripping to introduce this syntactic sugar than for it to remove the syntactic sugar. That was my takeaway from #8457 anyways.

(This change is actually part of #11565; this PR is based on that one.)

[anandthakker]

👍 makes sense, just wanted to make sure that this was the intent

[1ec5]

I split out #11565 to fix the linear interpolator issue; this PR depends on it. (A bit more yarn came out than I expected there.)

#11391 is once again rearing its ugly head, failing macOS builds on CI but not locally on High Sierra:

MGLBackgroundLayerTests
  testProperties, ((layer.backgroundColor) equal to (functionExpression)) failed: ("mgl_step:from:stops:($zoomLevel, NSCalibratedRGBColorSpace 0.985948 0 0.0269506 1, {18 = NSCalibratedRGBColorSpace 0.985948 0 0.0269506 1})") is not equal to ("mgl_step:from:stops:($zoomLevel, NSCalibratedRGBColorSpace 1 0 0 1, {18 = NSCalibratedRGBColorSpace 1 0 0 1})") - backgroundColor should round-trip camera expressions.
  /Users/distiller/project/platform/darwin/test/MGLBackgroundStyleLayerTests.mm:57
  
        XCTAssertEqualObjects(layer.backgroundColor, functionExpression,
                              @"backgroundColor should round-trip camera expressions.");

  

[1ec5]

All the builds now pass on CI, other than nitpick, which will pass once mapbox/mapbox-gl-js#6427 and #11565 land.