-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from 11 commits
a9cc2f9
537562c
1ce9ea1
e79edaf
03ac661
e01bafa
c8bb865
8bb8460
649e1d4
aa7fd47
7a479e3
bbba3ee
1cdf37f
64522f5
d0080a4
3833d3e
d10d082
29bd59e
21ff1b6
220673b
3420f3f
e39f561
d45f48f
b98ffea
39f4288
44a2654
00de0c6
372099a
e0871f7
354a555
fbb26fa
b7e8e6c
2d0e390
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
// @flow | ||
import { number } from "../style-spec/util/interpolate"; | ||
import Point from "@mapbox/point-geometry"; | ||
|
||
/** | ||
* 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 {EdgeInsetLike} target | ||
* @param {number} t | ||
* @returns {EdgeInsets} | ||
* @memberof EdgeInsets | ||
*/ | ||
interpolate(target: EdgeInsetLike, t: number): EdgeInsets { | ||
if (target.top != null) this.top = number(this.top, target.top, t); | ||
if (target.bottom != null) this.bottom = number(this.bottom, target.bottom, t); | ||
if (target.left != null) this.left = number(this.left, target.left, t); | ||
if (target.right != null) this.right = number(this.right, target.right, t); | ||
|
||
return this; | ||
} | ||
|
||
/** | ||
* 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 totalXInset = Math.min(this.left + this.right, width); | ||
astojilj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const totalYInset = Math.min(this.top + this.bottom, height); | ||
|
||
const x = Math.min(this.left, width) + 0.5 * (width - totalXInset); | ||
const y = Math.min(this.top, height) + 0.5 * (height - totalYInset); | ||
|
||
return new Point(x, y); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this be simpler as this?
The main difference would be that in the case where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the simpler one you suggested is better! |
||
} | ||
|
||
equals(other: EdgeInsetLike): 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 {EdgeInsetJSON} | ||
* @memberof EdgeInsets | ||
*/ | ||
toJSON(): EdgeInsetJSON { | ||
return { | ||
top: this.top, | ||
bottom: this.bottom, | ||
left: this.left, | ||
right: this.right | ||
}; | ||
} | ||
} | ||
|
||
export type EdgeInsetLike = EdgeInsets | {top?: number, bottom?: number, right?: number, left?: number} | EdgeInsetJSON; | ||
export type EdgeInsetJSON = {top: number, bottom: number, right: number, left: number} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not using PaddingOptions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that padding might be a better name here since that is what is exposed. |
||
|
||
export default EdgeInsets; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,10 @@ import tileCover from '../util/tile_cover'; | |
import { UnwrappedTileID } from '../source/tile_id'; | ||
import EXTENT from '../data/extent'; | ||
import { vec4, mat4, mat2 } from 'gl-matrix'; | ||
import EdgeInsets from './edge_insets'; | ||
|
||
import type { OverscaledTileID, CanonicalTileID } from '../source/tile_id'; | ||
import type { EdgeInsetLike, EdgeInsetJSON } from './edge_insets'; | ||
|
||
/** | ||
* A single transform, generally used for a single tile to be | ||
|
@@ -47,6 +49,7 @@ class Transform { | |
_minZoom: number; | ||
_maxZoom: number; | ||
_center: LngLat; | ||
_edgeInsets: EdgeInsets; | ||
_constraining: boolean; | ||
_posMatrixCache: {[number]: Float32Array}; | ||
_alignedPosMatrixCache: {[number]: Float32Array}; | ||
|
@@ -69,6 +72,7 @@ class Transform { | |
this._fov = 0.6435011087932844; | ||
this._pitch = 0; | ||
this._unmodified = true; | ||
this._edgeInsets = new EdgeInsets(); | ||
this._posMatrixCache = {}; | ||
this._alignedPosMatrixCache = {}; | ||
} | ||
|
@@ -85,6 +89,7 @@ class Transform { | |
clone._fov = this._fov; | ||
clone._pitch = this._pitch; | ||
clone._unmodified = this._unmodified; | ||
clone._edgeInsets = this._edgeInsets.clone(); | ||
clone._calcMatrices(); | ||
return clone; | ||
} | ||
|
@@ -118,8 +123,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 { | ||
|
@@ -185,6 +190,52 @@ class Transform { | |
this._calcMatrices(); | ||
} | ||
|
||
get padding(): EdgeInsetJSON { return this._edgeInsets.toJSON(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Serving the existing API in native, i didn't like that in Android we use "padding" and in iOS "edgeInsets". is padding here added to keep it close to native implementation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just did it so the public facing term for is it simpler, but happy to change it to whatever. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this feature, I think
I did find this https://developer.mozilla.org/en-US/docs/Web/CSS/inset "experimental technology" but it sounds like this @arindam1993 are there other parts of the GL-JS code base that also use this idea of padding? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @arindam1993 , related to this - what's your opinion about propagating CSS style padding specified to Map's parent HTML container to transform? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chloekraw I don't think so. @astojilj I don't think we should, I think devs waning to create floating UI elements, which this feature is designed for would want to set a However I think it might be useful to push the current state of Having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
set padding(padding: EdgeInsetLike) { | ||
if (this._edgeInsets.equals(padding)) return; | ||
this._unmodified = false; | ||
//Update edge-insets inplace | ||
this._edgeInsets.interpolate(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 {EdgeInsetLike} padding | ||
* @returns {boolean} | ||
* @memberof Transform | ||
*/ | ||
isPaddingEqual(padding: EdgeInsetLike): boolean { | ||
return this._edgeInsets.equals(padding); | ||
} | ||
|
||
/** | ||
* Helper method to upadte edge-insets inplace | ||
* | ||
* @param {EdgeInsetLike} target | ||
* @param {number} t | ||
* @memberof Transform | ||
*/ | ||
interpolatePadding(target: EdgeInsetLike, t: number) { | ||
this._unmodified = false; | ||
this._edgeInsets.interpolate(target, t); | ||
this._constrain(); | ||
this._calcMatrices(); | ||
} | ||
|
||
/** | ||
* Return a zoom level that will cover all tiles the transform | ||
* @param {Object} options | ||
|
@@ -520,15 +571,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(Math.PI - groundAngle - halfFov); | ||
const fovAboveCenter = this._fov * (0.5 + offset.y / this.height); | ||
const topHalfSurfaceDistance = Math.sin(fovAboveCenter) * this.cameraToCenterDistance / Math.sin(Math.PI - groundAngle - fovAboveCenter); | ||
const point = this.point; | ||
const x = point.x, y = point.y; | ||
|
||
|
@@ -541,6 +594,10 @@ class Transform { | |
let m = new Float64Array(16); | ||
mat4.perspective(m, this._fov, this.width / this.height, 1, farZ); | ||
|
||
//Apply center of perspective offset | ||
m[8] = -offset.x * 2 / this.width; | ||
m[9] = offset.y * 2 / this.height; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be done using the methods provided by There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,15 +9,60 @@ import SegmentVector from '../data/segment'; | |
import DepthMode from '../gl/depth_mode'; | ||
import StencilMode from '../gl/stencil_mode'; | ||
import CullFaceMode from '../gl/cull_face_mode'; | ||
import { debugUniformValues } from './program/debug_program'; | ||
import ColorMode from '../gl/color_mode'; | ||
import { debugUniformValues, debugSSRectUniformValues } 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, 0.4); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does draw_debug.js go to production code?
and in that that page would be used for this step. Please doublecheck my opinion with other reviewers - I might be missing something... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. draw_debug does go into the final bundle, and we do publish docs for And regarding the debug page, I agree, I can add one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Having this as part of the built-in debug shaders allows easily debugging issues when building/updating work in this area. I can see it being very useful for debugging flyTo animations for example. That said, this PR adds a total of +4.13 kB to the gl-js bundle. Is there a simpler debug option that can be built? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding a debug view. They can be pretty useful for understanding things. If the size and complexity of the shader another approach could be to use a gl scissor and clear to draw a line at each edge. gl.enable(gl.SCISSOR_TEST);
gl.scissor(x, y, width, height);
context.clear({ color });
gl.disable(gl.SCISSOR_TEST); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^^ I did this, it is much simpler though it only saved half a kb of bundle size. |
||
const btmColor = new Color(0, 1, 0, 0.4); | ||
const leftColor = new Color(0, 0, 1, 0.4); | ||
const rightColor = new Color(1, 0, 1, 0.4); | ||
const centerColor = new Color(0, 1, 1, 0.4); | ||
|
||
export function drawDebugPadding(painter: Painter) { | ||
const padding = painter.transform.padding; | ||
// Top | ||
drawDebugSSRect(painter, 0, 0, painter.transform.width, padding.top, topColor); | ||
// Bottom | ||
drawDebugSSRect(painter, 0, painter.transform.height - padding.bottom, painter.transform.width, padding.bottom, btmColor); | ||
// Left | ||
drawDebugSSRect(painter, 0, 0, padding.left, painter.transform.height, leftColor); | ||
// Right | ||
drawDebugSSRect(painter, painter.transform.width - padding.right, 0, padding.right, painter.transform.height, rightColor); | ||
// Center | ||
const center = painter.transform.centerPoint; | ||
const centerSize = 10; | ||
drawDebugSSRect(painter, center.x - centerSize / 2, center.y - centerSize / 2, centerSize, centerSize, centerColor); | ||
} | ||
|
||
function drawDebugSSRect(painter: Painter, x: number, y: number, width: number, height: number, color: Color) { | ||
const context = painter.context; | ||
const gl = context.gl; | ||
|
||
const program = painter.useProgram('debugSSRect'); | ||
|
||
const depthMode = DepthMode.disabled; | ||
const stencilMode = StencilMode.disabled; | ||
const colorMode = ColorMode.alphaBlended; | ||
const id = '$debug_ss_rect'; | ||
// convert pixel-space coordinates to gl coordinates | ||
const glWidth = 2 * browser.devicePixelRatio * width / painter.width; | ||
const glHeight = 2 * browser.devicePixelRatio * -height / painter.height; | ||
const glX = (2 * browser.devicePixelRatio * x / painter.width - 1); | ||
const glY = -1 * (2 * browser.devicePixelRatio * y / painter.height - 1); | ||
|
||
program.draw(context, gl.TRIANGLES, depthMode, stencilMode, colorMode, CullFaceMode.disabled, | ||
debugSSRectUniformValues(color, [glX, glY], [glWidth, glHeight]), id, | ||
painter.viewportBuffer, painter.quadTriangleIndexBuffer, painter.viewportSegments); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
function drawDebug(painter: Painter, sourceCache: SourceCache, coords: Array<OverscaledTileID>) { | ||
for (let i = 0; i < coords.length; i++) { | ||
drawDebugTile(painter, sourceCache, coords[i]); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
attribute vec2 a_pos; | ||
|
||
uniform vec2 u_offset; | ||
uniform vec2 u_dim; | ||
|
||
void main() { | ||
gl_Position = vec4((a_pos*u_dim)+u_offset, 0, 1); | ||
} |
There was a problem hiding this comment.
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.