Conversation
src/projection.js
Outdated
There was a problem hiding this comment.
This will bypass our normalization of the materialized Projection object, instead passing through a D3 projection as-is. Is that what we want? If so, I guess that means that the Projection type needs to be relaxed to also allow D3 projections.
src/projection.js
Outdated
| if (projection == null) return; | ||
| } | ||
|
|
||
| const type = projection; // save before namedProjection overwrites it |
There was a problem hiding this comment.
This means that the type won’t be normalized; for example, the projection name MeRcaToR should normalized to mercator.
src/projection.js
Outdated
| }, | ||
| ...(projection.invert && { | ||
| invert([x, y]) { | ||
| return projection.invert([(x - tx) / (k ?? 1), (y - ty) / (k ?? 1)]); |
There was a problem hiding this comment.
The asymmetry between invert and apply is worrisome. Should we be using transform.invert here instead? Or should be inline the translate and scale in apply instead?
src/projection.js
Outdated
| }, | ||
| apply([x, y]) { | ||
| let result = null; | ||
| projection.stream(transform.stream({point: (x, y) => void (result = [x, y])})).point(x, y); |
There was a problem hiding this comment.
And why not call projection([x, y]) here?
mbostock
left a comment
mbostock
left a comment
There was a problem hiding this comment.
Okay, I have pared back this PR by avoiding the introduction of a new “materialized projection” interface (i.e., Projection), instead exposing the existing ProjectionImplementation interface which was already exposed as context.projection.
This avoids the challenge of trying to normalize a provided ProjectionImplementation into a Projection (noted above). More importantly, it ensures when you reuse a projection across a plot, the projections behave identically rather than rescaling the projection to fit the dimensions of the other plot. This mirrors the current behavior of plot.scale (with respect to scales), and I believe is what we want. If you want to share the same projection type (or some other partial specification of a projection), then you should simply splat your projection options into multiple plots; you don’t need plot.scale to help with that.
All that said, I’d still like to expose a projection.apply and projection.invert convenience method, since that seems useful for implementing interaction. And in the case that the provided projection implementation is a function and exposes projection.invert, we can simply use that as-is. I’ll work on that next…
2 participants
plot.scale("projection")exposes the materialized projection from a plotThe returned object exposes the resolved projection options, reflecting the actual values used to construct the projection, making it possible to create a new plot with the same projection. The projection object also exposes an apply method that projects a [longitude, latitude] point to [x, y] pixel coordinates, as well as an invert method.
closes #1191
supersedes #2038