-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Expose projection matrix parameters #3136
base: main
Are you sure you want to change the base?
Expose projection matrix parameters #3136
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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. |
Any updates on this? |
Yes, it looks perfect! |
@HarelM , I've rebased this |
debee89
to
9dadb77
Compare
It's not a breaking change as far as I can tell. |
Some docs generation appear to be breaking, but all other tests are passing. |
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. |
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. |
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. |
The issue which I remembered was the following that is talking about different matrices for 2D and 3D: CC: @olsen232 |
It's also worth mentioning that making the |
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. |
aa65413
to
a00fce3
Compare
Another curveball with exposing Transform to the public api is also that it requires removing lots of |
3ce2ce0
to
a00fce3
Compare
I've added an args object, with nearZ, farZ, projMatrix to the method instead of the transform. |
@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. |
Regarding #3797 -
And here's my most relevant new understanding:
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) |
Hi, Did not look at the code yer, since i am not so flexible and only on mobile right now.. |
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. More on the naming in my issue. |
What I wrote above and in the issue has little relevance to your idea that different types of matrix could be useful. 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. |
*/ | ||
type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4) => void; | ||
type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4, args: { farZ: number; nearZ: number; projMatrix: mat4 }) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -58,6 +58,8 @@ export class Transform { | |||
_constraining: boolean; | |||
_posMatrixCache: {[_: string]: mat4}; | |||
_alignedPosMatrixCache: {[_: string]: mat4}; | |||
_farZ: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are basically public, I would consider removing the "_" and add doc comments describing what they do.
@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. |
*/ | ||
type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4) => void; | ||
type CustomRenderMethod = (gl: WebGLRenderingContext|WebGL2RenderingContext, matrix: mat4, args: { farZ: number; nearZ: number; projMatrix: mat4 }) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 Naming wise the cleanest approach would be the following:
An alternative would be, to expose I could try to implement this and replace all occurrences with the new name. |
This PR is not about the names in the transform class but rather adding parameters to the custom layer interface's render method. |
added a review. i think this should clarify the exposure of the correct parameters. |
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. |
If it's pending it means it locally for your account only. You need to submit it. |
@@ -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}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pessimistress @birkskyum @olsen232 any comments on the above suggestion?
I think it's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code of transform currently there is no separate projectionMatrix
viewMatrix
and 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 shouldn
t the
projMatrixbe the
modelViewProjectionMatrix` ? Since the projection is applied to it, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
sorry :/ |
Closes #3080, closes #4044
@Pessimistress , is this what you had in mind?
Launch Checklist
CHANGELOG.md
under the## main
section.