Expose projection matrix parameters #3136

birkskyum · 2023-09-28T23:50:35Z

Closes #3080, closes #4044

@Pessimistress , is this what you had in mind?

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.

codecov-commenter · 2023-09-28T23:54:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.18%. Comparing base (3d0f0db) to head (b0ceae8).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3136      +/-   ##
==========================================
- Coverage   86.61%   86.18%   -0.43%     
==========================================
  Files         241      241              
  Lines       32518    32520       +2     
  Branches     2145     2035     -110     
==========================================
- Hits        28165    28027     -138     
- Misses       3395     3509     +114     
- Partials      958      984      +26

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

HarelM · 2023-09-29T05:27:45Z

The code looks good although I'm not super excited about sending both the transform and one of its properties, but I don't want to break this API so I guess this is the best option.
I'll wait for @Pessimistress to review as if this doesn't solve her problem I would avoid merging this.

HarelM · 2024-01-23T21:07:39Z

Any updates on this?
We are nearing the release of version 4.x.
If a breaking change is needed, now might be the time.
If this is not a real requirement, let's close this PR...

Pessimistress · 2024-03-27T17:10:18Z

Yes, it looks perfect!

birkskyum · 2024-04-27T11:19:36Z

@HarelM , I've rebased this

birkskyum · 2024-04-27T11:35:52Z

It's not a breaking change as far as I can tell.

birkskyum · 2024-04-27T11:37:34Z

Some docs generation appear to be breaking, but all other tests are passing.

birkskyum · 2024-04-27T11:45:04Z

I'm not super excited about sending both the transform and one of its properties

I agree with this, there could maybe be more elegant way. All i know is that we do a lot with the viewport right now, with globe, terrain3d, threejs, deck, babylon etc., so we really need some way to get this info out and this is the simplest non-breaking path I can think of.

HarelM · 2024-04-27T12:13:54Z

What I had in mind is changing the input for the custom layer to be an object, this way there's no problem to add more optional parameters and there no order issue if you want to use only one or two parameters, but this is a breaking change and will have to wait for version 5.x if we want to introduce it.

birkskyum · 2024-05-08T18:56:54Z

We discussed in the TSC to include this third transform parameter in the 4.x cycle as a temporary measure:

type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4, transform: Transform) => void;

and then from v5 do a breaking change to have the CustomRenderMethod take a single options object instead of these three arguments, so that it's easier to maintain it going forward.

HarelM · 2024-05-08T19:06:23Z

The issue which I remembered was the following that is talking about different matrices for 2D and 3D:

Custom Layer Interface is missing a mercatorMatrix3d #3797

CC: @olsen232

HarelM · 2024-05-08T19:13:22Z

It's also worth mentioning that making the Transform class a public API kind of class might be complicated in terms of keeping it backwards compatible (it's a very big class which we have thoughts about splitting it).
I would advise to try and see if we can narrow down the parameters that we pass as part of this API - for example - farZ, nearZ, and projectionMatrix - this way we don't expose the entire transform class and backwards compatibility is easier.
We can also start the object that we are planning for version 5 with these parameters only and add the other parameters to it in version 5, I hope this makes sense...

Pessimistress · 2024-05-08T19:28:00Z

If we know what the parameter will look like (or at least part of it) in v5, I would much rather have that object as the 3rd parameter in v4 than the Transform instance. It would be a lot easier to implement backward compatibility if v4->v5 the only change is the 2nd (matrix) parameter getting dropped.

birkskyum · 2024-05-08T20:19:28Z

Another curveball with exposing Transform to the public api is also that it requires removing lots of @interal for the docs gen (Transform itself, and everything it reference recurrently). Arguably a lot more than we want.

birkskyum · 2024-05-08T20:30:35Z

I've added an args object, with nearZ, farZ, projMatrix to the method instead of the transform.

This reverts commit a00fce3.

birkskyum · 2024-05-08T20:46:58Z

@Pessimistress , would it be sufficient having just these projMatrix, farZ, nearZ?

Also, I don't know the answer to this question from @dennemark if the projection matrix on Transform isn't the one we want, because it has somehow already been altered.

olsen232 · 2024-05-08T21:33:22Z

Regarding #3797 -
Since I wrote up that issue I've become a bit more familiar with MapLibre, I would write it differently now.
What I wrote was mostly true but perhaps not quite. Here's my current take.

There is a matrix called mercatorMatrix, which is passed to custom layers, and is the only matrix they have available.
This matrix is perfectly adequate in a context without 3D terrain.
This matrix has an unhelpful and changing Z offset in a context with 3D terrain, causing custom layers to apparently jump up and down (which surprises custom layer authors like me)
This is actually by design and custom layer authors are supposed to compensate for this weird Z offset by using map.queryTerrainElevation - the returned values from this contain the compensation for the wrong Z offset (which surprises clients of queryTerrainElevation).
Both of these surprises are fixed by Return above-sea-level elevation from queryTerrainElevation + Pass lowered mercator matrix to custom layer #3854 - it removes the weird offsets from both at the same time. It's a behavior change so might be for major-release only.

And here's my most relevant new understanding:

In a context with 3D terrain, the fixed matrix (as in Return above-sea-level elevation from queryTerrainElevation + Pass lowered mercator matrix to custom layer #3854) is the only one a custom layer author would need.
In a context without 3D terrain, "fixing" the matrix in this way doesn't do anything, so the old and new approach are identical in this context anyway. So again, in that context, there's only one matrix a custom layer author would need (which is the same as the one we have right now).

This means, there's no need to provide custom layer authors two matrices for them to choose between - one would be sufficient. However, for backwards compatibility reasons, it might be useful to keep providing the old matrix in some way, even though it is (IMO) strictly less useful than the fixed matrix (again, see #3854)

dennemark · 2024-05-09T00:26:46Z

Hi,
Thanks for the elaboration!
When you talk about 3D terrain custom layer only needing the mercator matrix, you mean the world-view-projection matrix? That is the one being used in threejs and babylonjs examples.
But as i stated in the second referenced issue, it is much better to supply a projection matrix to i.e. babylonjs and place its camera according to the maplibre camera matrix, since then shaders work with a correct view matrix. So while it is possible to use only one matrix, it is more profitable to expose the underlying projection matrix, too.

Did not look at the code yer, since i am not so flexible and only on mobile right now..

dennemark · 2024-05-09T00:59:19Z

I had a look at the code. It is missing the exposure of projection matrix so far. In my issue i propose where it should be done. But the naming is not clear yet.
I also think that this issue does not deal with projection matrix currently being the wrong name, since the matrix rather represents the wvp-matrix as far as i could see in the code.
If it is possible, it would be nice to add this. Even if it is a private property like _nearZ and _farZ.

More on the naming in my issue.
#4044

olsen232 · 2024-05-09T01:07:10Z

What I wrote above and in the issue has little relevance to your idea that different types of matrix could be useful.
I was concerned that the provided mercatorMatrix had unhelpful z-offsets - I wasn't sure if it should be supplemented or replaced by one with more helpful Z-offsets - I knew that I needed a different matrix that had been fixed, but I thought in some other context, the original one might be more useful. I now believe that we can just fix it for everyone as in #3854 .
But in all cases, the matrix I'm talking about in is the one that, at least in theory, transforms world-space to clip space.
https://learnopengl.com/Getting-started/Coordinate-Systems
(I don't know what the proper names of matrices should be except for what I just read in that website).

However, you are talking about having other types of matrices also available, that do different types of transforms. I can't really comment on that, it could well be that custom layer authors would make use of different types of matrices sometimes.

HarelM · 2024-05-09T05:41:25Z

src/style/style_layer/custom_style_layer.ts

 */
-type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4) => void;
+type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4, args: { farZ: number; nearZ: number; projMatrix: mat4 }) => void;


Please create a type for this and add docs to it describing each parameter since this is a public API, something with a name similar to CustomLayerInput or CustomLayerOptions.
This is the type we will expand in the future.

HarelM · 2024-05-09T05:42:32Z

src/geo/transform.ts

@@ -58,6 +58,8 @@ export class Transform {
    _constraining: boolean;
    _posMatrixCache: {[_: string]: mat4};
    _alignedPosMatrixCache: {[_: string]: mat4};
+    _farZ: number;


These are basically public, I would consider removing the "_" and add doc comments describing what they do.

HarelM · 2024-05-09T05:45:32Z

@olsen232 @dennemark @Pessimistress please review this change and let us know if it needs anything else, besides my nitpicking, I think this is good to go.

Pessimistress · 2024-05-09T14:36:18Z

src/style/style_layer/custom_style_layer.ts

 */
-type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4) => void;
+type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4, args: { farZ: number; nearZ: number; projMatrix: mat4 }) => void;


One other parameter that would be really useful is the z of the camera target, either in pixels or meters above sea level.

Right now we are accessing it via map.transform.elevation, but I won't be surprised if it changes in the future. Mapbox has some complicated logic to make a smoother panning experience, such that the camera doesn't always point to the surface of the terrain.

See https://github.com/visgl/deck.gl/blob/86f3f84f44a4b456042481b51d519554967b3837/modules/mapbox/src/deck-utils.ts#L212

dennemark · 2024-05-14T14:24:54Z

Hi,

concerning my issue, I think it could be handled in this or a new PR. Choose what you prefer.

We would have two options in my opinion in exposing the actual projection Matrix. As @olsen232 explained, he used a matrix to transform from world space to clip space ( https://learnopengl.com/Getting-started/Coordinate-Systems ). This matrix currently is named this.projMatrix as far as I understand. But looking at the link it actually is the multiplied view matrix and projection matrix.

Naming wise the cleanest approach would be the following:

Rename this.projMatrix to this.viewProjectionMatrix
Separate view and projection matrix and expose them as this.viewMatrix and this.projMatrix
But this would cause a breaking change for users who use the current this.projMatrix

An alternative would be, to expose
this.projectionMatrix, this.viewMatrix and this.viewProjectionMatrix
and mark this.projMatrix as deprecated. This would be backwards compatible.
I think the same goes for this.invProjMatrix

I could try to implement this and replace all occurrences with the new name.

HarelM · 2024-05-14T16:05:10Z

This PR is not about the names in the transform class but rather adding parameters to the custom layer interface's render method.
Since almost none of these matrices are exposed, and we want to introduce a new object for the API, we can get the names right from the first go.
@dennemark check out the changed files in this PR to get a better understanding of the change suggested here.

dennemark · 2024-05-14T16:17:22Z

This PR is not about the names in the transform class but rather adding parameters to the custom layer interface's render method. Since almost none of these matrices are exposed, and we want to introduce a new object for the API, we can get the names right from the first go. @dennemark check out the changed files in this PR to get a better understanding of the change suggested here.

added a review. i think this should clarify the exposure of the correct parameters.

HarelM · 2024-05-15T08:05:22Z

added a review

I don't see it, did you forget to publish it?

dennemark · 2024-05-15T09:07:31Z

added a review

I don't see it, did you forget to publish it?

I see it just above my post. You don`t ? It says it is pending.

HarelM · 2024-05-15T09:11:06Z

If it's pending it means it locally for your account only. You need to submit it.

dennemark · 2024-05-14T16:16:02Z

src/render/draw_custom.ts

@@ -36,7 +36,7 @@ export function drawCustom(painter: Painter, sourceCache: SourceCache, layer: Cu

        context.setDepthMode(depthMode);

-        implementation.render(context.gl, painter.transform.customLayerMatrix());
+        implementation.render(context.gl, painter.transform.customLayerMatrix(), {farZ: painter.transform._farZ, nearZ: painter.transform._nearZ, projMatrix: painter.transform.projMatrix});


Relating to #3136 (comment)

If we expose the parameters like this, we will continue the wrong naming of matrices in the transform in the custom layer function, too. We should therefore avoid using the transform.projMatrix here.

I would propose exposing more matrices in transform like this: this.projectionMatrix, this.viewMatrix and this.viewProjectionMatrix.
Then we can expose them for the custom layer render function like this:
implementation.render(context.gl, painter.transform.customLayerMatrix(), {farZ: painter.transform._farZ, nearZ: painter.transform._nearZ, projectionMatrix: painter.transform.projectionMatrix, viewMatrix painter.transform.viewMatrix, viewProjectionMatrix: painter.transform.projMatrix});

Please consider, that I did not change the naming of projMatrix in this case, but just added another projectionMatrix. We could add docs to the variables for clarification.

/** * projMatrix represents the matrix converting from world space to clip space * https://learnopengl.com/Getting-started/Coordinate-Systems */ projMatrix mat4; /** * viewMatrix represents the matrix converting from world space to view space * https://learnopengl.com/Getting-started/Coordinate-Systems */ viewMatrix mat4; /** * projectionMatrix represents the matrix converting from view space to clip space * https://learnopengl.com/Getting-started/Coordinate-Systems */ projectionMatrix mat4;

I would recommend being even more specific since projMatrix and projectionMatrix are the same name basically:
worldToViewProjectionMatrix, viewToClipProjectionMatrix and keep the projMatrix name in transform, but not in the custom layer interface and change that name in version 5 to align the code (although transform is not a real publicly facing class/API, I prefer to change "public" fields name in it only when chaning a major version just to be on the safe side).

I agree, that is why I thought for backwards compatibility we could set a deprecation mark on projMatrix and already expose the new correct name.
How about adding the following variables to transform and then exposing them in the implementation.render function?

/** * @deprecated in version 5 this variable will be accessible via worldToClipProjectionMatrix * projMatrix represents the matrix converting from world space to clip space * https://learnopengl.com/Getting-started/Coordinate-Systems */ projMatrix mat4; /** * worldToClipProjectionMatrix represents the matrix converting from world space to clip space * https://learnopengl.com/Getting-started/Coordinate-Systems */ worldToClipProjectionMatrix mat4; /** * worldToViewMatrix represents the matrix converting from world space to view space * https://learnopengl.com/Getting-started/Coordinate-Systems */ worldToViewMatrix mat4; /** * viewToClipProjectionMatrix represents the matrix converting from view space to clip space * https://learnopengl.com/Getting-started/Coordinate-Systems */ viewToClipProjectionMatrix mat4;

@Pessimistress @birkskyum @olsen232 any comments on the above suggestion?
I think it's a good idea.

viewMatrix, projectionMatrix and viewProjectionMatrix have very well-established meanings in 3D programming. I do not think you need to reinvent new names - that will be somehow be more confusing.

I agree that the current projMatrix is not a good name. It should simply be kept internal.

projectionMatrix describes properties of the camera itself, such as fov, near, far, aspect ratio.
viewMatrix applies the position and orientation of the camera, i.e. center, pitch, bearing, zoom.
modelMatrix applies any adjustment to the world coordinates including terrain offset, Mercator pixel scaling etc.
The existing projMatrix is actually modelViewProjectionMatrix which is the multiplication of the above.

In the code of transform currently there is no separate projectionMatrix viewMatrixand modelMatrix. Rather step by step all transforms are applied to the matrix m. So the projectionMatrix could be easily set after this line:

maplibre-gl-js/src/geo/transform.ts

Line 890 in 3f481c8

m[9] = offset.y * 2 / this.height;

But the viewMatrix and modelMatrix are not separated yet. For me it would be enough to get the projectionMatrix. If we want to expose the other two, we would have to create them indiviudally and afterwards apply them to the m matrix.
Naming wise shouldnt the projMatrixbe the modelViewProjectionMatrix` ? Since the projection is applied to it, too.

You are right, edited.

I will be happy with just a projectionMatrix too, plus elevation. The view matrix is fairly straight forward to construct since center, pitch, zoom etc are well documented.

dennemark · 2024-05-15T09:15:13Z

If it's pending it means it locally for your account only. You need to submit it.

sorry :/

HarelM added the waiting for code review fixes label Jan 23, 2024

birkskyum mentioned this pull request Apr 27, 2024

Add the actual projection matrix to transform #4044

Open

birkskyum force-pushed the expose-projection-matrix-params branch from debee89 to 9dadb77 Compare April 27, 2024 11:32

birkskyum requested a review from HarelM April 27, 2024 11:42

birkskyum mentioned this pull request May 8, 2024

Breaking changes MapLibre GL JS v5 #3834

Open

birkskyum added 2 commits May 8, 2024 22:16

Expose projection matrix parameters

d657101

Remove "Internal" from classes

a00fce3

birkskyum force-pushed the expose-projection-matrix-params branch from aa65413 to a00fce3 Compare May 8, 2024 20:16

birkskyum force-pushed the expose-projection-matrix-params branch from 3ce2ce0 to a00fce3 Compare May 8, 2024 20:20

put transform inside args object

6ace547

birkskyum added 4 commits May 8, 2024 22:34

lint

1db35f4

expose only nearZ, farZ, projMatrix

998660a

Revert "Remove "Internal" from classes"

a167a88

This reverts commit a00fce3.

revert changes

687798d

birkskyum added 3 commits May 8, 2024 22:47

lint

bdfd47e

lint

41ed148

docs

b0ceae8

HarelM reviewed May 9, 2024

View reviewed changes

Pessimistress reviewed May 9, 2024

View reviewed changes

dennemark reviewed May 15, 2024

View reviewed changes

Expose projection matrix parameters #3136

Are you sure you want to change the base?

Expose projection matrix parameters #3136

Conversation

birkskyum commented Sep 28, 2023 • edited

Launch Checklist

codecov-commenter commented Sep 28, 2023 • edited

Codecov Report

HarelM commented Sep 29, 2023

HarelM commented Jan 23, 2024

Pessimistress commented Mar 27, 2024

birkskyum commented Apr 27, 2024 • edited

birkskyum commented Apr 27, 2024

birkskyum commented Apr 27, 2024 • edited

birkskyum commented Apr 27, 2024 • edited

HarelM commented Apr 27, 2024

birkskyum commented May 8, 2024 • edited

HarelM commented May 8, 2024

HarelM commented May 8, 2024

Pessimistress commented May 8, 2024

birkskyum commented May 8, 2024 • edited

birkskyum commented May 8, 2024 • edited

birkskyum commented May 8, 2024 • edited

olsen232 commented May 8, 2024

dennemark commented May 9, 2024

dennemark commented May 9, 2024

olsen232 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented May 9, 2024

Choose a reason for hiding this comment

dennemark commented May 14, 2024

HarelM commented May 14, 2024

dennemark commented May 14, 2024

HarelM commented May 15, 2024

dennemark commented May 15, 2024

HarelM commented May 15, 2024

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

Pessimistress May 15, 2024 • edited

Choose a reason for hiding this comment

Pessimistress May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennemark commented May 15, 2024

birkskyum commented Sep 28, 2023 •

edited

codecov-commenter commented Sep 28, 2023 •

edited

birkskyum commented Apr 27, 2024 •

edited

birkskyum commented Apr 27, 2024 •

edited

birkskyum commented Apr 27, 2024 •

edited

birkskyum commented May 8, 2024 •

edited

birkskyum commented May 8, 2024 •

edited

birkskyum commented May 8, 2024 •

edited

birkskyum commented May 8, 2024 •

edited

Pessimistress May 15, 2024 •

edited

Pessimistress May 15, 2024 •

edited