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

add clamp for extreme lats #124

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

add clamp for extreme lats #124

wants to merge 7 commits into from

Conversation

peterqliu
Copy link

@peterqliu peterqliu commented May 1, 2019

motivation: mapbox/mapbox-gl-js#8201

This PR assumes that expected behavior for features outside +/-85.05 latitude will be clamped at the respective extrema -- visually, that would be at the top and bottom edges of the Mercator map.

In mapbox-gl-js, each tile is rendered 0<= coord < 8192 in both directions. The upper bound is exclusive, to avoid overdraw with the tiles immediately south and east of it. This works well for most tiles, except the bottom row of any zoom level -- they lack a southerly neighbor to take over the feature right at 8192.

The tile rendering is structured to be agnostic of the tile's own tileCoordinates, which rules out conditional logic for rendering that very bottom edge.

Given these constraints, this (deeply inelegant) PR clamps incoming latitudes such that extremely northerly features render right at the map's top, and southerly ones no more than y = 8191 of the bottom tile row of any zoom level.

This ensures that those southerly features get rendered (albeit 1/8192 of a tile height too high), and additionally prevents the enclosed sin function from projecting features back toward the equator as they approach even more extreme latitudes.

@peterqliu peterqliu changed the title add clamp for lats beyond +/-90 add clamp for extreme lats May 2, 2019
src/convert.js Outdated
@@ -2,6 +2,9 @@
import simplify from './simplify';
import createFeature from './feature';

const maxMercatorLatitude = 85.05;
const minVectorLatitude = -maxMercatorLatitude * 8191/8192;

Choose a reason for hiding this comment

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

Why 8191/8192? Probably should add a comment explaining this

Copy link
Author

Choose a reason for hiding this comment

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

👍 will add comment if we end up going this route. for now, it's explained in the PR text

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

I'll try to see if there's a less hacky way to fix the inclusive bottom boundary. We should clamp against max latitude anyway though — I'm 👍 on that in any case.

src/convert.js Outdated
const y2 = 0.5 - 0.25 * Math.log((1 + sin) / (1 - sin)) / Math.PI;
return y2 < 0 ? 0 : y2 > 1 ? 1 : y2;
return y2;
Copy link
Member

@mourner mourner May 2, 2019

Choose a reason for hiding this comment

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

Nit: If we do the 8191 / 8192 hack, I think it should follow after the projection (not before). Currently (looking at tests) the clamp is 1px off (3094 instead of 3095). E.g. we do return Math.max(y2, 8191 / 8192);

Copy link
Author

Choose a reason for hiding this comment

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

return Math.max(y2, 8191 / 8192)

@mourner would this clamp features on every tile locally to 8191?

Copy link
Member

Choose a reason for hiding this comment

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

@peterqliu no, since this is a global projection — it projects latitude into 0..1 range (0 is near the north pole, 1 is near the south one). Tile-aware projection happens further down the line

@asheemmamoowala asheemmamoowala changed the base branch from master to main June 19, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants