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

A transform that uses @mapbox/polylabel to derive “pois” #2098

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jul 5, 2024

The poi/polylabel transform guarantees that the selected point belongs to the interior of the polygon or multipolygon. For line and point geometries, it defers to the classic centroid.

This PR applies the transform by default, as an almost always superior alternative to Plot.centroid.

For most geometries the change is almost imperceptible. Some geometries get a much better treatment:

  • USA: gets located somewhere near Nebraska — whereas its centroid might be in Canada, and its geoCentroid close to the Canadian border.
  • France: gets located around Paris — whereas its geoCentroid is in Spain, its centroid (in Mercator projection) in the Bay of Biscay.
  • Antarctica, and (to a lesser point) Russia — highly dependent on the projection, but generally better than the centroid, esp. with sphere clipping (and much better than the geoCentroid in most projections).
  • South Africa gets a point to the west of the country, further from Lesotho.
  • Chile might get an inferior treatment, as its poi sometimes gets located very far south where the country widens a little.

For the few specific projections that require clipping to sphere, make sure you use their versions from d3-geo-polygon rather than from d3-geo-projection.

TODO:

  • I absolutely want to find a better name than "poi", but I don't think "polylabel" is the correct choice because this is not only for labels, and also it's not the raw polylabel (it supports multipolygons, projections, and uses an ellipse rather than a circle).
  • documentation / API.md

cc: @mourner

demo: https://observablehq.com/@observablehq/poi-polylabel-transform

@Fil Fil requested a review from mbostock July 5, 2024 15:38
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Adding a dependency is a bummer. How big is it? Maybe we can pass the implementation in instead? Most plots won’t need this dependency.

Copy link

@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.

Very nice!

path(G[i]);
for (const h of holes) polygons.find(([ring]) => polygonContains(ring, h[0]))?.push(h);
const a = greatest(
polygons.map((d) => polylabel(d, 0.01)),
Copy link

Choose a reason for hiding this comment

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

One thing to be careful about is picking the right precision for the input data — too small, and it will be slow, too big, and results will be off. E.g. if this is meant to work on screen coordinates, it may be too small and 1 (pixel) would be good enough.

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 is happening in pixel coordinates; I thought it would be “better” to use a number below 1 (so we can have sub-pixel precision), but maybe 0.1, 0.5 or even 1 is fine. But I haven't seen much of a concrete difference between 1 and 0.01 anyway :)

@Fil
Copy link
Contributor Author

Fil commented Jul 5, 2024

How big is it?

The dependencies are:

If we copy and embed the code, the total size of grows by 1.8% (from 65715 to 66904 bytes, size of the gziped version of the minified script). There might be ways to reduce this a bit if we embed it, since we might not need all of it (though @mourner's code is usually quite minimalist).

@mourner
Copy link

mourner commented Jul 6, 2024

@Fil let me know if there's anything I can do on the packages side to make including them as a dependency more viable and reduce the bundle size. Looking at the code, there doesn't seem to be anything superfluous there. Only need to bump tinyqueue to export ESM properly (edit: did so).

@Fil
Copy link
Contributor Author

Fil commented Jul 6, 2024

I was not clear in my remark. The cost of 1200 bytes is for the whole feature, including the two imports and my glue code. There is no difference between copying the source code or importing from the dependency, since it's part of the bundle (only d3 modules are external dependencies). We could cull a few bytes by copying the code and trimming it down to the absolute minimum (e.g. removing the debug log), and maybe there is a way to make Plot.centroid and this transform share more code.

Also, an alternative way of approaching this would be to make it part of d3-geo.

And I'll follow @mourner's suggestion to set the precision to 1 pixel.

@mourner
Copy link

mourner commented Jul 6, 2024

I also just released polylabel 2.0.1 that saves some bytes, worth upgrading.

@mbostock
Copy link
Member

mbostock commented Jul 6, 2024

I think if it’s a bundled dependency it’s okay.

@Fil
Copy link
Contributor Author

Fil commented Jul 7, 2024

Do you think peak could be a good name for this transform? (Instead of poi)

I've also considered wrapping this under the Plot.centroid transform, by adding a "derive" or "interpolate" option that would be similar to Plot.raster’s interpolate option. I have mixed feelings as this abuses the "centroid" word a little (not a center of mass anymore), but it could be seen as a generalization. The signature of that function might be (G, X, Y, projection) => void or (G, projection) => [X, Y] or (G, path) => [X, Y]

Also for more generality, the parameter describing the ellipse should be a vector (length = alpha ratio, angle = 0) — but this can be added later if requested (useful for people who would want to add labels at a 45° angle).

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