Improvements to marker transparency (#3431)

* Marker switching back to fully opaque when removing terrain * Removing log * Fix lint * Removing console logs * Marker visibility depending on depth buffer check, smooth marker transition * Adding unit test * Updating Changelog, fixing test * Fix test * Adding check for center of marker if base fails * Fix unit test * Removing console.log * Fix another unit test * Clarifying clip space * Phrasing: gl space to clip space * Reverting change to near plane, adjusting forgiveness * Adding unit test for lngLatToCameraDepth * Increasing timeout to prevent flaking test * Increasing timeout more * Increasing timeout for another flaky test * Reverting attempted fix to flaky tests * Changing comments to tsdoc style * Don't add opacity: 1 with no terrain * Improving marker test coverage
maplibre · Dec 6, 2023 · 7e45696 · 7e45696
1 parent bd6780a
commit 7e45696
Show file tree

Hide file tree

Showing 11 changed files with 161 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,14 @@
 ## main
 
 ### ✨ Features and improvements
+
+- Improved precision and added a subtle fade transition to marker opacity changes ([#3431](https://github.com/maplibre/maplibre-gl-js/pull/3431))
 - _...Add new stuff here..._
 
 ### 🐞 Bug fixes
 
 - Fix the shifted mouse events after a css transform scale on the map container ([#3437](https://github.com/maplibre/maplibre-gl-js/pull/3437))
+- Fix markers remaining transparent when disabling terrain ([#3431](https://github.com/maplibre/maplibre-gl-js/pull/3431))
 - _...Add new stuff here..._
 
 ## 4.0.0-pre.2
@@ -51,6 +54,7 @@
 ## 3.6.1
 
 ### 🐞 Bug fixes
+
 - Fix `undefined` `_onEaseFrame` call in `Camera._renderFrameCallback()` while doing `Camera.jumpTo` during a `Camera.easeTo` ([#3332](https://github.com/maplibre/maplibre-gl-js/pull/3332))
 
 ## 3.6.0

diff --git a/src/css/maplibre-gl.css b/src/css/maplibre-gl.css
@@ -711,6 +711,7 @@ a.maplibregl-ctrl-logo.maplibregl-compact {
     top: 0;
     left: 0;
     will-change: transform;
+    transition: opacity 0.2s;
 }
 
 .maplibregl-user-location-dot {

diff --git a/src/geo/transform.test.ts b/src/geo/transform.test.ts
@@ -395,10 +395,10 @@ describe('transform', () => {
 
         expect(transform.customLayerMatrix()[0].toString().length).toBeGreaterThan(10);
         expect(transform.glCoordMatrix[0].toString().length).toBeGreaterThan(10);
-        expect(transform.maxPitchScaleFactor()).toBeCloseTo(2.366025418080343, 10);
+        expect(transform.maxPitchScaleFactor()).toBeCloseTo(2.366025418080343, 5);
     });
 
-    test('recalcuateZoom', () => {
+    test('recalculateZoom', () => {
         const transform = new Transform(0, 22, 0, 60, true);
         transform.elevation = 200;
         transform.center = new LngLat(10.0, 50.0);
@@ -416,16 +416,16 @@ describe('transform', () => {
         // expect new zoom because of elevation change
         terrain.getElevationForLngLatZoom = () => 400;
         transform.recalculateZoom(terrain as any);
-        expect(transform.zoom).toBe(14.127997275621933);
+        expect(transform.zoom).toBeCloseTo(14.127997275621933, 10);
         expect(transform.elevation).toBe(400);
 
-        expect(transform._center.lng).toBe(10.00000000000071);
-        expect(transform._center.lat).toBe(50.00000000000017);
+        expect(transform._center.lng).toBeCloseTo(10, 10);
+        expect(transform._center.lat).toBeCloseTo(50, 10);
 
         // expect new zoom because of elevation change to point below sea level
         terrain.getElevationForLngLatZoom = () => -200;
         transform.recalculateZoom(terrain as any);
-        expect(transform.zoom).toBe(13.773740316343467);
+        expect(transform.zoom).toBeCloseTo(13.773740316343467, 10);
         expect(transform.elevation).toBe(-200);
     });
 
@@ -461,4 +461,15 @@ describe('transform', () => {
         expect(top).toBeCloseTo(79.1823898251593, 10);
         expect(transform.getBounds().getNorthWest().toArray()).toStrictEqual(transform.pointLocation(new Point(0, top)).toArray());
     });
+
+    test('lngLatToCameraDepth', () => {
+        const transform = new Transform(0, 22, 0, 85, true);
+        transform.resize(500, 500);
+        transform.center = new LngLat(10.0, 50.0);
+
+        expect(transform.lngLatToCameraDepth(new LngLat(10, 50), 4)).toBeCloseTo(0.9997324396231673);
+        transform.pitch = 60;
+        expect(transform.lngLatToCameraDepth(new LngLat(10, 50), 4)).toBeCloseTo(0.9865782165762236);
+
+    });
 });
diff --git a/src/geo/transform.ts b/src/geo/transform.ts
@@ -458,13 +458,23 @@ export class Transform {
     zoomScale(zoom: number) { return Math.pow(2, zoom); }
     scaleZoom(scale: number) { return Math.log(scale) / Math.LN2; }
 
+    /**
+     * Convert from LngLat to world coordinates (Mercator coordinates scaled by 512)
+     * @param lngLat - the lngLat
+     * @returns Point
+     */
     project(lnglat: LngLat) {
         const lat = clamp(lnglat.lat, -this.maxValidLatitude, this.maxValidLatitude);
         return new Point(
             mercatorXfromLng(lnglat.lng) * this.worldSize,
             mercatorYfromLat(lat) * this.worldSize);
     }
 
+    /**
+     * Convert from world coordinates ([0, 512],[0, 512]) to LngLat ([-180, 180], [-90, 90])
+     * @param point - world coordinate
+     * @returns LngLat
+     */
     unproject(point: Point): LngLat {
         return new MercatorCoordinate(point.x / this.worldSize, point.y / this.worldSize).toLngLat();
     }
@@ -527,7 +537,7 @@ export class Transform {
     }
 
     /**
-     * Given a location, return the screen point that corresponds to it
+     * Given a LngLat location, return the screen point that corresponds to it
      * @param lnglat - location
      * @param terrain - optional terrain
      * @returns screen point
@@ -550,7 +560,7 @@ export class Transform {
 
     /**
      * Given a geographical lnglat, return an unrounded
-     * coordinate that represents it at this transform's zoom level.
+     * coordinate that represents it at low zoom level.
      * @param lnglat - the location
      * @returns The mercator coordinate
      */
@@ -560,7 +570,7 @@ export class Transform {
 
     /**
      * Given a Coordinate, return its geographical position.
-     * @param coord - mercator coordivates
+     * @param coord - mercator coordinates
      * @returns lng and lat
      */
     coordinateLocation(coord: MercatorCoordinate): LngLat {
@@ -588,8 +598,8 @@ export class Transform {
         // unproject two points to get a line and then find the point on that
         // line with z=0
 
-        const coord0 = [p.x, p.y, 0, 1] as any;
-        const coord1 = [p.x, p.y, 1, 1] as any;
+        const coord0 = [p.x, p.y, 0, 1] as vec4;
+        const coord1 = [p.x, p.y, 1, 1] as vec4;
 
         vec4.transformMat4(coord0, coord0, this.pixelMatrixInverse);
         vec4.transformMat4(coord1, coord1, this.pixelMatrixInverse);
@@ -618,7 +628,7 @@ export class Transform {
      * @returns screen point
      */
     coordinatePoint(coord: MercatorCoordinate, elevation: number = 0, pixelMatrix = this.pixelMatrix): Point {
-        const p = [coord.x * this.worldSize, coord.y * this.worldSize, elevation, 1] as any;
+        const p = [coord.x * this.worldSize, coord.y * this.worldSize, elevation, 1] as vec4;
         vec4.transformMat4(p, p, pixelMatrix);
         return new Point(p[0] / p[3], p[1] / p[3]);
     }
@@ -833,12 +843,12 @@ export class Transform {
         // - the more depth precision is available for features (good)
         // - clipping starts appearing sooner when the camera is close to 3d features (bad)
         //
-        // Smaller values worked well for mapbox-gl-js but deckgl was encountering precision issues
-        // when rendering it's layers using custom layers. This value was experimentally chosen and
+        // Other values work for mapbox-gl-js but deckgl was encountering precision issues
+        // when rendering custom layers. This value was experimentally chosen and
         // seems to solve z-fighting issues in deckgl while not clipping buildings too close to the camera.
         const nearZ = this.height / 50;
 
-        // matrix for conversion from location to GL coordinates (-1 .. 1)
+        // matrix for conversion from location to clip space(-1 .. 1)
         m = new Float64Array(16) as any;
         mat4.perspective(m, this._fov, this.width / this.height, nearZ, farZ);
 
@@ -853,21 +863,21 @@ export class Transform {
         mat4.translate(m, m, [-x, -y, 0]);
 
         // The mercatorMatrix can be used to transform points from mercator coordinates
-        // ([0, 0] nw, [1, 1] se) to GL coordinates.
+        // ([0, 0] nw, [1, 1] se) to clip space.
         this.mercatorMatrix = mat4.scale([] as any, m, [this.worldSize, this.worldSize, this.worldSize]);
 
         // scale vertically to meters per pixel (inverse of ground resolution):
         mat4.scale(m, m, [1, 1, this._pixelPerMeter]);
 
-        // matrix for conversion from location to screen coordinates in 2D
+        // matrix for conversion from world space to screen coordinates in 2D
         this.pixelMatrix = mat4.multiply(new Float64Array(16) as any, this.labelPlaneMatrix, m);
 
-        // matrix for conversion from location to GL coordinates (-1 .. 1)
+        // matrix for conversion from world space to clip space (-1 .. 1)
         mat4.translate(m, m, [0, 0, -this.elevation]); // elevate camera over terrain
         this.projMatrix = m;
         this.invProjMatrix = mat4.invert([] as any, m);
 
-        // matrix for conversion from location to screen coordinates in 2D
+        // matrix for conversion from world space to screen coordinates in 3D
         this.pixelMatrix3D = mat4.multiply(new Float64Array(16) as any, this.labelPlaneMatrix, m);
 
         // Make a second projection matrix that is aligned to a pixel grid for rendering raster tiles.
@@ -884,7 +894,7 @@ export class Transform {
         mat4.translate(alignedM, alignedM, [dx > 0.5 ? dx - 1 : dx, dy > 0.5 ? dy - 1 : dy, 0]);
         this.alignedProjMatrix = alignedM;
 
-        // inverse matrix for conversion from screen coordinaes to location
+        // inverse matrix for conversion from screen coordinates to location
         m = mat4.invert(new Float64Array(16) as any, this.pixelMatrix);
         if (!m) throw new Error('failed to invert matrix');
         this.pixelMatrixInverse = m;
@@ -955,4 +965,18 @@ export class Transform {
             ];
         }
     }
+    /**
+     * Return the distance to the camera in clip space from a LngLat.
+     * This can be compared to the value from the depth buffer (terrain.depthAtPoint)
+     * to determine whether a point is occluded.
+     * @param lngLat - the point
+     * @param elevation - the point's elevation
+     * @returns depth value in clip space (between 0 and 1)
+     */
+    lngLatToCameraDepth(lngLat: LngLat, elevation: number) {
+        const coord = this.locationCoordinate(lngLat);
+        const p = [coord.x * this.worldSize, coord.y * this.worldSize, elevation, 1] as vec4;
+        vec4.transformMat4(p, p, this.projMatrix);
+        return (p[2] / p[3]);
+    }
 }
diff --git a/src/gl/framebuffer.ts b/src/gl/framebuffer.ts
@@ -25,7 +25,7 @@ export class Framebuffer {
         if (hasDepth) {
             this.depthAttachment = hasStencil ? new DepthStencilAttachment(context, fbo) : new DepthAttachment(context, fbo);
         } else if (hasStencil) {
-            throw new Error('Stencil cannot be setted without depth');
+            throw new Error('Stencil cannot be set without depth');
         }
         if (gl.checkFramebufferStatus(gl.FRAMEBUFFER) !== gl.FRAMEBUFFER_COMPLETE) {
             throw new Error('Framebuffer is not complete');

diff --git a/src/render/terrain.test.ts b/src/render/terrain.test.ts
@@ -29,7 +29,7 @@ describe('Terrain', () => {
         jest.restoreAllMocks();
     });
 
-    test('pointCoordiate should not return null', () => {
+    test('pointCoordinate should not return null', () => {
         expect.assertions(1);
         const painter = {
             context: new Context(gl),
@@ -91,7 +91,7 @@ describe('Terrain', () => {
     };
 
     test(
-        `pointCoordiate should return negative mercator x
+        `pointCoordinate should return negative mercator x
         if the point is on the LEFT outside the central globe`,
         () => {
             expect.assertions(1);
@@ -103,7 +103,7 @@ describe('Terrain', () => {
         });
 
     test(
-        `pointCoordiate should return mercator x greater than 1
+        `pointCoordinate should return mercator x greater than 1
         if the point is on the RIGHT outside the central globe`,
         () => {
             expect.assertions(1);

diff --git a/src/render/terrain.ts b/src/render/terrain.ts
@@ -124,7 +124,7 @@ export class Terrain {
      */
     _coordsTexture: Texture;
     /**
-     * accuracy of the coords. 2 * tileSize should be enoughth.
+     * accuracy of the coords. 2 * tileSize should be enough.
      */
     _coordsTextureSize: number;
     /**
@@ -348,6 +348,23 @@ export class Terrain {
         );
     }
 
+    /**
+     * Reads the depth value from the depth-framebuffer at a given screen pixel
+     * @param p - Screen coordinate
+     * @returns depth value in clip space (between 0 and 1)
+     */
+
+    depthAtPoint(p: Point): number {
+        const rgba = new Uint8Array(4);
+        const context = this.painter.context, gl = context.gl;
+        context.bindFramebuffer.set(this.getFramebuffer('depth').framebuffer);
+        gl.readPixels(p.x, this.painter.height / devicePixelRatio - p.y - 1, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE, rgba);
+        context.bindFramebuffer.set(null);
+        // decode coordinates (encoding see terran_depth.fragment.glsl)
+        const depthValue = (rgba[0] / (256 * 256 * 256) + rgba[1] / (256 * 256) + rgba[2] / 256 + rgba[3]) / 256;
+        return depthValue;
+    }
+
     /**
      * create a regular mesh which will be used by all terrain-tiles
      * @returns the created regular mesh

diff --git a/src/style/style_layer/custom_style_layer.ts b/src/style/style_layer/custom_style_layer.ts
@@ -6,7 +6,7 @@ import {LayerSpecification} from '@maplibre/maplibre-gl-style-spec';
 /**
  * @param gl - The map's gl context.
  * @param matrix - The map's camera matrix. It projects spherical mercator
- * coordinates to gl coordinates. The spherical mercator coordinate `[0, 0]` represents the
+ * coordinates to gl clip space coordinates. The spherical mercator coordinate `[0, 0]` represents the
  * top left corner of the mercator world and `[1, 1]` represents the bottom right corner. When
  * the `renderingMode` is `"3d"`, the z coordinate is conformal. A box with identical x, y, and z
  * lengths in mercator units would be rendered as a cube. {@link MercatorCoordinate.fromLngLat}

diff --git a/src/symbol/projection.ts b/src/symbol/projection.ts
@@ -25,9 +25,10 @@ export {updateLineLabels, hideGlyphs, getLabelPlaneMatrix, getGlCoordMatrix, pro
  * The points for both anchors and lines are stored in tile units. Each tile has it's own
  * coordinate space going from (0, 0) at the top left to (EXTENT, EXTENT) at the bottom right.
  *
- * ## GL coordinate space
- * At the end of everything, the vertex shader needs to produce a position in GL coordinate space,
+ * ## Clip space (GL coordinate space)
+ * At the end of everything, the vertex shader needs to produce a position in clip space,
  * which is (-1, 1) at the top left and (1, -1) in the bottom right.
+ * In the depth buffer, values are between 0 (near plane) to 1 (far plane).
  *
  * ## Map pixel coordinate spaces
  * Each tile has a pixel coordinate space. It's just the tile units scaled so that one unit is
@@ -51,7 +52,7 @@ export {updateLineLabels, hideGlyphs, getLabelPlaneMatrix, getGlCoordMatrix, pro
  *      - viewport pixel space      pitch-alignment=viewport    rotation-alignment=*
  * 2. if the label follows a line, find the point along the line that is the correct distance from the anchor.
  * 3. add the glyph's corner offset to the point from step 3
- * 4. convert from the label coordinate space to gl coordinates
+ * 4. convert from the label coordinate space to clip space
  *
  * For horizontal labels we want to do step 1 in the shader for performance reasons (no cpu work).
  *      This is what `u_label_plane_matrix` is used for.
@@ -83,7 +84,7 @@ function getLabelPlaneMatrix(posMatrix: mat4,
 }
 
 /*
- * Returns a matrix for converting from the correct label coordinate space to gl coords.
+ * Returns a matrix for converting from the correct label coordinate space to clip space.
  */
 function getGlCoordMatrix(posMatrix: mat4,
     pitchWithMap: boolean,

diff --git a/src/ui/marker.test.ts b/src/ui/marker.test.ts
@@ -800,8 +800,10 @@ describe('marker', () => {
             .setLngLat([0, 0])
             .addTo(map);
         map.terrain = {
-            getElevationForLngLatZoom: () => 0
+            getElevationForLngLatZoom: () => 0,
+            depthAtPoint: () => .9
         } as any as Terrain;
+        map.transform.lngLatToCameraDepth = () => .95;
 
         marker.setOffset([10, 10]);
 
@@ -836,4 +838,36 @@ describe('marker', () => {
         expect(map._oneTimeListeners.render).toHaveLength(0);
         map.remove();
     });
+
+    test('Marker changes opacity behind terrain and when terrain is removed', () => {
+        const map = createMap();
+        map.transform.lngLatToCameraDepth = () => .95; // Mocking distance to marker
+        const marker = new Marker()
+            .setLngLat([0, 0])
+            .addTo(map);
+
+        expect(marker.getElement().style.opacity).toMatch('');
+
+        // Add terrain, not blocking marker
+        map.terrain = {
+            getElevationForLngLatZoom: () => 0,
+            depthAtPoint: () => .95 // Mocking distance to terrain
+        } as any as Terrain;
+        map.fire('terrain');
+
+        expect(marker.getElement().style.opacity).toMatch('1');
+
+        // Terrain blocks marker
+        map.terrain.depthAtPoint = () => .92; // Mocking terrain blocking marker
+        map.fire('moveend');
+
+        expect(marker.getElement().style.opacity).toMatch('.2');
+
+        // Remove terrain
+        map.terrain = null;
+        map.fire('terrain');
+        expect(marker.getElement().style.opacity).toMatch('1');
+
+        map.remove();
+    });
 });