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

Projection expressions #5040

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 13, 2024

Blocked by / How to use

Closes #5039

interpolate example

const exprestInt = createExpression(['interpolate', ['linear'], ['zoom'], 10, 'vertical-perspective', 12, 'mercator'], v8.projection.type)
Screen.Recording.2024-11-13.at.01.28.44.mov

step example

const exprestInt = createExpression(['step', ['zoom'], 'vertical-perspective', 12, 'mercator'], v8.projection.type)
Screen.Recording.2024-11-13.at.01.50.51.mov

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!
  • Add an entry to CHANGELOG.md under the ## main section.

@birkskyum birkskyum changed the title Projection expression support Projection expression support (support interpolate-projection) Nov 13, 2024
@birkskyum birkskyum changed the title Projection expression support (support interpolate-projection) Projection expression support (interpolate-projection etc.) Nov 13, 2024
@birkskyum birkskyum changed the title Projection expression support (interpolate-projection etc.) Projection expressions (interpolate-projection, step) Nov 13, 2024
src/ui/events.ts Outdated
@@ -752,7 +752,7 @@ export type MapProjectionEvent = {
* Specifies the name of the new projection.
* Additionally includes 'globe-mercator' to describe globe that has internally switched to mercator.
*/
newProjection: ProjectionSpecification['type'] | 'globe-mercator';
newProjection: ProjectionConfigSpecification['type'] | 'globe-mercator';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove globe-mercator?

@HarelM
Copy link
Collaborator

HarelM commented Nov 13, 2024

I was expecting the code here to use the expression value of projection, is it using the expression value somewhere that I have missed?

@HarelM HarelM mentioned this pull request Nov 13, 2024
15 tasks
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.27%. Comparing base (8555091) to head (63b317f).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/style/style.ts 63.63% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5040      +/-   ##
==========================================
+ Coverage   89.25%   89.27%   +0.02%     
==========================================
  Files         269      269              
  Lines       38286    38330      +44     
  Branches     2347     2427      +80     
==========================================
+ Hits        34172    34220      +48     
- Misses       3117     3130      +13     
+ Partials      997      980      -17     

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

@birkskyum birkskyum changed the title Projection expressions (interpolate-projection, step) String support for ZoomDependent camera expressions (useful for projection expressions) Nov 14, 2024
@birkskyum birkskyum changed the title String support for ZoomDependent camera expressions (useful for projection expressions) Projection expressions Nov 14, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@birkskyum
Copy link
Member Author

birkskyum commented Nov 17, 2024

@HarelM , there is some preparation work needed in GL JS before this can be hooked up to the map style.

I think the best take on it is to make this PR just an "'update style-spec", allowing the createExpression to work. Then we can refactor incrementally on the main branch, until we can hook it up to the map.style, and only then make the changelog item for the new feature. Do you think that can work?

For example, I see some issues with tile loading, when going between projections:

Screen.Recording.2024-11-17.at.14.38.39.mov

And if terrain is enabled, the globe/vertical-perspective fog appear to break things.

Screen.Recording.2024-11-17.at.14.39.52.mov

Also, the vertical-perspective projection, in the setProjection, it doesn't take a globeness value that we can use to pass in the transition.

Also there could be some perf here, where we maybe should update the globeness in the existing projection instance, instead of making a lot of new Projection instances.

@HarelM
Copy link
Collaborator

HarelM commented Nov 17, 2024

Spec package will be updated automatically in a few days by dependabot.
If there are other changes related to this package update then it should be part of this PR I guess.
Other existing bugs not related to the sec change should be handled regardless of this change.

@birkskyum birkskyum closed this Nov 17, 2024
@birkskyum
Copy link
Member Author

birkskyum commented Nov 17, 2024

Sounds good, I'll leave it to dependabot to make the version change (thought it didn't handled major-release updates).

@birkskyum birkskyum reopened this Nov 17, 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.

Feature parity "interpolate" expressions for projectionDefinition
3 participants