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

Port asymmetric viewport rendering to gl-js #8638

Merged
merged 33 commits into from
Feb 13, 2020
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
a9cc2f9
add edge insets class
Aug 9, 2019
537562c
add unit tests for EdgeInsets
Aug 10, 2019
1ce9ea1
add EdgeInsets docs comments
Aug 10, 2019
e79edaf
WIP tests for rendering padding
Aug 13, 2019
03ac661
working padding visualization
Aug 13, 2019
e01bafa
working initial transition
Aug 14, 2019
c8bb865
working centerPoint refactor
Aug 15, 2019
8bb8460
fix bugs with camera transition functions and padding logic
Aug 15, 2019
649e1d4
account for pixelRatio in showPadding
Aug 15, 2019
aa7fd47
Add synchrounous getPadding and setPadding methods to Map
Aug 17, 2019
7a479e3
fix broken import
Aug 19, 2019
bbba3ee
add render tests for visualizing padding and fix camera unit test
Aug 20, 2019
1cdf37f
Merge branch 'master' of github.com:mapbox/mapbox-gl-js into feat/asy…
Aug 20, 2019
64522f5
add unit test that ensures padding doesnt change if no new values are…
Aug 21, 2019
d0080a4
Merge branch 'master' of github.com:mapbox/mapbox-gl-js into feat/asy…
Sep 9, 2019
3833d3e
fix linter issues
Sep 9, 2019
d10d082
fix more linter issues
Sep 9, 2019
29bd59e
Fix edgeInset interpolation issue
Sep 10, 2019
21ff1b6
Merge <master> into <feat/asymmetric-viewport>
Feb 3, 2020
220673b
Fix integration with LOD based covering tiles
Feb 3, 2020
3420f3f
switch to simpler getCenter implementation
Feb 4, 2020
e39f561
Fix linting
Feb 4, 2020
d45f48f
Remove all user-facing `EdgeInset` types and fix flow errors
Feb 5, 2020
b98ffea
Account for off-axis padding in fitBounds and related functions
Feb 5, 2020
39f4288
Use glClear for drawing debug padding
Feb 5, 2020
44a2654
Delete unused ss_rect shader
Feb 5, 2020
00de0c6
Fix missing imports
Feb 6, 2020
372099a
Remove duplicated centerPoint code
Feb 12, 2020
e0871f7
Remove padding events from camera
Feb 12, 2020
354a555
Remove unused SerializedPadding type
Feb 12, 2020
fbb26fa
Update render tests to represent distortion caused by asymmetric view…
Feb 12, 2020
b7e8e6c
Fix lint error caused by unused variables
Feb 12, 2020
2d0e390
Update render tests to fix precision errors with 0 padding line rende…
Feb 13, 2020
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
103 changes: 103 additions & 0 deletions src/geo/edge_insets.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// @flow
import {number} from "../style-spec/util/interpolate";
import Point from "@mapbox/point-geometry";
import {clamp} from "../util/util";

/**
* An `EdgeInset` object represents screen space padding applied to the edges of the viewport.
* This shifts the apprent center or the vanishing point of the map. This is useful for adding floating UI elements
* on top of the map and having the vanishing point shift as UI elements resize.
*
* @param {number} [top=0]
* @param {number} [bottom=0]
* @param {number} [left=0]
* @param {number} [right=0]
*/
class EdgeInsets {
top: number;
bottom: number;
left: number;
right: number;

constructor(top: number = 0, bottom: number = 0, left: number = 0, right: number = 0) {
if (isNaN(top) || top < 0 ||
isNaN(bottom) || bottom < 0 ||
isNaN(left) || left < 0 ||
isNaN(right) || right < 0
) {
throw new Error('Invalid value for edge-insets, top, bottom, left and right must all be numbers');
}

this.top = top;
this.bottom = bottom;
this.left = left;
this.right = right;
}

/**
* Interpolates the inset in-place.
* This maintains the current inset value for any inset not present in `target`.
*
* @param {PaddingOptions} target
* @param {number} t
* @returns {EdgeInsets}
* @memberof EdgeInsets
*/
interpolate(start: PaddingOptions | EdgeInsets, target: PaddingOptions, t: number): EdgeInsets {
if (target.top != null && start.top != null) this.top = number(start.top, target.top, t);
if (target.bottom != null && start.bottom != null) this.bottom = number(start.bottom, target.bottom, t);
if (target.left != null && start.left != null) this.left = number(start.left, target.left, t);
if (target.right != null && start.right != null) this.right = number(start.right, target.right, t);
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little involved — can we get rid of the ifs by defaulting them to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this logic ensures that the if the key is not present the value does not change change from the existing value logic for adding padding. Defaulting to 0 would remove the padding. and having it here ensures that that logic does not have to be implemented in every public interface function that accepts padding as input.


return this;
Copy link
Contributor

Choose a reason for hiding this comment

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

This interpolates the padding between the previous frame's value and the target instead of from the starting value to the target. Switching to the more functional approach used for other interpolation could be a good idea.

}

/**
* Utility method that computes the new apprent center or vanishing point after applying insets.
* This is in pixels and with the top left being (0.0) and +y being downwards.
*
* @param {number} width
* @param {number} height
* @returns {Point}
* @memberof EdgeInsets
*/
getCenter(width: number, height: number): Point {
// Clamp insets so they never overflow width/height and always calculate a valid center
const x = clamp((this.left + width - this.right) / 2, 0, width);
const y = clamp((this.top + height - this.bottom) / 2, 0, height);

return new Point(x, y);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be simpler as this?

const x  = (width + this.left - this.right) / 2;
const y = (height + this.top - this.bottom) / 2; 

The main difference would be that in the case where left + right > width it would take equal amounts off of both sides rather than only compromising on the right side. Not sure which is preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the simpler one you suggested is better!
Do you think we should throw an error/warning when the overflow happens?

}

equals(other: PaddingOptions): boolean {
return this.top === other.top &&
this.bottom === other.bottom &&
this.left === other.left &&
this.right === other.right;
}

clone(): EdgeInsets {
return new EdgeInsets(this.top, this.bottom, this.left, this.right);
}

/**
* Returns the current sdtate as json, useful when you want to have a
* read-only representation of the inset.
*
* @returns {PaddingOptions}
* @memberof EdgeInsets
*/
toJSON(): PaddingOptions {
return {
top: this.top,
bottom: this.bottom,
left: this.left,
right: this.right
};
}
}

export type PaddingOptions = {top: ?number, bottom: ?number, right: ?number, left: ?number};
export type SerializedPadding = {top: number, bottom: number, right: number, left: number};

export default EdgeInsets;
Copy link
Member

Choose a reason for hiding this comment

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

If we simplify this class somewhat, could it be small enough to the point it's simpler to have this state as a part of transform rather than a separate class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, but this seems easier to write unit tests for, and handles a lot of the validation and updating logic without adding additional private methods to the transform class.

76 changes: 67 additions & 9 deletions src/geo/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ import {number as interpolate} from '../style-spec/util/interpolate';
import EXTENT from '../data/extent';
import {vec4, mat4, mat2, vec2} from 'gl-matrix';
import {Aabb, Frustum} from '../util/primitives.js';
import EdgeInsets from './edge_insets';

import {UnwrappedTileID, OverscaledTileID, CanonicalTileID} from '../source/tile_id';
import type {PaddingOptions} from './edge_insets';

/**
* A single transform, generally used for a single tile to be
Expand Down Expand Up @@ -49,6 +51,7 @@ class Transform {
_minPitch: number;
_maxPitch: number;
_center: LngLat;
_edgeInsets: EdgeInsets;
_constraining: boolean;
_posMatrixCache: {[string]: Float32Array};
_alignedPosMatrixCache: {[string]: Float32Array};
Expand All @@ -74,6 +77,7 @@ class Transform {
this._fov = 0.6435011087932844;
this._pitch = 0;
this._unmodified = true;
this._edgeInsets = new EdgeInsets();
this._posMatrixCache = {};
this._alignedPosMatrixCache = {};
}
Expand All @@ -90,6 +94,7 @@ class Transform {
clone._fov = this._fov;
clone._pitch = this._pitch;
clone._unmodified = this._unmodified;
clone._edgeInsets = this._edgeInsets.clone();
clone._calcMatrices();
return clone;
}
Expand Down Expand Up @@ -137,8 +142,8 @@ class Transform {
return this.tileSize * this.scale;
}

get centerPoint(): Point {
return this.size._div(2);
get centerOffset(): Point {
return this._edgeInsets.getCenter(this.width, this.height).sub(this.size._div(2));
arindam1993 marked this conversation as resolved.
Show resolved Hide resolved
}

get size(): Point {
Expand Down Expand Up @@ -204,6 +209,52 @@ class Transform {
this._calcMatrices();
}

get padding(): PaddingOptions { return this._edgeInsets.toJSON(); }
set padding(padding: PaddingOptions) {
if (this._edgeInsets.equals(padding)) return;
this._unmodified = false;
//Update edge-insets inplace
this._edgeInsets.interpolate(this._edgeInsets, padding, 1);
this._calcMatrices();
}

/**
* The center of the screen in pixels with the top-left corner being (0,0)
* and +y axis pointing downwards. This accounts for padding.
*
* @readonly
* @type {Point}
* @memberof Transform
*/
get centerPoint(): Point {
return this._edgeInsets.getCenter(this.width, this.height);
}

/**
* Returns if the padding params match
*
* @param {PaddingOptions} padding
* @returns {boolean}
* @memberof Transform
*/
isPaddingEqual(padding: PaddingOptions): boolean {
return this._edgeInsets.equals(padding);
}

/**
* Helper method to upadte edge-insets inplace
*
* @param {PaddingOptions} target
* @param {number} t
* @memberof Transform
*/
interpolatePadding(start: PaddingOptions, target: PaddingOptions, t: number) {
this._unmodified = false;
this._edgeInsets.interpolate(start, target, t);
this._constrain();
this._calcMatrices();
}

/**
* Return a zoom level that will cover all tiles the transform
* @param {Object} options
Expand Down Expand Up @@ -281,9 +332,10 @@ class Transform {
const centerPoint = [numTiles * centerCoord.x, numTiles * centerCoord.y, 0];
const cameraFrustum = Frustum.fromInvProjectionMatrix(this.invProjMatrix, this.worldSize, z);

// No change of LOD behavior for pitch lower than 60: return only tile ids from the requested zoom level
// No change of LOD behavior for pitch lower than 60 and when there is no top padding: return only tile ids from the requested zoom level
let minZoom = options.minzoom || 0;
if (this.pitch <= 60.0)
// Use 0.1 as an epsilon to avoid for explicit == 0.0 floating point checks
if (this.pitch <= 60.0 && this._edgeInsets.top < 0.1)
minZoom = z;

// There should always be a certain number of maximum zoom level tiles surrounding the center location
Expand Down Expand Up @@ -616,15 +668,17 @@ class Transform {
_calcMatrices() {
if (!this.height) return;

this.cameraToCenterDistance = 0.5 / Math.tan(this._fov / 2) * this.height;
const halfFov = this._fov / 2;
const offset = this.centerOffset;
this.cameraToCenterDistance = 0.5 / Math.tan(halfFov) * this.height;

// Find the distance from the center point [width/2, height/2] to the
// center top point [width/2, 0] in Z units, using the law of sines.
// Find the distance from the center point [width/2 + offset.x, height/2 + offset.y] to the
// center top point [width/2 + offset.x, 0] in Z units, using the law of sines.
// 1 Z unit is equivalent to 1 horizontal px at the center of the map
// (the distance between[width/2, height/2] and [width/2 + 1, height/2])
const halfFov = this._fov / 2;
const groundAngle = Math.PI / 2 + this._pitch;
const topHalfSurfaceDistance = Math.sin(halfFov) * this.cameraToCenterDistance / Math.sin(clamp(Math.PI - groundAngle - halfFov, 0.01, Math.PI - 0.01));
const fovAboveCenter = this._fov * (0.5 + offset.y / this.height);
const topHalfSurfaceDistance = Math.sin(fovAboveCenter) * this.cameraToCenterDistance / Math.sin(clamp(Math.PI - groundAngle - fovAboveCenter, 0.01, Math.PI - 0.01));
const point = this.point;
const x = point.x, y = point.y;

Expand All @@ -646,6 +700,10 @@ class Transform {
let m = new Float64Array(16);
mat4.perspective(m, this._fov, this.width / this.height, nearZ, farZ);

//Apply center of perspective offset
m[8] = -offset.x * 2 / this.width;
m[9] = offset.y * 2 / this.height;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be done using the methods provided by mat4 (translate, multiply, etc) rather than directly editing the matrix directly? It took me a while to understand the math here

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 only api method that allows that I think can work is the asymmetric view frustum method but that takes 4 different fov's as input, and back calculating that seemed more janky to me.

mat4.scale(m, m, [1, -1, 1]);
mat4.translate(m, m, [0, 0, -this.cameraToCenterDistance]);
mat4.rotateX(m, m, this._pitch);
Expand Down
50 changes: 50 additions & 0 deletions src/render/draw_debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,63 @@ import StencilMode from '../gl/stencil_mode';
import CullFaceMode from '../gl/cull_face_mode';
import {debugUniformValues} from './program/debug_program';
import Color from '../style-spec/util/color';
import browser from '../util/browser';

import type Painter from './painter';
import type SourceCache from '../source/source_cache';
import type {OverscaledTileID} from '../source/tile_id';

export default drawDebug;

const topColor = new Color(1, 0, 0, 1);
const btmColor = new Color(0, 1, 0, 1);
const leftColor = new Color(0, 0, 1, 1);
const rightColor = new Color(1, 0, 1, 1);
const centerColor = new Color(0, 1, 1, 1);

export function drawDebugPadding(painter: Painter) {
const padding = painter.transform.padding;
const lineWidth = 3;
// Top
drawHorizontalLine(painter, painter.transform.height - (padding.top || 0), lineWidth, topColor);
// Bottom
drawHorizontalLine(painter, padding.bottom || 0, lineWidth, btmColor);
// Left
drawVerticalLine(painter, padding.left || 0, lineWidth, leftColor);
// Right
drawVerticalLine(painter, painter.transform.width - (padding.right || 0), lineWidth, rightColor);
// Center
const center = painter.transform.centerPoint;
drawCrosshair(painter, center.x, painter.transform.height - center.y, centerColor);
}

function drawCrosshair(painter: Painter, x: number, y: number, color: Color) {
const size = 20;
const lineWidth = 2;
//Vertical line
drawDebugSSRect(painter, x - lineWidth / 2, y - size / 2, lineWidth, size, color);
//Horizontal line
drawDebugSSRect(painter, x - size / 2, y - lineWidth / 2, size, lineWidth, color);
}

function drawHorizontalLine(painter: Painter, y: number, lineWidth: number, color: Color) {
drawDebugSSRect(painter, 0, y + lineWidth / 2, painter.transform.width, lineWidth, color);
}

function drawVerticalLine(painter: Painter, x: number, lineWidth: number, color: Color) {
drawDebugSSRect(painter, x - lineWidth / 2, 0, lineWidth, painter.transform.height, color);
}

function drawDebugSSRect(painter: Painter, x: number, y: number, width: number, height: number, color: Color) {
const context = painter.context;
const gl = context.gl;

gl.enable(gl.SCISSOR_TEST);
gl.scissor(x * browser.devicePixelRatio, y * browser.devicePixelRatio, width * browser.devicePixelRatio, height * browser.devicePixelRatio);
context.clear({color});
gl.disable(gl.SCISSOR_TEST);
}

Copy link
Member

Choose a reason for hiding this comment

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

Do you anticipate this debug code being used often, or did it already serve its purpose since the asymmetric viewport implementation is complete? Just noting an opportunity to potentially cut this for smaller bundle size impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super often, no actually, but it seems like an useful tool to have for render tests, examples and debug pages. I could make it so it is only available in the dev build, but it will break our usual convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also added some new render tests in the newest commit that represents the distortion caused by asymmetric viewport much better.
https://github.com/mapbox/mapbox-gl-js/pull/8638/files#diff-633f8e084d5e1ba0bb38d2c145e1b771

function drawDebug(painter: Painter, sourceCache: SourceCache, coords: Array<OverscaledTileID>) {
for (let i = 0; i < coords.length; i++) {
drawDebugTile(painter, sourceCache, coords[i]);
Expand Down
7 changes: 6 additions & 1 deletion src/render/painter.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import fillExtrusion from './draw_fill_extrusion';
import hillshade from './draw_hillshade';
import raster from './draw_raster';
import background from './draw_background';
import debug from './draw_debug';
import debug, {drawDebugPadding} from './draw_debug';
import custom from './draw_custom';

const draw = {
Expand Down Expand Up @@ -69,6 +69,7 @@ export type RenderPass = 'offscreen' | 'opaque' | 'translucent';
type PainterOptions = {
showOverdrawInspector: boolean,
showTileBoundaries: boolean,
showPadding: boolean,
rotating: boolean,
zooming: boolean,
moving: boolean,
Expand Down Expand Up @@ -475,6 +476,10 @@ class Painter {
}
}

if (this.options.showPadding) {
drawDebugPadding(this);
}

// Set defaults for most GL values so that anyone using the state after the render
// encounters more expected values.
this.context.setDefault();
Expand Down
Loading