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

Alternate zoom with terrain #12354

Merged
merged 50 commits into from
Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
1159341
remove code that pitches camera above terrain WIP
avpeery Sep 30, 2022
5364b04
WIP
avpeery Oct 7, 2022
47504b8
remove changing z position and updating camera orientation
avpeery Oct 11, 2022
b01337e
re-add change to minHeight
avpeery Oct 11, 2022
5e01266
remove extra spacing
avpeery Oct 11, 2022
da3d155
update integration tests - since pitch is not being adjusted
avpeery Oct 13, 2022
b5cb3ac
fix failing unit tests, remove unused code
avpeery Oct 18, 2022
e590e8e
add new render test to show camera stops and doesn't pitch when colli…
avpeery Oct 18, 2022
7ad12dc
fix clipping terrain with drag gestures
avpeery Oct 19, 2022
f434e74
formatting fix
avpeery Oct 19, 2022
9556be9
revert to original comment
avpeery Oct 19, 2022
5c70f09
decreased clipping
avpeery Oct 19, 2022
59c4b2a
removing orientation change - does not help lessen clipping
avpeery Oct 19, 2022
2636f8f
update render tests
avpeery Oct 19, 2022
30e461f
run constrainCamera when camera is moving and when potential style or…
avpeery Oct 19, 2022
619f578
typo
avpeery Oct 19, 2022
2e7e89e
update averageElevation unit test
avpeery Oct 20, 2022
4001079
reset pitch and bearing for unit test
avpeery Oct 26, 2022
f159782
skip constraining when zooming as it is already contrained
avpeery Oct 26, 2022
62e080f
change comment
avpeery Oct 26, 2022
6fb34fe
add comment to explain hard-coding 85 value
avpeery Oct 26, 2022
7454aef
Wording
avpeery Oct 26, 2022
9818d69
divide logic between under terrain and safe distance from terrain
avpeery Nov 2, 2022
793f74d
stop zooming at safe distance from terrain
avpeery Nov 2, 2022
8c1de95
update to _isCameraConstrained
avpeery Nov 2, 2022
821aa0e
update camera state add back in
avpeery Nov 2, 2022
51fb421
add isDragging check
avpeery Nov 3, 2022
1759ba5
prevent sliding when zooming into terrain
avpeery Nov 4, 2022
fa64d10
unneeded changes
avpeery Nov 4, 2022
6f2355a
unneeded change
avpeery Nov 4, 2022
1cac447
bump back minimum height to avoid any clipping through terrain
avpeery Nov 4, 2022
9890b39
catch double click and double tap zoom events
avpeery Nov 8, 2022
3796928
fix flow issues
avpeery Nov 8, 2022
71814dc
update unit tests
avpeery Nov 9, 2022
1ec5bd6
update render tests
avpeery Nov 9, 2022
9683c27
use default value for constrainCamera
avpeery Nov 9, 2022
01536c1
don't change minimum height based on maxPitch
avpeery Nov 18, 2022
20bbce5
resolve situation when zooming with keyboard
avpeery Nov 18, 2022
7775495
control keyboard zooming
avpeery Nov 18, 2022
b51a88a
simplify logic
avpeery Nov 18, 2022
e40ef59
set camera position directly
avpeery Nov 18, 2022
a4bf61c
fix unit test
avpeery Nov 18, 2022
2afa50d
update render test
avpeery Nov 18, 2022
b061c73
add default argument for isDragging
avpeery Nov 18, 2022
af15e40
removed comment
avpeery Nov 18, 2022
fdfeac2
Merge branch 'main' into alternate-zoom-with-terrain
avpeery Dec 9, 2022
b790159
update setting camera position, rename isDragging
avpeery Dec 12, 2022
96f5c5d
only update camera state when updating camera position
avpeery Dec 12, 2022
34d921e
decided against constraining keyboard panning and zooming
avpeery Dec 13, 2022
6020e65
Clarify naming
avpeery Dec 13, 2022
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
49 changes: 28 additions & 21 deletions src/geo/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ class Transform {
_nearZ: number;
_farZ: number;
_mercatorScaleRatio: number;
_isCameraConstrained: boolean;

constructor(minZoom: ?number, maxZoom: ?number, minPitch: ?number, maxPitch: ?number, renderWorldCopies: boolean | void, projection?: ?ProjectionSpecification, bounds: ?LngLatBounds) {
this.tileSize = 512; // constant
Expand Down Expand Up @@ -225,13 +226,14 @@ class Transform {
this._updateCameraOnTerrain();
this._calcMatrices();
}
updateElevation(constrainCameraOverTerrain: boolean) { // On render, no need for higher granularity on update reasons.

updateElevation(constrainCameraOverTerrain: boolean, isDragging: boolean = false) {
const centerAltitudeChanged = this._elevation && this._elevation.exaggeration() !== this._centerAltitudeValidForExaggeration;
if (this._seaLevelZoom == null || centerAltitudeChanged) {
this._updateCameraOnTerrain();
}
if (constrainCameraOverTerrain || centerAltitudeChanged) {
this._constrainCameraAltitude();
this._constrainCamera(isDragging);
}
this._calcMatrices();
}
Expand Down Expand Up @@ -1586,7 +1588,7 @@ class Transform {
}
}

_constrainCameraAltitude() {
_constrainCamera(isDragging: boolean = false) {
if (!this._elevation)
return;

Expand All @@ -1597,30 +1599,35 @@ class Transform {
// The default camera position might have been compensated by the active projection model.
const mercPixelsPerMeter = mercatorZfromAltitude(1, this._center.lat) * this.worldSize;
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();
const terrainElevation = this.pixelsPerMeter / this.worldSize * elevationAtCamera;
const cameraHeight = this._camera.position[2] - terrainElevation;

if (cameraHeight < minHeight) {
const center = this.locationCoordinate(this._center, this._centerAltitude);
const cameraToCenter = [center.x - pos[0], center.y - pos[1], center.z - pos[2]];
const prevDistToCamera = vec3.length(cameraToCenter);
// If camera is under terrain or dragging at unsafe distance from terrain, force camera position above terrain
if (cameraHeight <= 0 || isDragging) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there more situations where we need to run this path? I'm not sure whether this should be tied to the drag gesture and instead run always. While testing the PR, I've noticed that when the camera has some inertia due to a click-drag-release gesture, it results in the camera getting too close to the terrain, then when picking it back up with a drag gesture the constraint gets applied again resulting in an immediate camera jump.

Copy link
Contributor Author

@avpeery avpeery Nov 21, 2022

Choose a reason for hiding this comment

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

If we run it always we experience a weird affect of jumping while panning/zooming, but it felt more intuitive with dragging and allows for the user to keep moving forward in the terrain by dragging if they zoom too close. I'll check out the mouse codepath that @SnailBones suggested. Thanks for the feedback and testing it out @karimnaaji!

const center = this.locationCoordinate(this._center, this._centerAltitude);
const cameraToCenter = [center.x - pos[0], center.y - pos[1], center.z - pos[2]];

// 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 prevDistToCamera = vec3.length(cameraToCenter);
// 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);

const newDistToCamera = vec3.length(cameraToCenter);
if (newDistToCamera === 0)
return;
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]];
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]];
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not introduced in this PR, but I'm curious as to why we adjust the camera along the vector of camera to center? Why not simply raise the camera`s z value?

this._updateStateFromCamera();

this._camera.orientation = orientationFromFrame(cameraToCenter, this._camera.up());
this._updateStateFromCamera();
// Set camera as constrained to keep zoom at safe distance from terrain
} else {
this._isCameraConstrained = true;
}
}
}

Expand Down Expand Up @@ -1681,7 +1688,7 @@ class Transform {
this.zoom += this.scaleZoom(s);
}

this._constrainCameraAltitude();
this._constrainCamera();
this._unmodified = unmodified;
this._constraining = false;
}
Expand Down Expand Up @@ -1968,10 +1975,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 4 allow camera closer to e.g. top of the hill, exposing
// 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, isDragging: boolean) {
avpeery marked this conversation as resolved.
Show resolved Hide resolved
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, isDragging);
}

_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, isDragging: 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 gets constrained over terrain. Issue constrainCameraOverTerrain = true
// here to cover potential under terrain situation on data, style, or other camera changes.
transform.updateElevation(true, isDragging);

// Reset tile lookup cache and update draped tiles coordinates.
this.resetTileLookupCache(this.proxySourceCache.id);
Expand Down
9 changes: 8 additions & 1 deletion src/ui/handler/keyboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,18 @@ class KeyboardHandler {
return {
cameraAnimation: (map: Map) => {
const zoom = map.getZoom();

// Camera is constrained to control zooming too close to terrain
if (map.transform._isCameraConstrained) {
if (zoomDir && zoomDir > 0) zoomDir = 0;
if (yDir && yDir < 0) yDir = 0;
map.transform._isCameraConstrained = false;
}

Copy link
Contributor

@SnailBones SnailBones Nov 18, 2022

Choose a reason for hiding this comment

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

I'm concerned that including this logic in the keyboard handler introduces excess complexity and fragility. Could we have the same behavior on all easeTo and flyTo events (or better yet, all map movements?), allowing us to keep interactions and terrain handling separate?

Copy link
Contributor Author

@avpeery avpeery Nov 18, 2022

Choose a reason for hiding this comment

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

Thanks for the feedback - I'll look into easeTo in general (I don't think flyTo is used in any of these movements). There does need to be some map movements that are allowed, so the user can zoom back or pan back when getting too close to terrain (otherwise they will get stuck)

Copy link
Contributor

@SnailBones SnailBones Nov 18, 2022

Choose a reason for hiding this comment

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

There does need to be some map movements that are allowed, so the user can zoom back or pan back when getting too close to terrain (otherwise they will get stuck)

Would it be possible to decide if a movement is allowed or not allowed from with transform.js?

Do you think that we could change the behavior of updateElevation _constrainCamera() so that they don't need any arguments and camera constraint is based on (invalid) position alone? Then instead of blocking specific interactions, invalid ones would essentially be a noop.

Or is it important for user experience that we allow zooming to a higher zoom level while constraining panning to a lower one? (The latter may well be the case, both mouse interactions seem to work well in this PR)

Besides code readability and maintainability, another reason why I think separating interaction and camera adjustment code would be preferable is that it's hard to predict how users will interact with camera position. For instance withreact-map-gl map position is overridden at every frame (the reason is to sync position with kepler.gl, but this applies to developers using the extension whether they use kepler.gl or no). So if clipping prevention depends on the specific type of interaction (or even the specific event called), we might see unpredictable behavior.

map.easeTo({
duration: 300,
easeId: 'keyboardHandler',
easing: easeOut,

zoom: zoomDir ? Math.round(zoom) + zoomDir * (e.shiftKey ? 2 : 1) : zoom,
bearing: map.getBearing() + bearingDir * this._bearingStep,
pitch: map.getPitch() + pitchDir * this._pitchStep,
Expand Down
15 changes: 14 additions & 1 deletion src/ui/handler_manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,10 @@ class HandlerManager {
return !!isMoving(this._eventsInProgress) || this.isZooming();
}

_isDragging(): boolean {
return !!this._eventsInProgress.drag;
}

_blockedByActive(activeHandlers: { [string]: Handler }, allowed: Array<string>, myName: string): boolean {
for (const name in activeHandlers) {
if (name === myName) continue;
Expand Down Expand Up @@ -491,12 +495,22 @@ class HandlerManager {
if (preZoom !== tr.zoom) this._map._update(true);
}

// Catches double click and double tap zooms when camera is constrained over terrain
if (tr._isCameraConstrained) map._stop(true);

if (!hasChange(combinedResult)) {
this._fireEvents(combinedEventsInProgress, deactivatedHandlers, true);
return;
}

let {panDelta, zoomDelta, bearingDelta, pitchDelta, around, aroundCoord, pinchAround} = combinedResult;

if (tr._isCameraConstrained) {
// Catches wheel zoom events when camera is constrained over terrain
if (zoomDelta > 0) zoomDelta = 0;
tr._isCameraConstrained = false;
}

if (pinchAround !== undefined) {
around = pinchAround;
}
Expand Down Expand Up @@ -582,7 +596,6 @@ class HandlerManager {
this._map._update();
if (!combinedResult.noInertia) this._inertia.record(combinedResult);
this._fireEvents(combinedEventsInProgress, deactivatedHandlers, true);

}

_fireEvents(newEventsInProgress: { [string]: Object }, deactivatedHandlers: Object, allowEndAnimation: boolean) {
Expand Down
6 changes: 5 additions & 1 deletion src/ui/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,10 @@ class Map extends Camera {
return this._rotating || (this.handlers && this.handlers.isRotating()) || false;
}

_isDragging(): boolean {
return (this.handlers && this.handlers._isDragging()) || false;
}

_createDelegatedListener(type: MapEvent, layers: Array<any>, listener: any): any {
if (type === 'mouseenter' || type === 'mouseover') {
let mousein = false;
Expand Down Expand Up @@ -3392,7 +3396,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._isDragging());
avpeery marked this conversation as resolved.
Show resolved Hide resolved
}

_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.
Loading