-
Notifications
You must be signed in to change notification settings - Fork 184
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
base: main
Are you sure you want to change the base?
Conversation
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.
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.
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.
Very nice!
src/transforms/centroid.js
Outdated
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)), |
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.
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.
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 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 :)
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). |
@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 |
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. |
I also just released polylabel 2.0.1 that saves some bytes, worth upgrading. |
I think if it’s a bundled dependency it’s okay. |
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 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). |
const context = { | ||
arc() {}, | ||
moveTo(x, y) { | ||
ring = [[x, -alpha * y]]; | ||
}, | ||
lineTo(x, y) { | ||
ring.push([x, -alpha * y]); | ||
}, | ||
closePath() { | ||
ring.push(ring[0]); | ||
if (polygonArea(ring) > 0) polygons.push([ring]); | ||
else holes.push(ring); | ||
} | ||
}; | ||
const path = geoPath(projection, context); |
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.
Tested to work with #2252:
const context = { | |
arc() {}, | |
moveTo(x, y) { | |
ring = [[x, -alpha * y]]; | |
}, | |
lineTo(x, y) { | |
ring.push([x, -alpha * y]); | |
}, | |
closePath() { | |
ring.push(ring[0]); | |
if (polygonArea(ring) > 0) polygons.push([ring]); | |
else holes.push(ring); | |
} | |
}; | |
const path = geoPath(projection, context); | |
const path = context.path().context({ | |
arc() {}, | |
moveTo(x, y) { | |
ring = [[x, -alpha * y]]; | |
}, | |
lineTo(x, y) { | |
ring.push([x, -alpha * y]); | |
}, | |
closePath() { | |
ring.push(ring[0]); | |
if (polygonArea(ring) > 0) polygons.push([ring]); | |
else holes.push(ring); | |
} | |
}); |
channels: { | ||
x: {value: X, scale: projection == null ? "x" : null, source: null}, | ||
y: {value: Y, scale: projection == null ? "y" : null, source: null} | ||
} |
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.
channels: { | |
x: {value: X, scale: projection == null ? "x" : null, source: null}, | |
y: {value: Y, scale: projection == null ? "y" : null, source: null} | |
} | |
channels: {x: {value: X, scale: null, source: null}, y: {value: Y, scale: null, source: null}} |
also after #2252
const getG = memoize1((data) => valueof(data, geometry)); | ||
return initializer( | ||
{...options, x: null, y: null, geometry: {transform: getG}}, | ||
(data, facets, channels, scales, dimensions, {projection}) => { |
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.
(data, facets, channels, scales, dimensions, {projection}) => { | |
(data, facets, channels, scales, dimensions, context) => { |
After #2252
Mathematicians might call it the Chebyshev center of the polygon, but not quite since we're scaling the polygon first. Maybe center is the right name for this. Allows to be vague about it as "a suitable center for some applications". Another good name if we want to be less hand-wavy is incenter, as a generalization of the triangle incenter. |
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:
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:
cc: @mourner
demo: https://observablehq.com/@observablehq/poi-polylabel-transform
closes #1587