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.

Sign up for GitHub

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

[projections] Adaptive geometry resampling for alternative projections #10753

Merged
merged 2 commits into from
Jun 8, 2021
Merged

[projections] Adaptive geometry resampling for alternative projections #10753

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2eac67
implement adaptive resampling of reprojected geometry
mourner Jun 7, 2021
534a911
address feedback
mourner Jun 8, 2021
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
112 changes: 64 additions & 48 deletions src/data/load_geometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@
import {warnOnce, clamp} from '../util/util.js';

import EXTENT from './extent.js';
import MercatorCoordinate from '../geo/mercator_coordinate.js';
import getProjection from '../geo/projection';
import assert from 'assert';

import type Point from '@mapbox/point-geometry';
import {lngFromMercatorX, latFromMercatorY} from '../geo/mercator_coordinate.js';
import getProjection from '../geo/projection/index.js';
import Point from '@mapbox/point-geometry';

// These bounds define the minimum and maximum supported coordinate values.
// While visible coordinates are within [0, EXTENT], tiles may theoretically
Expand All @@ -23,21 +21,22 @@ export function setProjection(projectionName) {
projection = getProjection(projectionName);
}

function resample(ring) {
if (ring.length === 0) return;
const result = [];
result.push(ring[0]);
for (let i = 1; i < ring.length; i++) {
const last = result[result.length - 1];
const p = ring[i];
const d = p.dist(last);
const m = 16;
for (let i = m; i < d; i += m) {
result.push(last.add(p.sub(last).mult(i / d)));
}
result.push(p);
function clampPoint(point: Point) {
const {x, y} = point;
point.x = clamp(x, MIN, MAX);
point.y = clamp(y, MIN, MAX);
if (x < point.x || x > point.x + 1 || y < point.y || y > point.y + 1) {
// warn when exceeding allowed extent except for the 1-px-off case
// https://github.com/mapbox/mapbox-gl-js/issues/8992
warnOnce('Geometry exceeds allowed extent, reduce your vector tile buffer size');
}
return result;
return point;
}

function pointToLineDist(px, py, ax, ay, bx, by) {
const dx = ax - bx;
const dy = ay - by;
return Math.abs((ay - py) * dx - (ax - px) * dy) / Math.hypot(dx, dy);
}

/**
Expand All @@ -48,40 +47,57 @@ function resample(ring) {
*/
export default function loadGeometry(feature: VectorTileFeature, canonical): Array<Array<Point>> {
if (!canonical) return [];
Copy link
Contributor

Choose a reason for hiding this comment

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

In my PR, I had to delete this to get unit tests to pass. Instead of returning an empty array, we need to retain the original logic if canonical is undefined/null and avoid reprojecting unnecessarily. We can refactor reproject to do this without changing the rest of the logic in loadGeometry.

function reproject(p, featureExtent, canonical) {
    let x, y;
    if (canonical) {
        const cs = projection.tileTransform(canonical);
        const z2 = Math.pow(2, canonical.z);
        const lng = lngFromMercatorX((canonical.x + p.x / featureExtent) / z2);
        const lat = latFromMercatorY((canonical.y + p.y / featureExtent) / z2);
        const projectedLngLat = projection.project(lng, lat);
        x = (projectedLngLat.x * cs.scale - cs.x) * EXTENT;
        y = (projectedLngLat.y * cs.scale - cs.y) * EXTENT;
    } else {
        const scale = EXTENT / featureExtent;
        x = Math.round(p.x * scale);
        y = Math.round(p.y * scale);
    }

    return new Point(x, y);
}


const cs = projection.tileTransform(canonical);
const reproject = (p, featureExtent) => {
const s = Math.pow(2, canonical.z)
const x_ = (canonical.x + p.x / featureExtent) / s;
const y_ = (canonical.y + p.y / featureExtent) / s;
const l = new MercatorCoordinate(x_, y_).toLngLat();
const {x, y} = projection.project(l.lng, l.lat);
p.x = (x * cs.scale - cs.x) * EXTENT;
p.y = (y * cs.scale - cs.y) * EXTENT;
};
const scale = EXTENT / EXTENT;
const z2 = Math.pow(2, canonical.z);
const featureExtent = feature.extent;

function reproject(p) {
const lng = lngFromMercatorX((canonical.x + p.x / featureExtent) / z2);
const lat = latFromMercatorY((canonical.y + p.y / featureExtent) / z2);
const {x, y} = projection.project(lng, lat);
return new Point(
(x * cs.scale - cs.x) * EXTENT,
(y * cs.scale - cs.y) * EXTENT
);
}

function addResampled(resampled, startMerc, endMerc, startProj, endProj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These two functions don't need to be declared in the loadGeometry method

Copy link
Member Author

Choose a reason for hiding this comment

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

It was initially a closure because it captures canonical, featureExtent and cs (tile transform), all of which would have to be moved to arguments in a quite a verbose way if we move it out. We could do it but it's also not a bottleneck.

const midMerc = new Point((startMerc.x + endMerc.x) / 2, (startMerc.y + endMerc.y) / 2);
const midProj = reproject(midMerc, feature.extent);
const err = pointToLineDist(midProj.x, midProj.y, startProj.x, startProj.y, endProj.x, endProj.y);

if (err >= 1) {
// TODO make sure we never reach max call stack
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: TODO comments cause lint to fail 🤷

addResampled(resampled, startMerc, midMerc, startProj, midProj);
addResampled(resampled, midMerc, endMerc, midProj, endProj);
} else {
resampled.push(clampPoint(endProj));
}
}

const geometry = feature.loadGeometry();

for (let r = 0; r < geometry.length; r++) {
let ring = geometry[r];
ring = resample(ring);
geometry[r] = ring;
for (let p = 0; p < ring.length; p++) {
let point = ring[p];
// round here because mapbox-gl-native uses integers to represent
// points and we need to do the same to avoid rendering differences.
//const x = Math.round(point.x * scale);
//const y = Math.round(point.y * scale);
reproject(point, feature.extent);
const {x, y} = point;

point.x = clamp(x, MIN, MAX);
point.y = clamp(y, MIN, MAX);

if (x < point.x || x > point.x + 1 || y < point.y || y > point.y + 1) {
// warn when exceeding allowed extent except for the 1-px-off case
// https://github.com/mapbox/mapbox-gl-js/issues/8992
warnOnce('Geometry exceeds allowed extent, reduce your vector tile buffer size');
const ring = geometry[r];
const resampled = [];

for (let i = 0, prevMerc, prevProj; i < ring.length; i++) {
const pointMerc = ring[i];
const pointProj = reproject(ring[i]);

if (i === 0 || feature.type === 1) {
resampled.push(clampPoint(pointProj));
} else {
addResampled(resampled, prevMerc, pointMerc, prevProj, pointProj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Flow doesn't like this because it thinks that addResampled can be called with prevMerc and prevProj as undefined variables. I added if (!startMerc || !startProj) return; at the start of addResampled to get around that but maybe there's a better way.

}

prevMerc = pointMerc;
prevProj = pointProj;
}

geometry[r] = resampled;
}

return geometry;
}