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

Prevent pitching when zooming into terrain #12280

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions src/geo/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ class Transform {
this._updateCameraOnTerrain();
}
if (constrainCameraOverTerrain || centerAltitudeChanged) {
this._constrainCameraAltitude();
this._constrainCamera();
}
this._calcMatrices();
}
Expand Down Expand Up @@ -1586,7 +1586,7 @@ class Transform {
}
}

_constrainCameraAltitude() {
_constrainCamera() {
if (!this._elevation)
return;

Expand All @@ -1599,7 +1599,7 @@ class Transform {
const pos = this._computeCameraPosition(mercPixelsPerMeter);

const elevationAtCamera = elevation.getAtPointOrZero(new MercatorCoordinate(...pos));
const minHeight = this._minimumHeightOverTerrain() * Math.cos(degToRad(this._maxPitch));
const minHeight = this._minimumHeightOverTerrain() * Math.cos(degToRad(85));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for hard-coding this value? Maybe add context in a comment?

Copy link
Contributor Author

@avpeery avpeery Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment. In maps with maxPitch set to less than 85, fast-zooming past terrain is exasperated since it would adjust the camera position too early. Hard-coding this value to 85 instead of maxPitch reduces this experience in these cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Math.cos(degToRad(85)) < Math.cos(degToRad(0)), should we use the upper bound of the possible values in order to keep a constant minHeigh safe bound? That would mean directly using:

        const minHeight = this._minimumHeightOverTerrain();

Since Math.cos(degToRad(0)) is 1. Would that work or lead to issues in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial experience of using minHeight = this._minimumHeightOverTerrain() is a bit more jarring. if you bounce into the terrain it pushes the camera farther up and away. It also allowed for the camera to go under the terrain. I'll look into why

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If this is the correct value, should we consider hard coding the result in order to avoid an unnecessary cos() operation?

Suggested change
const minHeight = this._minimumHeightOverTerrain() * Math.cos(degToRad(85));
const cosMaxPitch = 0.08715560745961583
const minHeight = this._minimumHeightOverTerrain() * cosMaxPitch;

Though, have you experimented with other values to see if you the behavior can be smoother?

const terrainElevation = this.pixelsPerMeter / this.worldSize * elevationAtCamera;
const cameraHeight = this._camera.position[2] - terrainElevation;

Expand All @@ -1611,15 +1611,13 @@ class Transform {
// Adjust the camera vector so that the camera is placed above the terrain.
// Distance between the camera and the center point is kept constant.
cameraToCenter[2] -= (minHeight - cameraHeight) / this._pixelsPerMercatorPixel;

const newDistToCamera = vec3.length(cameraToCenter);

if (newDistToCamera === 0)
return;

vec3.scale(cameraToCenter, cameraToCenter, prevDistToCamera / newDistToCamera * this._pixelsPerMercatorPixel);
this._camera.position = [center.x - cameraToCenter[0], center.y - cameraToCenter[1], center.z * this._pixelsPerMercatorPixel - cameraToCenter[2]];

this._camera.orientation = orientationFromFrame(cameraToCenter, this._camera.up());
this._updateStateFromCamera();
}
}
Expand Down Expand Up @@ -1681,7 +1679,7 @@ class Transform {
this.zoom += this.scaleZoom(s);
}

this._constrainCameraAltitude();
this._constrainCamera();
this._unmodified = unmodified;
this._constraining = false;
}
Expand Down Expand Up @@ -1968,10 +1966,10 @@ class Transform {

_minimumHeightOverTerrain(): number {
// Determine minimum height for the camera over the terrain related to current zoom.
// Values above than 2 allow max-pitch camera closer to e.g. top of the hill, exposing
// Values above than 4 allow max-pitch camera closer to e.g. top of the hill, exposing
avpeery marked this conversation as resolved.
Show resolved Hide resolved
// drape raster overscale artifacts or cut terrain (see under it) as it gets clipped on
// near plane. Returned value is in mercator coordinates.
const MAX_DRAPE_OVERZOOM = 2;
const MAX_DRAPE_OVERZOOM = 4;
const zoom = Math.min((this._seaLevelZoom != null ? this._seaLevelZoom : this._zoom) + MAX_DRAPE_OVERZOOM, this._maxZoom);
return this._mercatorZfromZoom(zoom);
}
Expand Down
4 changes: 2 additions & 2 deletions src/render/painter.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,15 +188,15 @@ class Painter {
this._backgroundTiles = {};
}

updateTerrain(style: Style, cameraChanging: boolean) {
updateTerrain(style: Style, isZooming: boolean) {
const enabled = !!style && !!style.terrain && this.transform.projection.supportsTerrain;
if (!enabled && (!this._terrain || !this._terrain.enabled)) return;
if (!this._terrain) {
this._terrain = new Terrain(this, style);
}
const terrain: Terrain = this._terrain;
this.transform.elevation = enabled ? terrain : null;
terrain.update(style, this.transform, cameraChanging);
terrain.update(style, this.transform, isZooming);
}

_updateFog(style: Style) {
Expand Down
9 changes: 4 additions & 5 deletions src/terrain/terrain.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,8 @@ export class Terrain extends Elevation {
* Validate terrain and update source cache used for elevation.
* Explicitly pass transform to update elevation (Transform.updateElevation)
* before using transform for source cache update.
* cameraChanging is true when camera is zooming, panning or orbiting.
*/
update(style: Style, transform: Transform, cameraChanging: boolean) {
update(style: Style, transform: Transform, isZooming: boolean) {
if (style && style.terrain) {
if (this._style !== style) {
this.style = style;
Expand Down Expand Up @@ -323,9 +322,9 @@ export class Terrain extends Elevation {
}

updateSourceCache();
// Camera, when changing, gets constrained over terrain. Issue constrainCameraOverTerrain = true
// here to cover potential under terrain situation on data or style change.
transform.updateElevation(!cameraChanging);
// Camera, when zooming, gets constrained over terrain. Issue constrainCameraOverTerrain = true
// here to cover potential under terrain situation on data, style, or other camera changes.
transform.updateElevation(!isZooming);

// Reset tile lookup cache and update draped tiles coordinates.
this.resetTileLookupCache(this.proxySourceCache.id);
Expand Down
2 changes: 1 addition & 1 deletion src/ui/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -3392,7 +3392,7 @@ class Map extends Camera {
_updateTerrain() {
// Recalculate if enabled/disabled and calculate elevation cover. As camera is using elevation tiles before
// render (and deferred update after zoom recalculation), this needs to be called when removing terrain source.
this.painter.updateTerrain(this.style, this.isMoving() || this.isRotating() || this.isZooming());
this.painter.updateTerrain(this.style, this.isZooming());
}

_calculateSpeedIndex(): number {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
{
"version": 8,
"metadata": {
"test": {
"height": 256,
"width": 256
}
},
"center": [
-104.93611234965806,
38.85724324489064
],
"zoom": 22,
"pitch": 85,
"bearing": -79,
"terrain": {
"source": "rgbterrain",
"exaggeration": 1
},
"sources": {
"rgbterrain": {
"type": "raster-dem",
"tiles": [
"local://tiles/12-759-1609.terrain.png"
],
"maxzoom": 11,
"tileSize": 256
}
},
"glyphs": "local://glyphs/{fontstack}/{range}.pbf",
"layers": [
{
"id": "background",
"type": "background",
"paint": {
"background-color": "gray"
}
},
{
"id": "hillshade-translucent",
"type": "hillshade",
"source": "rgbterrain",
"paint": {
"hillshade-exaggeration": 1
}
}
]
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
48 changes: 40 additions & 8 deletions test/unit/geo/transform.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import LngLat from '../../../src/geo/lng_lat.js';
import {OverscaledTileID, CanonicalTileID} from '../../../src/source/tile_id.js';
import {fixedNum, fixedLngLat, fixedCoord, fixedPoint, fixedVec3, fixedVec4} from '../../util/fixed.js';
import {FreeCameraOptions} from '../../../src/ui/free_camera.js';
import MercatorCoordinate, {mercatorZfromAltitude, MAX_MERCATOR_LATITUDE} from '../../../src/geo/mercator_coordinate.js';
import MercatorCoordinate, {mercatorZfromAltitude, altitudeFromMercatorZ, MAX_MERCATOR_LATITUDE} from '../../../src/geo/mercator_coordinate.js';
import {vec3, quat} from 'gl-matrix';
import LngLatBounds from '../../../src/geo/lng_lat_bounds.js';
import {degToRad, radToDeg} from '../../../src/util/util.js';
Expand Down Expand Up @@ -714,13 +714,12 @@ test('transform', (t) => {
};
};

test('Constrained camera height over terrain', (t) => {
test('Pitch does not change when camera collides with terrain', (t) => {
const transform = new Transform();
transform.resize(200, 200);
transform.maxPitch = 85;

transform.elevation = createCollisionElevation(10);
transform.constantCameraHeight = false;
transform.bearing = -45;
transform.pitch = 85;

Expand All @@ -729,13 +728,36 @@ test('transform', (t) => {
const zoom = transform._zoomFromMercatorZ(altitudeZ);
transform.zoom = zoom;

// Pitch should have been adjusted so that the camera isn't under the terrain
const pixelsPerMeter = mercatorZfromAltitude(1, transform.center.lat) * transform.worldSize;
const updatedAltitude = transform.cameraToCenterDistance / pixelsPerMeter * Math.cos(degToRad(transform.pitch));
const cameraAltitude = altitudeFromMercatorZ(transform.getFreeCameraOptions().position.z, transform.getFreeCameraOptions().position.y);
transform.updateElevation(false);

t.ok(cameraAltitude > 10);
t.equal(fixedNum(transform.bearing), -45);
t.equal(fixedNum(transform.pitch), 85);

t.end();
});

test('Camera height is above terrain with constant elevation', (t) => {
const transform = new Transform();
transform.resize(200, 200);
transform.maxPitch = 85;

transform.elevation = createConstantElevation(10);
transform.bearing = -45;
transform.pitch = 85;

// Set camera altitude to 5 meters
const altitudeZ = mercatorZfromAltitude(5, transform.center.lat) / Math.cos(degToRad(85));
const zoom = transform._zoomFromMercatorZ(altitudeZ);
transform.zoom = zoom;

const cameraAltitude = altitudeFromMercatorZ(transform.getFreeCameraOptions().position.z, transform.getFreeCameraOptions().position.y);

t.true(updatedAltitude > 10);
t.ok(cameraAltitude > 10);
t.equal(fixedNum(transform.zoom), fixedNum(zoom));
t.equal(fixedNum(transform.bearing), -45);
t.equal(fixedNum(transform.pitch), 85);

t.end();
});
Expand Down Expand Up @@ -763,31 +785,38 @@ test('transform', (t) => {

t.equal(fixedNum(cameraAltitude()), 5);
t.equal(transform._centerAltitude, 0);

// increase exaggeration to lift the center (and camera that follows it) up.
transform.elevation._exaggeration = 1;
transform.updateElevation(false);
t.equal(transform._centerAltitude, 10);

const cameraAltitudeAfterLift = cameraAltitude();
t.equal(fixedNum(cameraAltitudeAfterLift), 15);

transform.elevation._exaggeration = 15;
transform.updateElevation(false);
t.equal(transform._centerAltitude, 150);
t.equal(fixedNum(cameraAltitude()), 155);

transform.elevation._exaggeration = 0;
transform.updateElevation(false);
t.equal(fixedNum(cameraAltitude()), 5);

// zoom out to 10 meters and back to 5
transform.zoom = transform._zoomFromMercatorZ(altitudeZ * 2);
t.equal(fixedNum(cameraAltitude()), 10);
transform.zoom = zoom;
t.equal(fixedNum(cameraAltitude()), 5);
t.equal(fixedNum(transform.pitch), 85);

transform.elevation = null;
t.ok(cameraAltitude() < 10);

// collision elevation keeps center at 0 but pushes camera up.
const elevation1 = createCollisionElevation(10);
elevation1._exaggeration = 0;

transform.elevation = elevation1;
transform.updateElevation(false);
t.equal(transform._centerAltitude, 0);
Expand All @@ -796,7 +825,8 @@ test('transform', (t) => {
elevation1._exaggeration = 1;
transform.updateElevation(false);
t.equal(transform._centerAltitude, 0);
t.ok(cameraAltitude() > 11 && cameraAltitude() < 12);
t.ok(cameraAltitude() > 10);
t.equal(fixedNum(transform.pitch), 85);

t.end();
});
Expand Down Expand Up @@ -1058,6 +1088,8 @@ test('transform', (t) => {
tilesDefaultElevation = null;
centerElevation = 1600;
tileElevation[new OverscaledTileID(14, 0, 14, 3413, 6218).key] = 1600;
transform.pitch = 0;
transform.bearing = 0;
transform.resize(768, 768);
transform.zoom = options.maxzoom = 22;
transform.center = {lng: -104.99813327, lat: 39.72784465999999};
Expand Down
6 changes: 3 additions & 3 deletions test/unit/terrain/terrain.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,19 +378,19 @@ test('Elevation', (t) => {
changed = map._updateAverageElevation(timestamp);
t.true(changed);
t.true(map._averageElevation.isEasing(timestamp));
assertAlmostEqual(t, map.transform.averageElevation, 154.15083854452925);
assertAlmostEqual(t, map.transform.averageElevation, -17.781004493268256);

timestamp += AVERAGE_ELEVATION_EASE_TIME * 0.5;
changed = map._updateAverageElevation(timestamp);
t.true(changed);
t.true(map._averageElevation.isEasing(timestamp));
assertAlmostEqual(t, map.transform.averageElevation, 308.3016770890585);
assertAlmostEqual(t, map.transform.averageElevation, -35.56200898653651);

timestamp += AVERAGE_ELEVATION_SAMPLING_INTERVAL;
changed = map._updateAverageElevation(timestamp);
t.false(changed);
t.false(map._averageElevation.isEasing(timestamp));
assertAlmostEqual(t, map.transform.averageElevation, 308.3016770890585);
assertAlmostEqual(t, map.transform.averageElevation, -35.56200898653651);

t.end();
});
Expand Down