curve: "auto" (line + projection) #1156
Conversation
Fil
force-pushed
the
fil/line-interpolate-geodesic
branch
from
0b99d4e to
da061c1
Compare
There was a problem hiding this comment.
I think we can avoid creating an entirely new Mark class for this. Basically, I would check whether the curve is "geodesic" in the constructor, and if so, don’t bind the x and y channels to their respective scales. Then use the projection and geoPath instead of the line shape during render.
In addition to being less code, this should be more maintainable; less likely that the two implementations could drift. And of course you could test instanceof Line and you’d get the expected result. (It’s highly surprising if calling Plot.line doesn’t return an instance of Plot.Line.)
I think I would also prefer the simpler name "sphere". I know that "geodesic" is the mathematically-precise term, but wouldn’t "sphere" be easier to remember? And I don’t think we’ll have multiple forms of spherical interpolation.
src/marks/line.js
Outdated
|
|
||
| // When the line mark is used with a "sphere" curve and a projection | ||
| // (supposedly spherical), keep x and y as numbers. | ||
| const sphere = curve === "sphere"; |
There was a problem hiding this comment.
You’ll need to coerce curve to a string and toLowerCase etc.
src/marks/line.js
Outdated
| path({ | ||
| type: "LineString", | ||
| coordinates: Array.from(I, (i) => [+X[i], +Y[i]]).filter(([x, y]) => !isNaN(x + y)) | ||
| }); |
There was a problem hiding this comment.
We could do this faster by invoking the projection stream directly (calling stream.lineStart, stream.point, stream.lineEnd) rather than going through geoPath.
Also, this will coerce nulls to zero rather than skipping them. We might want to use coerceNumbers here, but we should at least be consistent.
There was a problem hiding this comment.
groupIndex already does filtering for undefined values, so we can check if i is -1 here instead. I’ll push some commits.
mbostock
force-pushed
the
fil/line-interpolate-geodesic
branch
from
15a7425 to
12bb0f1
Compare
|
I reverted fc75219 since that seems unrelated (and there are no tests so I’m not sure what it was trying to fix). I’ve also renamed it to curve |
|
The use case for fc75219 were the x/y coordinates passed as strings in the beagle.js example. Now solved by coerceNumbers in sphereLine. |
| x: {value: x, scale: curve === curveProjection ? undefined : "x"}, // unscaled if projected | ||
| y: {value: y, scale: curve === curveProjection ? undefined : "y"}, // unscaled if projected |
Fil
changed the title
* curve: "geodesic" for Plot.line closes observablehq#1146 * Line handles the "sphere" curve to follow great circles on spherical projections. * fix filtering, input normalization * Update README * curve: "projection" Co-authored-by: Mike Bostock <[email protected]>
closes #1146