Facilitate for globe transition expression refactoring #5139

HarelM · 2024-12-01T15:55:05Z

Launch Checklist

This is a draft PR at this point to see what still need to be fixed and for others to see.
CC: @birkskyum @ibesora @kubapelc

The idea behind this refactoring:

Move all logic that is common for vertical-perspective and mercator to the transform helper.
Create a new class to hold all the logic of transitioning while relaying on the two transforms to do their part when called.

I wanted to get an initial feedback and see if I can do only the refactoring and keep the current state as is.
I don't think I managed to do it, I still need to see what I broke.
But if this doesn't break anything we can merge it as is and do another PR to support the expression.

Let me know what you think.

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

src/util/util.ts

codecov · 2024-12-01T16:02:47Z

Codecov Report

Attention: Patch coverage is 87.23629% with 121 lines in your changes missing coverage. Please review.

Project coverage is 91.78%. Comparing base (9a85e82) to head (94019c3).
Report is 4 commits behind head on main.

❌ We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.

Files with missing lines	Patch %	Lines
...c/geo/projection/vertical_perspective_transform.ts	84.21%	111 Missing ⚠️
src/geo/projection/globe_transform.ts	88.31%	6 Missing and 3 partials ⚠️
src/geo/projection/projection_factory.ts	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5139      +/-   ##
==========================================
- Coverage   91.82%   91.78%   -0.04%     
==========================================
  Files         278      279       +1     
  Lines       38347    38647     +300     
  Branches     6705     6752      +47     
==========================================
+ Hits        35213    35474     +261     
- Misses       3001     3039      +38     
- Partials      133      134       +1

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

birkskyum · 2024-12-01T16:22:06Z

I think this looks like the right direction, with the globe transform (being a mercator<->verticalPerspective lerp) to contain a mercatorTransform and now also a verticalPerspectiveTransform, and call into those, instead of having too much logic of it's own.

HarelM · 2024-12-01T18:20:52Z

I think I won't be able to understand what I changed to break the globe view just from looking at the code.
I'll create a new branch and make sure I do it one step at a time hopefully without breaking stuff on the way.

HarelM · 2024-12-01T21:14:25Z

Surprisingly I think I managed to find what created the issues in the branch after trying to recreate it in another branch...
I'll see if I can fix the remaining failing unit test, but otherwise this might be ready for review.
@kubapelc I found strange behavior in the getProjectionData and getProjectionDataForCustomLayer - both methods don't really follow the "if globe do that otherwise do what we did in mercator".
Last two commits are basically reverting the logic there, but I still find it odd.
Any chance you can take a look and let me know if there are improvements that can be done?

kubapelc · 2024-12-02T08:08:28Z

The new getProjectionData on globe transform look good to me! As for getProjectionDataForCustomLayer it is somewhat broken in main right now, I'm fixing it to resolve #5117. But I have some render tests failing after the fix, and I haven't figured it out yet.

HarelM · 2024-12-02T08:37:46Z

I think you can simply fix it by opening a PR to this branch, it would be better to avoid the need for me to resolve conflicts.
Is it possible to get a fallbackMatrix without the code from mercator transform? Or should I move part of the code from mercator transfrom to the helper to calculate the fallbackMatrix in case of globe/vertical-perspective?
There is also the same question for prefetchTiles and a few more other places where I couldn't find a proper solution.
If you could take a look at those it would be great.
I'm also thinking about splitting the camera helper the same way I did for the globe transform, it creates a clear separation of code and is very easy to see where issues exists this way.

kubapelc · 2024-12-02T08:42:45Z

src/geo/projection/vertical_perspective_transform.ts

+        mat4.scale(globeMatrixUncorrected, globeMatrixUncorrected, scaleVec); // Scale the unit sphere to a sphere with diameter of 1
+        this._globeViewProjMatrixNoCorrection = globeMatrixUncorrected;
+
+        mat4.rotateX(globeMatrix, globeMatrix, this.center.lat * Math.PI / 180.0);


GPU atan correction is missing here. Maybe it got deleted by accident?

Originally this line was:

mat4.rotateX(globeMatrix, globeMatrix, this.center.lat * Math.PI / 180.0 - this._globeLatitudeErrorCorrectionRadians);

I wanted to move it to the globe code and out of vertical perspective probably forgot to add it where needed.
Thanks! I'll see if there's a good way to solve this, maybe pass this parameter to the method somehow...

I think you can pass GlobeProjection object to vertical_perspective_transform and copy the code around _globeLatitudeErrorCorrectionRadians there too.

I'm not sure this is the right way to "share data", I've seen this with useGlobeControls and I'm still thinking if there's a better way to handle this, maybe thought the helper, I'm not sure...
There's also a lot of duplicated code to create each transform, maybe we can expose the helper (and call it data, IDK) and avoid the need to wrap every getter in the helper as part of the transform code.
Not sure, I'm still thinking about it, this is a good chance to see if the design is extendable enough as this PR adds a "third" transform basically.

kubapelc · 2024-12-02T09:23:30Z

Is it possible to get a fallbackMatrix without the code from mercator transform?

I think it makes sense to query mercator transfrom to get the fallback matrix. since it is the exact matrix which is used when mercator is active. Globe mostly needs it to do the projection transition.

There is also the same question for prefetchTiles and a few more other places where I couldn't find a proper solution.

I think precacheTiles is ok in its current state, since there is no equivalent for globe transform. Mercator needs to compute a separate matrix for each tile, globe does not. (But I'm honestly not sure this precaching step even helps performance, I think maplibre did do it originally, but it was a side effect of some other function which was obsoleted and removed by the globe changes, iirc., and so I left it in.)

I'll go over all the mercator transform usages.

I'm also thinking about splitting the camera helper the same way I did for the globe transform

Sounds good!

kubapelc · 2024-12-02T09:35:53Z

src/geo/projection/globe_transform.ts

    }

    public getCircleRadiusCorrection(): number {
-        return lerp(this._mercatorTransform.getCircleRadiusCorrection(), Math.cos(this.getAnimatedLatitude() * Math.PI / 180), this._globeness);
+        // HM TODO: there was a "double" interpolation here which was removed. Check if it's needed.
+        return lerp(this._mercatorTransform.getCircleRadiusCorrection(), this._verticalPerspectiveTransform.getCircleRadiusCorrection(), this._globeness);


The double interpolation here was correct. The function getAnimatedLatitude() would only have an effect on latitude if the globe was moved such that the map center was very close to mercator range around +-85°, because globe allows the user to pan much closer to poles than mercator, so when transitioning to mercator the latitude is smoothly interpolated to the max latitude mercator allows.

So in this case we interpolate circle correction from globe value to mercator value, but at the same time we also interpolate globe's latitude (also affecting the globe's circle correction), hence the double interpolation.

kubapelc · 2024-12-02T09:37:49Z

I went over all mercator transform usages in the current globe_transform.ts, everything looks good, I only added a comment about the removed double interpolation.

ibesora

I like where this is going, nice work!
Added some comments but feel free to ignore them (except for the typo ones!)

ibesora · 2024-12-02T09:16:40Z

src/geo/projection/globe_transform.ts

        this._projectionInstance = globeProjection;
        this._mercatorTransform = new MercatorTransform();
+        this._verticalPerspectiveTransform = new VeritcalPerspectiveTransform();


VeritcalPerspectiveTransform contains a typo. Should be VerticalPerspectiveTransform

ibesora · 2024-12-02T09:25:33Z

src/geo/projection/globe_transform.ts

-        // Globe uses the same cameraToCenterDistance as mercator.
-        return this._mercatorTransform.cameraToCenterDistance;
-    }
+    public get cameraPosition(): vec3 { return this.isGlobeRendering ? this._verticalPerspectiveTransform.cameraPosition : this._mercatorTransform.cameraPosition; }


Instead of having this this.isGlobeRendering ? ... : ... repeated in every function, can we maybe go a further step abstracting and have something like this._actualTransform that changes between this._verticalPerspectiveTransform and this._mercatorTransform depending on the value of this.isGlobeRendering?

ibesora · 2024-12-02T09:27:05Z

src/geo/projection/globe_transform.ts

-            data.mainMatrix = this._globeViewProjMatrix32f;
-        }
+        const mercatorProjectionData = this._mercatorTransform.getProjectionData(params);
+        const vertialPerspectiveProjectionData = this._verticalPerspectiveTransform.getProjectionData(params);


Typo on vertical again

ibesora · 2024-12-02T09:36:27Z

src/geo/projection/globe_transform.ts

    }

    public getCircleRadiusCorrection(): number {
-        return lerp(this._mercatorTransform.getCircleRadiusCorrection(), Math.cos(this.getAnimatedLatitude() * Math.PI / 180), this._globeness);
+        // HM TODO: there was a "double" interpolation here which was removed. Check if it's needed.


I'd say so. The center latitude changes a little bit between mercator and globe transforms.
Without the other lerp we are just using one of them, although in practice it should produce similar results

ibesora · 2024-12-02T09:45:45Z

src/geo/projection/vertical_perspective_transform.ts

+    tMax: number;
+} | null;
+
+export class VeritcalPerspectiveTransform implements ITransform {


I think this is where most of the typos are coming from

Initial commit to move the logic of transition out to a new file

616dac1

HarelM requested review from kubapelc, ibesora and birkskyum December 1, 2024 15:55

HarelM added 2 commits December 1, 2024 17:55

Merge branch 'main' into projection-expression

430ffce

Remove one line method

e6419f1

birkskyum reviewed Dec 1, 2024

View reviewed changes

src/util/util.ts Show resolved Hide resolved

Fix build test

4acc6ed

HarelM mentioned this pull request Dec 1, 2024

Refactor globe transfrom #5140

Draft

8 tasks

HarelM added 2 commits December 1, 2024 22:47

Fix projection data bad refactoring

b975432

Fix render and size tests

8a10085

HarelM added 2 commits December 1, 2024 23:37

Fix failing unit test

2995ed7

Uncomment unit tests

e1744f8

Merge branch 'main' into projection-expression

94019c3

kubapelc reviewed Dec 2, 2024

View reviewed changes

ibesora reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Facilitate for globe transition expression refactoring #5139

Facilitate for globe transition expression refactoring #5139

HarelM commented Dec 1, 2024

codecov bot commented Dec 1, 2024 •

edited

Loading

birkskyum commented Dec 1, 2024 •

edited

Loading

HarelM commented Dec 1, 2024

HarelM commented Dec 1, 2024 •

edited

Loading

kubapelc commented Dec 2, 2024

HarelM commented Dec 2, 2024

kubapelc Dec 2, 2024

HarelM Dec 2, 2024

kubapelc Dec 2, 2024

HarelM Dec 2, 2024 •

edited

Loading

kubapelc commented Dec 2, 2024

kubapelc Dec 2, 2024 •

edited

Loading

kubapelc commented Dec 2, 2024

ibesora left a comment

ibesora Dec 2, 2024

ibesora Dec 2, 2024

ibesora Dec 2, 2024

ibesora Dec 2, 2024

ibesora Dec 2, 2024

Facilitate for globe transition expression refactoring #5139

Are you sure you want to change the base?

Facilitate for globe transition expression refactoring #5139

Conversation

HarelM commented Dec 1, 2024

Launch Checklist

codecov bot commented Dec 1, 2024 • edited Loading

Codecov Report

birkskyum commented Dec 1, 2024 • edited Loading

HarelM commented Dec 1, 2024

HarelM commented Dec 1, 2024 • edited Loading

kubapelc commented Dec 2, 2024

HarelM commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

kubapelc commented Dec 2, 2024

kubapelc Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

kubapelc commented Dec 2, 2024

ibesora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2024 •

edited

Loading

birkskyum commented Dec 1, 2024 •

edited

Loading

HarelM commented Dec 1, 2024 •

edited

Loading

HarelM Dec 2, 2024 •

edited

Loading

kubapelc Dec 2, 2024 •

edited

Loading