Skip to content

Conversation

@birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 13, 2024

@HarelM

There's still no interpolate-projection, but this small extension to your work makes the:

  "interpolate",
      [
        "linear"
      ],
      [
        "zoom"
      ],
      8,
      ["vertical-perspective","vertical-perspective",1],
      10,
      ["mercator","mercator",1]

Into this, which I find is a lot cleaner:

  "interpolate",
      [
        "linear"
      ],
      [
        "zoom"
      ],
      8,
      "vertical-perspective",
      10,
      "mercator"

For some reason, I just can't see why it's better for the API to use ["vertical-perspective","vertical-perspective",1] to describe the zoom stops, when the last element is ignored, and the other duplicated.

Just to double-check, do you prefer the arrays, even though they hold no additional information than a primitive string?

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@birkskyum birkskyum requested a review from HarelM November 13, 2024 13:56
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (6ac2725) to head (3f642e3).

Files with missing lines Patch % Lines
src/expression/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                            Coverage Diff                             @@
##           interpolate-expression-with-projection     #902      +/-   ##
==========================================================================
- Coverage                                   92.62%   92.56%   -0.07%     
==========================================================================
  Files                                         107      107              
  Lines                                        4761     4761              
  Branches                                     1352     1352              
==========================================================================
- Hits                                         4410     4407       -3     
- Misses                                        351      354       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@birkskyum birkskyum changed the title Use strings instead of the projection arrays Tweak - Use strings instead of strings parsed to projection arrays Nov 13, 2024
@birkskyum birkskyum merged commit 72bf603 into maplibre:interpolate-expression-with-projection Nov 13, 2024
5 checks passed
],
0,
["literal", ["vertical-perspective", "vertical-perspective", 1]],
"vertical-perspective",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean literal is no longer supported?

Copy link
Member Author

@birkskyum birkskyum Nov 13, 2024

Choose a reason for hiding this comment

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

Yes, it makes very little sense to me why we want to support literals representing the transition from one transition to the other, unless we want to support transitions between intermediates (which I don't consider particularly useful - I've yet to think of a use):

5,
["vertical-perspective", "mercator", 0.2],
 10,
["equal-earth", "utm23", 0.5]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough.

@HarelM
Copy link
Collaborator

HarelM commented Nov 13, 2024

I don't prefer the array, I just think it makes sense to support them, isn't it?

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.

3 participants