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

[Obsolete] d3.geoPolyhedral() now uses d3-geo-polygon if the plugin is available. #130

Closed
wants to merge 7 commits into from

Conversation

Fil
Copy link
Member

@Fil Fil commented Oct 1, 2017

I have been as conservative as possible: d3-geo-projection works as usual without d3-geo-polygon, and checks that it has projection.preclip (d3-geo > 1.8.1).

Examples and issues at d3-geo-polygon

Solves #129
Solves #124
Solves d3/d3-geo#108
Solves #86

Most of the credit goes to @jasondavies @mbostock and @espinielli

Note that d3-geo-polygon is still unstable. When we stream points that belong to the clipping polygon itself, it sometimes bleeds out -- for example on the Waterman projection with a default aspect -- rotate [0,0]).

I have been as conservative as possible: d3-geo-projection works as usual without d3-geo-polygon, and checks that it has projection.preclip (d3-geo > 1.8.1).

Examples and issues at [d3-geo-polygon](https://github.com/d3/d3-geo-polygon)

Solves #129
Solves #124
Solves d3/d3-geo#108
Solves #86
@Fil Fil requested a review from mbostock October 1, 2017 22:19
@espinielli
Copy link

IMHO no credit to me but for watching what you were doing...well done, @Fil ! Super professional.

@mbostock
Copy link
Member

mbostock commented Oct 2, 2017

test/compare-images should be testing the default configuration of the projections; we shouldn’t change it to use d3.geoClipAntimeridian if the default configuration is to use d3.geoClipPolygon. But it seems that d3.geoClipPolygon isn’t quite ready for use yet because of the equator issue?

@Fil
Copy link
Member Author

Fil commented Oct 2, 2017

Indeed this is to make sure the test runs with the default configuration, which is still to use d3.geoClipAntimeridian.

But when d3-geo-polygon is available[1], d3.geoPolyhedral switches to "experimental" mode and uses it, and that's not what we want to test here. However, as we must to list the optional dependency d3-geo-polygon in d3-geo-projection/package.json (otherwise the code simply doesn't "compile"), when we run the tests the plugin kicks in, and I have to explicitly disable it.

As discussed in #129, we could duplicate geoPolyhedral to d3-geo-polygon and not have to disable clipPolygon when we run the tests. But it would carry a lot of code with it, so not a good idea.

Is d3.geoClipPolygon ready to use? I would say yes: it already allows so many new maps that were simply not possible before. Lee tetrahedron, Dymaxion, etc., run with no issue. Cahill-Keyes has the equator problem too, but it can be solved temporarily by adding a small gamma rotation.

It just happens that one of these cases where d3-geo-polygon gets lost, is when everything is perfectly aligned, ie the default [0,0] aspect of geoPolyhedralWaterman and the like. Frustrating but IMV not blocking. Especially since nothing changes at all (results and API) when d3-geo-polygon is not explicitly added by the user.

[1] My first implementation added a parameter to the polyhedral() function that would explicitely ask for clipPolygon: useless bloat.

@Fil
Copy link
Member Author

Fil commented Nov 16, 2017

If we asked to explicitly set a global flag (in whatever form we want) for when people want to use the experimental feature, the default tests would run without anything specific. Would this be better?

@Fil
Copy link
Member Author

Fil commented Feb 20, 2018

It took me many hours of debugging, and the fix was so tiny. Good that we're not paid by the line.

“This branch cannot be rebased due to conflicts” => I've created a new one at #132

@Fil Fil closed this Feb 20, 2018
@Fil Fil changed the title d3.geoPolyhedral() now uses d3-geo-polygon if the plugin is available. [Obsolete] d3.geoPolyhedral() now uses d3-geo-polygon if the plugin is available. Feb 20, 2018
@Fil Fil deleted the polygon-clipping-for-polyhedral branch March 11, 2018 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants