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

Custom Layer Interface is missing a mercatorMatrix3d #3797

Open
olsen232 opened this issue Mar 6, 2024 · 8 comments
Open

Custom Layer Interface is missing a mercatorMatrix3d #3797

olsen232 opened this issue Mar 6, 2024 · 8 comments
Labels
need more info Further information is requested

Comments

@olsen232
Copy link
Contributor

olsen232 commented Mar 6, 2024

The Custom Layer Interface documentation gives an example of how to make a custom 2D layer.
It documents that you can choose whether the layer is 2D or 3D - but the effects of that choice are quite limited, possibly only enabling or disabling the depth buffer.
Different transformational matrices are used for 2D and 3D rendering, but the Custom Layer Interface is always provided with a matrix called mercatorMatrix. This matrix is appropriate, I believe, for 2D rendering, but not for 3D rendering, since it is missing one of the translate steps that the matrices used for 3D rendering have. Specifically, it doesn't have this line of code, or any equivalent:
mat4.translate(m, m, [0, 0, -this.elevation]); // elevate camera over terrain

Because it is missing this translation - which is not always the same amount, and depends in some way on the terrain height at the center of the camera - using this matrix to render a point in a 3D context causes the point to appear too high, by a small amount when the camera-center-terrain-height is near sea level, and by a large amount when the camera-center-terrain-height is high altitude.

maplibre-gl-js version:
4.1.0 or main @ 3345713

browser:
Brave, but this logic is not browser dependent.

Steps to Trigger Behavior

Use a CustomLayerInterface to render points / objects in a 3D view, with terrain enabled, in a place with hilly terrain (effects of the bug will be minimal if the terrain is close to sea level).

Link to Demonstration

#3796 - which is still a draft - shows the issue (the red point is too high), and shows a basic fix (the green point is in the correct location). To try it out, checkout these changes, run build-dev, and open test/examples/custom-layer-interface-bug.html
Moving the view around will cause the red dot's height to jump to some other value whenever recalculateZoom is called, which happens at the end of each panning movement.

Here is a screenshot of the issue as shown by the #3796
Screenshot 2024-03-06 at 10 47 25 AM

Note that the green point has the right position, this is possible since I've modified src/geo/transform.ts to expose another matrix called mercatorMatrix3D.

Expected Behavior

Actual Behavior

  • Red point is in the wrong place.
  • No matrix provided by CustomLayerInterface to get the red point in the right place.
  • Documentation doesn't explain how to get the red point in the right place when rendering in 3D.
@olsen232
Copy link
Contributor Author

olsen232 commented Mar 6, 2024

Here is a flowchart of how existing matrices are calculated:

flowchart TD
    S[START] --> |scale by 1, 1, _pixelPerMeter| T[temp]
    S --> |scale by worldSize, worldSize, worldSize| MM[mercatorMatrix]
    T --> |translate by 0, 0, -elevation| PR[projMatrix]
    T --> |multiply by labelPlaneMatrix| PI[pixelMatrix]
    PR --> |multiply by labelPlaneMatrix| PI3[pixelMatrix3D]
    
Loading

And here is a table of which matrices exist, and what is missing:

2D 3D
mercator-coords to clip space mercatorMatrix (missing)
world space to screen space pixelMatrix pixelMatrix3D

The description of these matrices is from the comments in the code - to be honest I'm still a bit vague on the details eg are mercator-coords the same as world-space (I believe they are very similar but they might not be exactly the same). Hopefully, someone familiar with the MapLibre matrices can weigh in on what the appropriate way to construct a mercatorMatrix3D is, again it will be similar to what I have done but perhaps not exactly the same.

@HarelM
Copy link
Member

HarelM commented Mar 6, 2024

Can you try use mermaid to draw the diagrams?
Sorry for the nit picking...

@HarelM
Copy link
Member

HarelM commented Mar 6, 2024

Maybe @maxammann can shred some light, I always find these matrices confusing...

@HarelM
Copy link
Member

HarelM commented Mar 6, 2024

Also, there's a problem with the custom layer interface as there seems to be a lot of different requirements to enhance it, but they don't "scale" well, i.e. adding more parameters to the method is not a good approach, I guess an input object should be used and thus allowing it to grow without breaking the interface...
Having said that, I personally don't have an experience with writing a custom layer and the examples is the docs are probably "too" basic...

@olsen232
Copy link
Contributor Author

olsen232 commented Mar 13, 2024

This is still waiting on a ML matrices / CustomLayer expert to weigh in on what an appropriate fix would be.

In the meantime, if you are experiencing this issue and you want to work around it, you can do the following in your Custom Layer, as I have:
Example code - from https://maplibre.org/maplibre-gl-js/docs/API/interfaces/CustomLayerInterface/

    render(gl, matrix) {
        gl.useProgram(this.program);
        gl.uniformMatrix4fv(gl.getUniformLocation(this.program, "u_matrix"), false, matrix);
        gl.drawArrays(gl.POINTS, 0, 1);
    }

should be changed to the following:

+const mercatorMatrix3D = new Array(16) as mat4;
... 
     render(gl, matrix) {
+        // This workaround can go away once there is a proper fix for https://github.com/maplibre/maplibre-gl-js/issues/3797
+        const tr = map.transform;
+        const zOffset = (-tr.elevation * tr._pixelPerMeter) / tr.worldSize;
+        mat4.translate(mercatorMatrix3D, matrix, [0, 0, zOffset]);
         
         gl.useProgram(this.program);
-        gl.uniformMatrix4fv(gl.getUniformLocation(this.program, "u_matrix"), false, matrix);
+        gl.uniformMatrix4fv(gl.getUniformLocation(this.program, "u_matrix"), false, mercatorMatrix3D);
         gl.drawArrays(gl.POINTS, 0, 1);
     }

@olsen232
Copy link
Contributor Author

This seems to have stalled - I've diagnosed it and found a possible fix, but somebody in ML will have to decide what is appropriate here:

  • should the matrix provided to a CustomLayer always be the fixed one that I've labelled mercatorMatrix3D?
  • should it sometimes be modified in this way, depending on whether the client chooses 2D or 3D?
  • should the client be able to access either matrix, and if so, how will that fit into the interface?

@prozessor13 maybe you can help?

@olsen232
Copy link
Contributor Author

This would be fixed by #3854, which someone is already working on, so that is now my preferred fix

@olsen232
Copy link
Contributor Author

olsen232 commented May 9, 2024

(I wrote a followup on this issue in the other discussion - copying it here for completeness.
The summary of it all is the same as above: #3854 is a good fix IMO.)

Since I wrote up this issue I've become a bit more familiar with MapLibre, I would write it differently now.
What I wrote above is 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.

So to sum up:

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants