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

Rename adaptive globe to globe-to-mercator #878

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 2, 2024

Rename of the adaptive globe from globe to globe-to-mercator as was suggested here:

Birk: it would be great to have the projection API extend well to: maplibre/maplibre-gl-js#168

Harel: Maybe we can name it globe-mercator to indicate both are used, IDK.

Jakub: I think globe-mercator is a good name.

There are many good reasons for leaving globe open to a globe-only projection (no mercator transition), and to be more explicit about what globe-to-mercator does. Doing an early rename of the adaptive projection will minimize the impact on plugin authors that are now starting to use our adaptive globe projection:

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!

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.73%. Comparing base (248b4f5) to head (1295a51).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #878   +/-   ##
=======================================
  Coverage   92.73%   92.73%           
=======================================
  Files         105      105           
  Lines        4683     4683           
  Branches     1323     1323           
=======================================
  Hits         4343     4343           
  Misses        340      340           

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

@birkskyum birkskyum requested a review from HarelM November 2, 2024 12:54
@birkskyum birkskyum changed the title Accurate projection language, split static/adaptive globe Accurate projection language, split globe-static and globe-to-web-mercator Nov 2, 2024
@birkskyum birkskyum changed the title Accurate projection language, split globe-static and globe-to-web-mercator Accurate projection language, split globe and globe-to-web-mercator Nov 2, 2024
@birkskyum birkskyum changed the title Accurate projection language, split globe and globe-to-web-mercator Accurate projection language Nov 2, 2024
@birkskyum birkskyum changed the title Accurate projection language mercator, globe, globe-mercator Nov 2, 2024
@HarelM
Copy link
Collaborator

HarelM commented Nov 2, 2024

When looking at this PR I can't help but think about making the value an interpulatable with expression. This way you have full control over the transition and can develop other layers (custom mainly) knowing exactly when things change instead of making assumptions.
This can lead in the future to allow transition from any protection to any protection based on zoom level (or maybe other stuff, IDK).
Let me know what you think.
Cc: @Pessimistress, @ibesora @louwers

@birkskyum
Copy link
Member Author

It's an interesting idea. I think we're better off hardcoding the useful options for the time being, and then later on re-asses if we can generalize something.

@HarelM
Copy link
Collaborator

HarelM commented Nov 2, 2024

Let's continue the discussion in the relevant issue.

@birkskyum
Copy link
Member Author

birkskyum commented Nov 3, 2024

That's a good idea. This PR just adds a setting to be able to use the already-implemented globe projection non-adaptively, comparable to the mercator.

@birkskyum birkskyum changed the title mercator, globe, globe-mercator Have non-adaptive globe and adaptive globe-mercator Nov 3, 2024
@birkskyum birkskyum changed the title Have non-adaptive globe and adaptive globe-mercator Rename to non-adaptive globe and adaptive globe-mercator Nov 3, 2024
@birkskyum birkskyum changed the title Rename to non-adaptive globe and adaptive globe-mercator Rename adaptive globe to globe-mercator Nov 4, 2024
@birkskyum
Copy link
Member Author

birkskyum commented Nov 4, 2024

@HarelM , I've reduced this to just the rename of the adaptive globe from globe to globe-mercator like you suggested.

@louwers
Copy link
Collaborator

louwers commented Nov 4, 2024

I think globe-mercator if pretty confusing. 🫠

Also, now that I think of it, is a globe even a projection? I mean, it is in the mathematical sense, but a 'map projection' is commonly a projection of a globe on a plane, not the other way around. https://en.wikipedia.org/wiki/Map_projection

We should maybe bring some GIS experts into the discussion? Edit: I called in the troops, maybe we get a good response. :) https://elk.zone/floss.social/@bart/113424884295371526

@birkskyum
Copy link
Member Author

birkskyum commented Nov 4, 2024

@louwers the globe is a Spherical projection.

We need space for a non-adaptive globe for a projection that doesn't transition to mercator, to support:

  • Custom layers based on spherical logic, that doesn't map well to the mercator.
  • Possible to accurately align overlays that doesn't yet support the transition to mercator, such like deckgl.
  • Allow for zooming into the poles, which isn't possible in a which isn't possible in Web Mercator, since it's a transverse mercator projection with the cylinder going though the poles.
  • Avoid issues of projection transitioning. When on the Globe with Terrain3D, there's a lot of issues with the transition to mercator.
    • Runtime Performance. As an example, the projections use different tile loading algorithms, causing extra tile loading / rendering.
    • Bundle size.
      • Each projection is very heavy. The adaption-logic + globe made the production bundle go from 800 to 877 kb ( 9.6% increase ⚠️ ), which bring a need for custom builds with single projections. Lacking bundle-splitting will cause adoption problems for projects that are mercator-only (like bing) and globe-only (like weather services). It's only possible to bundle-split if the projection can run standalone.
      • I've been investigating ESM / tree shaking for years now, but since the projection can change at runtime (because it's in the style), instead of being a e.g. a Class that's imported before compilation, I don't think we can tree shake this. If we have more projections down the line, we might want to consider lazy loading them as an alternative, or having them in dedicated npm packages, but that's, like expressions, possible to look into later on.
    • Different handling of sky/atmosphere/horizon.
    • The noticeable wrapping animation
      • On high zoom the horizon curvature is removed, and the horizon is much much closer, which looks odd.
      • On low zoom the whole globe is wrapped/unwrapped if you zoom out fast enough).
  • Feature parity with other projects that support this - like ol-cesium

@louwers
Copy link
Collaborator

louwers commented Nov 4, 2024

Maybe globe-to-mercator would be a bit more descriptive?

Wr.t. to @HarelM's comment about expressions. At a later stage we can take an object for this property that you can use to configure where transitions occur. We would keep the string version for backward compatibility and so people can easily pick a sensible default without having to supply a complicated object-based configuration if they don't need it.

@birkskyum birkskyum changed the title Rename adaptive globe to globe-mercator Rename adaptive globe to globe-to-mercator Nov 4, 2024
@pka
Copy link
Contributor

pka commented Nov 4, 2024

globe-to-mercator sounds good to me. I've made a prototype switching from Equal Earth projection to Web Mercator, so it could even make sense to declare more than one projection. But transitioning from a globe to a flat map is special enough to justify a dedicated combined projection name.

@birkskyum birkskyum merged commit d009e1d into main Nov 4, 2024
6 checks passed
@birkskyum birkskyum deleted the explicit-projection-naming branch November 4, 2024 17:28
@birkskyum
Copy link
Member Author

birkskyum commented Nov 4, 2024

@pka Equal Earth support sounds great! Looking forward to see it.

@birkskyum
Copy link
Member Author

birkskyum commented Nov 4, 2024

Breaking experimental / under-development properties, is that technically a SemVer patch?

@HarelM
Copy link
Collaborator

HarelM commented Nov 4, 2024

If someone is counting on this version to change when there are breaking changes then yes.
Since I don't think a lot of people care about the version of the style-spec I would advise to change the major version of it to keep semver standard.
I must say I still don't think this is a good approach to the style spec - mainly because if we think of the projection as an equivalent to a color name, then you don't have blue-to-black colors when you want to change the color based on zoom level, you have blue, black and an expression if you want to do "transitioning" colors...
Anyway... I guess I'm in the minority here...

@louwers
Copy link
Collaborator

louwers commented Nov 4, 2024

@HarelM On the other hand, if 99% of the people where happy with blue-to-black and didn't want to customize it further, it would make sense to have a shorthand for it.

birkskyum added a commit to birkskyum/maplibre-style-spec that referenced this pull request Nov 5, 2024
HarelM pushed a commit that referenced this pull request Nov 5, 2024
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.

5 participants