Use Transitionalable infrastructure for transfrom globeness changes.

[HarelM]

Launch Checklist

Hopefully the final PR to make the transition work with the current infrastructure.

I've decoupled the entanglement between globe projection and globe transform by moving the sync logic into the map render flow.
CameraHelper still needs the projection, but I'm not sure I have a good idea how to solve this, state management would've saved a lot of grief here, but we don't have that at the moment.

Removed most of the TODOs I had, still two places that I'm not sure how to solve.
Overall this is ready to be merged to the upstream branch and then to main, maybe...

[codecov]

Codecov Report

Attention: Patch coverage is 88.33333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 91.81%. Comparing base (6275dd2) to head (ff56cc2).
Report is 1 commits behind head on projection-expression.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/geo/projection/globe_projection.ts	68.00%	8 Missing ⚠️
src/geo/projection/projection_factory.ts	70.00%	6 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##           projection-expression    #5164      +/-   ##
=========================================================
- Coverage                  91.82%   91.81%   -0.02%     
=========================================================
  Files                        281      281              
  Lines                      38813    38723      -90     
  Branches                    6815     6727      -88     
=========================================================
- Hits                       35640    35552      -88     
+ Misses                      3044     3040       -4     
- Partials                     129      131       +2     

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

[HarelM]

I think I managed to incorporate most of the changes I wanted to introduce.
I'd love to know what I might have done wrong, or if you see any issues with the implementation and direction.
I still want to better decouple transform and projection, but since this seems to be working as expected (at least from the test perspective) I think this can be reviewed and merged to the other branch.

[HarelM]

I've added a few more corrections and fixed a few more of my "TODO"s, please review and let me know what you think.
It might be ready to be merged to main afterwards I think.

[HarelM]

Looking at the current experience, I'm not sure I understand the decision to make the transition to mercator only at zoom 12, I think around zoom 7 is more appropriate as you see the map fully and can't see it as a globe.
It might just be my experience, the nice thing about this feature is that every map designer can choose :-)

[HarelM]

When pitch is high, zoom 7 is not enough to "stop seeing the horizon", I now see it. My last question can be ignored.

[HarelM]

I wanted to change the following line in painter as part of this PR:

-const renderOptions: RenderOptions = {isRenderingToTexture: false, isRenderingGlobe: style.projection.name === 'globe'};
+const renderOptions: RenderOptions = {isRenderingToTexture: false, isRenderingGlobe: style.projection.transitionState > 0};

But I'm getting the following error in the render tests:

@birkskyum @ibesora I know you guys worked on it, is this an actual bug that needs to be fixed related to fog+globe+terrain that is currently a bit "hidden" due to the above change that needs to be done?

[ibesora]

@birkskyum @ibesora I know you guys worked on it, is this an actual bug that needs to be fixed related to fog+globe+terrain that is currently a bit "hidden" due to the above change that needs to be done?

The difference is due to fog not being rendered when globe projection is used. With your change, it is rendered when we've transitioned to mercator, which was something we weren't doing before because the fog matrix is not correctly set when rendering globe. Since the underlying transform is still globe when transitionState > 0, the fog matrix will be incorrect.
I'd keep it as it is until I get the fog support for globe done

[HarelM]

Is it possible to fix this branch to support fog only when mercator transform kicks in?
Mercator transform is begin used somewhat as a proxy in globe transform now, and I hope the fog matrix is calculated correctly there...?
Let me know if this small fix can be incorporated here to complete the transformation and remove the "hard codded" === "globe" from the code

[ibesora]

Nice improvement on top of the other ones you've been making!

[HarelM]

@kubapelc said he'll be able to review this tomorrow, so I'll wait for his feedback before I merge.

[ibesora]

Is it possible to fix this branch to support fog only when mercator transform kicks in?

Yeah, I think just returning the Mercator fog matrix on the globe function to compute the matrix would be enough. Will try it later and either push a commit to your branch or report back

[HarelM]

@ibesora thanks for the help! I've changed it to use the current transform so it is more general right now, when in mercator "state" it will use the mercator martix, I must have missed that when refactoring the globe and vertical perspective transfrom, thanks again!

[kubapelc]

I'm not sure I understand the decision to make the transition to mercator only at zoom 12

I didn't check pitched view when setting the constant :D I set it to 12, because I wanted it to be at as zoomed in as possible to hide the transition (the more zoomed in globe is, the more it converges to mercator), but around 13 or 14 there are already floating point artifacts. I think that original logic might be flawed anyway, since the artifacts are likely to appear at different zooms depending on latitude.

I still think the default transition should happen at as high zooms to make globe and mercator as similar as possible at the given zoom range, because symbol placement does not support projection transition and is either mercator or globe. The current setting (I'm not sure what it is exactly) seems to work well!

Making the transition configurable is a great change!

[HarelM]

Ok, I'll be merging this now.
If there are any code review changes I'll apply them to the upstream branch later today and then probably merge this too.
@kubapelc if you know of a good way to check that performance haven't degraded let me know.