-
Notifications
You must be signed in to change notification settings - Fork 185
projection.clip option #1151
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
projection.clip option #1151
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.
The projection.clipAngle option seems good.
I would not change the default projection.domain option based on the presence of the projection.clipAngle option. That is not documented here and seems surprising. In general I think it’s better if there are fewer interactions between options; I don’t see a strong reason to require such shorthand here when this option will be relatively rare. (Especially if you also intend to support the "sphere"
shorthand for projection.domain, which I also think is probably overkill… but also doesn’t have much harm, so maybe okay.)
I am against adding a geoClipRectangle to all projections. I understand the rationale but I think the fact that it is not configurable will be highly frustrating in some cases. There are legitimate use cases to not have it and the workaround for negative margins is not easily discovered (intuitive). This should be an option on the projection (clip: "frame"
presumably). I could also see wanting it to be an option on a mark like the existing mark.clip option, but that might be overkill. I’m not sure yet whether it should be on by default.
I guess this makes me wonder if we should use projection.clip of "23 deg"
(or probably just the number 23
?) instead of the projection.clipAngle option, and "frame"
when we want to clip to the frame, and null
to disable entirely. It’s rare that you need both forms of clipping simultaneously (and you could always do it with a custom projection if necessary).
At first I made it an option on the projection, then added the "sphere" option, then realized it was too much. It can definitely be an option on the projection and it also makes total sense to use projection clipping rather than svg clipping when it is set as an option on the mark. If it's going to be opt-in (and not a default, as I thought it should be), it's a non-blocking enhancement, and we can release without this PR (clipAngle can wait too). Gives us some breathing time to fine-tune the approach. I like the idea that clip: 23 (a number) calls clipAngle. |
If it’s a mark option, it should be two different options, though—or two different values of the same mark.clip option. Meaning I don’t think we should change the existing _mark.clip |
I agree we should decide whether projection clipping to the frame is the default. That decision is a blocker for release. I think I am okay with it being on by default as long as you can turn it off via the projection.clip option (or change to a great circle etc.). |
I want it on by default: overlapping facets just look too bad. I'll give it another shot. |
3892bd6
to
7732410
Compare
lgtm! |
* add 110m for faster tests * projection.clipAngle * always clipExtent(frame) * projection fit & clip tests * two techniques for bleed-edges maps * Update README * Update README * Update README * fix tests (clipAngle => clip) * fix warning Co-authored-by: Mike Bostock <[email protected]>
Introduces two projection clipping methods:
New option: projection.clipAngle
a number, passed directly to the projection factory. This also forces fitting the projection to the frame with its restricted sphere path.
Always clip-to-frame
Systematically applies the clipRectangle post-clip stream transform. Benefits:
Note that this is a lighter, less configurable version of #1150:
The only drawback I've seen so far is that you can't have a "bleed edges" projection, where the interpolation of a polygon along the sphere is hidden by the frame. However, this can be approximated with negative margins, or a white stroke Plot.frame if you need facets (see examples in tests).
Since this changes almost all the test maps, I'm taking this as an opportunity to switch most of them from 50m to 110m geometries, in order to make the tests a bit faster, and the repo a bit smaller.
closes #1137
closes #1131
supersedes #1132 and #1150