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

Merge and adapt Jason Davies’ clip-polygon branch from d3v3 to v4 #108

Closed
wants to merge 9 commits into from
Closed

Merge and adapt Jason Davies’ clip-polygon branch from d3v3 to v4 #108

wants to merge 9 commits into from

Conversation

Fil
Copy link
Member

@Fil Fil commented Jul 24, 2017

Fixes #46

Original code: https://github.com/d3/d3/commits/clip-polygon

See #46
and d3/d3-geo-projection#86 (comment)

I went back to the original naming:
 - clip/polygon.js defines clipPolygon()
 - clip/rejoin.js defines clipPolygonRejoin()

A test block: https://bl.ocks.org/Fil/cb7930254c9e7062114678d62d9be5ac

Documentation, tests and examples of projections will come at a later stage.

Thanks to:
- Jason Davies @jasondavies for the genius algorithm and implementation
- Mike Bostock @mbostock for the explanations and guidance
- Enrico Spinielli @espinielli for the Furuti and Cahill-Keyes projections
@Fil
Copy link
Member Author

Fil commented Jul 25, 2017

There is still a weird issue: sometimes a part of the drawing will not be rendered.

In https://bl.ocks.org/Fil/cb7930254c9e7062114678d62d9be5ac, the outline of the Sphere will sometimes be half cut (varying between reloads).
furuti
furuti-half

In https://bl.ocks.org/Fil/a5c101787b70c93ef2455159f9fd26cb the whole world will either be totally drawn, or totally blank.
tetra
tetra-half

@mbostock
Copy link
Member

I have a vague recollection of a discussion with Jason regarding resorting the points along the polygon edge after clipping… I wonder if that is the issue here. Sorry, just speculation.

@espinielli
Copy link

I just cloned the Furuti3 and made it minimalistic in order to better see what the issue is with the cuts...
See https://bl.ocks.org/espinielli/b37aa550ce21c431e22722d1e17d5129

If everything is ok you get
screen shot 2017-08-04 at 9 26 02 pm

otherwise
screen shot 2017-08-04 at 9 25 47 pm

Refresh a few times to get at least one of both...

I also noted some line "spikes"
screen shot 2017-08-04 at 9 44 16 pm

@Fil
Copy link
Member Author

Fil commented Aug 4, 2017

[EDIT: OBSOLETE COMMENT] I think I might have found the cause in
d3/d3-geo-projection#107 (comment)
(up-to-date block at https://bl.ocks.org/Fil/c36ed66a4d50d77150712c80642a78d5 ).

It looks like the issue was caused by the sphere being clipped by the outline — which also represents the sphere, except it's computed by joining the faces not by running sphereStream. So we had two "almost identical" polygons, and some random crept in when comparing epsilons. Having a "clipPolygon sphere" using a much smaller epsilon than the sphereStream seems to solve it (or, almost solve it), in that the clipPolygon contains the sphereStream.

Not 100% sure yet though.

@Fil
Copy link
Member Author

Fil commented Aug 4, 2017

OK the solution is much simpler than playing with epsilons. It's only a case of not clipping the sphereStream.

I have pushed the code to http://blockbuilder.org/Fil/c36ed66a4d50d77150712c80642a78d5 and http://blockbuilder.org/Fil/cb7930254c9e7062114678d62d9be5ac and both seem 100% solved.

the solution is: save the clipPolygon(), remove it (with clipAngle 180), stream the sphere, then set the clipPolygon again — just like we do for rotate().

    	clipPolygon = proj.clipPolygon(),
        rotateStream = stream_(stream),
        sphereStream = (proj.rotate([0, 0]).clipAngle(180), stream_(stream));
    proj.rotate(rotate).clipPolygon(clipPolygon);

I'll push it to the PR tomorrow. Good night!

@espinielli
Copy link

Looking forward to trying it out!

Fil added a commit to Fil/d3-geo-projection that referenced this pull request Aug 5, 2017
Fil added a commit to Fil/d3-geo-projection that referenced this pull request Aug 5, 2017
@espinielli
Copy link

I was playing with d3-geo as from @Fil 's bl.ocks.
I aim at doing "Earth in an Icosahedron" (first simple, then a Jak van Wijk format as pictured in his myriahedral paper and in Furuti's site).

So this bl.ock
is the simple one and I almost got there:

almost

I found a couple of surprises, and only (sigh!) one solution.
Here they are:

  1. the first is due to longitudes not being in [-180, 180) range.
    The solution to the last one is either as in L24 of map.js
    or a similar treatment in
    L1312 of geo.js.
    If such a measure is not taken the result is

    mess, or art?

    Is it a requirement of d3-geo to have lon in -180/180 (and lat in -90/90)?

  2. one that I do not know where to look for...and leaves me with

preview

Maybe it is related to clipping on Sphere...

@Fil
Copy link
Member Author

Fil commented Aug 19, 2017

For [1] it appears to be one of the bugs in polygonContains (#105 (comment)).

For [2] the polyhedral Sphere w/ clipPolygon, something is still missing. I had a similar issue with Waterman for example. In this case it seems we can solve it by introducing clipNone which would do neither clipAngle nor clipAntimeridian; quickly hacked here:
http://blockbuilder.org/Fil/b8f5c05efd57b4b8fb653ed0796bdada

@espinielli
Copy link

Thank you now I got JvW's foldout:

preview

@Fil
Copy link
Member Author

Fil commented Aug 21, 2017

I pushed clipNone and a sort of API to call it with .clipAngle(-1)(see icosaedron).

However we still have a problem to solve: if we use world-atlas/110m or 50m, and use rotate([0,0]), clipPolygon fails totally. I made a test bed to show this here:
http://blockbuilder.org/Fil/cb787b39e229f0016728ba51e018b5e7
just adapt the topojson source and the rotation parameter.

Strangely world-110m in Enrico's block doesn't have this problem. So I think we're close (but maybe we just opened a new can of worms!).

Note that when this is solved (currently, when using a small rotation), we can also dispense with the SVG or canvas clipping and use d3-geo clipPolygon instead: progress!

@Fil
Copy link
Member Author

Fil commented Aug 21, 2017

The most visible difference between the topojson files is that, in Enrico's world-110m, mainland Russia is made of two polygons (one East of the antimeridian line, one West); in the case of world-atlas, it's one polygon that wraps around.

@espinielli
Copy link

espinielli commented Aug 22, 2017

Opppss! Where did I get world110m from? maybe it is one of the older versions...
Anyway, it allowed you to spot an issue and perfect even more the code: well done @Fil !!!!!

@Fil
Copy link
Member Author

Fil commented Aug 22, 2017

I think I'm done here; the question is what API design.

Hacking clipAngle(-1) as I did is probably not sane, and we want to be able to read both the clipPolygon and clipAngle current values, keeping in mind that up to now "no clipAngle" meant "antimeridian cutting".

clipPolygon is not a breaking change (everything still works the same in d3-geo-projection.v2) but it's certainly worth a version change so that d3-geo-projection can require it when we introduce new polyhedral projections.

@mbostock
Copy link
Member

mbostock commented Aug 24, 2017

My ideal API would be projection.clip(clip) where clip is an object so some variety that controls the clipping method (implementation) and any parameters. So for example:

projection.clip(d3.geoClipAntimeridian); // replaces projection.clipAngle(null)
projection.clip(d3.geoClipCircle(90)); // replaces projection.clipAngle(90)
projection.clip(d3.geoClipPolygon()); // replaces projection.clipPolygon(…)
projection.clip(null); // disables clipping entirely?

With the new “pipeline” design I was hoping to have clipping be extensible, but I think it’d be fine to punt on that extensibility for now. So in other words only a fixed number of clip implementations would be supported (none, antimeridian, circle, polygon), and if you tried to pass in an instance of something else it would throw an error. And likewise the clip instance wouldn’t have any public methods, other than maybe some way of inspecting what it represented.

@Fil
Copy link
Member Author

Fil commented Aug 24, 2017

+1 for keeping whatever API private for now, there are still dark corners in this area that need to be cleaned.

Also, currently pre-clipping is entangled with rotation, with no obviously compelling reason (the examples given are now all running smoothly with or without #90), so any API that we expose here would be quite complicated and possibly subject to change.

Would d3.geoClipAntimeridian return a descriptive JSON object or would it include the "clip function"?

In the first case, we could have { type: "Sphere"}, { type: "Antimeridian" }, { type: "Angle", angle: 90 }, { type: "Polygon", coordinates: [[…]] } — Antimeridian could arguably be a LineString, but unfortunately geoCircle() is not a GeoJSON citizen, so we can't really map the clipAngle to a standard GeoJSON object.

@mbostock
Copy link
Member

The most opaque way of doing it would be to just have a sentinel object. Something equivalent to:

var clipAntimeridian = {};

It’s a little different for the circle class, since the angle is configurable. So you might say similar to:

function ClipCircle(angle) {
  this.angle = angle;
}

And then in the d3-geo projection class, you’d say something like:

if (clip === clipAntimeridian) {
  doAntimeridianClipping();
} else if (clip instanceof ClipCircle) {
  doCircleClipping(clip.angle);
} else {
  etc();
}

You could do this with an object and a type string, but generally I prefer to use symbols as identifiers rather than strings.

@mbostock
Copy link
Member

OTOH, if we do decouple clipping from rotation, it’s tempting to make the clipping interface implement the projection stream interface—then you can pass in whatever implementation you want to clipping, and we don’t have to have a fixed set of implementations. (Note that this would still be less flexible than the imagined projection pipeline, as you’d only be able to control one part of a fixed pipeline.)

The downside of this approach would be that the clipping projection stream operates on coordinates in radians, which is inconsistent with the rest of the API. And I think switching it to operate in degrees for consistency would have a noticeable performance and code complexity cost… though, I haven’t looked into it much.

@Fil
Copy link
Member Author

Fil commented Aug 24, 2017

I feel this is taking us too far away from the original goal of this PR. The future stream/pipeline API is going to be a different and also very big effort — and it's only tangentially related to this. I'd prefer to land this small win now before opening the next.

Can we maybe just add clipPolygon() as it is, publicly use clipNone() as a proxy of clipAngle(-1), and call it a day?

It's not perfect, and I still don't like the logic in the setup of the Sphere stream in polyhedral https://github.com/Fil/d3-geo-projection/blob/clipPolygon/src/polyhedral/index.js#L105 — also I don't understand why polyhedralCollignon and polyhedralButterfly need clipPolygon(null), while Waterman doesn't.

@mbostock
Copy link
Member

I have doubts as to whether the proposed pipeline API will ever happen—it was an experiment from several years ago and I don’t currently have the bandwidth to revisit it. I’m not proposing we take that on now, and certainly not as part of this effort. What I was proposing was in #108 (comment) doesn’t seem too onerous to do now, and is cleaner than overloading clipAngle with -1, no?

@Fil
Copy link
Member Author

Fil commented Aug 24, 2017

I'll test that tomorrow with the various projections.

(And now I think I understand why polyhedralCollignon fails with clipPolygon, and it's not related to clipPolygon itself, but to the way polyhedral builds the sphere.)

@Fil
Copy link
Member Author

Fil commented Aug 25, 2017

This morning I researched the issue with the way the Sphere stream is set up in polyhedral, and found a way to fix it in polyhedral so that clipPolygon() finally works as intended in many projections.

If this holds, we won't need clipNone() or the clipAngle(-1) hack anymore. In that case it would mean that it is enough to introduce projection.clipPolygon(), and not touch the API any more than this.

So let's put this to rest & do more tests for a while. My current tests are listed at https://bl.ocks.org/Fil/fc967d8ac6aa246863c6f2a4b7dbe41c

Fil added a commit to Fil/d3-geo-projection that referenced this pull request Aug 25, 2017
… centroids — solves a lot of the issues at d3/d3-geo#108 (comment) and allows to use clipPolygon() properly on all polyhedral projections (tested to this date).
@mbostock
Copy link
Member

Thanks for your perseverance with this. Your tests are beautiful!

I’m happy with just adding projection.clipPolygon. As I understand it, there would still be three states:

  1. Antimeridian cutting - projection.clipAngle and projection.clipPolygon return null.

  2. Circle clipping - projection.clipAngle returns a number and projection.clipPolygon returns null.

  3. Polygon clipping - projection.clipAngle returns null and projection.clipPolygon returns the polygon coordinates array.

I think it might be cleaner to introduce a second new method, projection.clipAntimeridian, and use that to enable antimeridian cutting rather than passing null to projection.clipAngle. (Otherwise, we’d presumably need to allow passing null to projection.clipPolygon as a redundant way of enabling antimeridian cutting?) The projection.clipAntimeridian method would accept and return a boolean, and we’d deprecate allowing projection.clipAngle to receive null.

@mbostock
Copy link
Member

Is anyone willing to recreate Jason Davies’ approximation of Fuller’s cuboctahedron projection?

@Fil
Copy link
Member Author

Fil commented Aug 25, 2017

Will do — in fact projection.clipPolygon() already accepts null (or equivalently []), but in the current implementation it switches to clipNone rather than clipAntimeridian (but that's because the real clipping is done by geoPolyhedral).

My use case was to have a full graticule on Lee's projection — it's tricky because clipPolygon will remove the 180° meridian. But several alternatives can solve it: .clipPolygon([]), .clipAngle(180), and clipAngle(null) (line 246 of the example, just before path(graticule)).

@Fil
Copy link
Member Author

Fil commented Aug 25, 2017

Fuller's historical projection(s) are an interesting challenge:

It's probably not the scope of d3-geo-projection to reconstruct the major historical projections, but trying to do so is going to be very interesting, even for projections that are now considered obsolete. A discussion for d3-geo-projection, or maybe a separate project altogether?

@espinielli
Copy link

espinielli commented Aug 25, 2017

You can get the poster images of Mar 1943 Life's Dymaxion map from Google books, see pag 44 and onwards.
A couple of years ago, I did get (via Chrome dev tools) and print the relevant images. The Dymaxion World is on my desk at work since then...a nice start of a conversation with any guests!

bf

Yes reproducing these "esoteric" maps would be fun. (now that d3-geo-projection starts to provide the relevant infrastructure)

@espinielli
Copy link

Given the icosahedron's faces, Jason's Airocean layout can be obtained by splitting a couple of faces; something like:

  // Split relevant face at centroid. (index depends on how you build your icosahedron)
  var face = icosahedron[6],
      centroid = face.centroid;
      temp = face.slice();
  face[0] = centroid;
  icosahedron.push(face = [temp[0], centroid, temp[2]]);
  face.centroid = centroid;
  icosahedron.push(face = [temp[0], temp[1], centroid]);
  face.centroid = centroid;

  // Split the other face at middle point of edge.
  face = icosahedron[8];
  temp = face.slice();
  centroid = d3.geo.interpolate(face[1], face[2])(.5);
  face[1] = centroid;
  icosahedron.push(face = [temp[0], temp[1], centroid]);
  face.centroid = icosahedron[8].centroid;
  icosahedron[7].splice(2, 0, centroid);

And then build/modify your spanning tree accordingly...
(I am at an airport, soon flying, so no real possibility to give it a try)

- projection.clipAntimeridian(), getter, returns (clip == Antimeridian)
- projection.clipAntimeridian(true-ish), setter, returns projection, sets clip = Antimeridian
- projection.clipAntimeridian(false-ish), setter, returns projection, does not change clip strategy (???)

- projection.clipAngle(), getter, returns theta or null
- projection.clipAngle([theta]), setter, returns projection, sets clip = Circle with angle theta
- projection.clipAngle(false-ish), setter, returns projection, sets clip = Antimeridian (backward compatibility)

- projection.clipPolygon(), getter, returns polygon or null
- projection.clipPolygon([polygon]), setter, returns projection, sets clip = Polygon with that polygon
- projection.clipPolygon(false-ish), setter, returns projection, sets clip = NoClip (???)

Add a few basic tests including one for #54.
@Fil
Copy link
Member Author

Fil commented Aug 27, 2017

API updated, see the question marks in 6f09e3e if you have some patience for it

@mbostock
Copy link
Member

What if clipAntimeridian(false) behaved like clipPolygon(false), in other words adopting the clipNone behavior. Then only clipAngle(false) would be the weird one for backwards-compatibility, and in a future major release we could make clipAngle(false) also trigger clipNone instead of clipAntimeridian?

@Fil
Copy link
Member Author

Fil commented Aug 28, 2017 via email

@Fil
Copy link
Member Author

Fil commented Aug 28, 2017

Another (last?) API change for consistency: clipPolygon now reads and returns a GeoJSON object.

@Fil
Copy link
Member Author

Fil commented Aug 28, 2017

this branch https://github.com/Fil/d3-geo-projection/tree/clipPolygon%2Binterrupted makes sure that the clipPolygon() can work with all of the “interrupted” projections. If it didn't work it would have been a red flag.

For sure it is slower than clipAntimeridian (so maybe it should not be merged, or made into an option), and there are a few hiccups that will need to be investigated. Sometimes Antarctica "escapes" the clipping and bleeds all over the map. And bits of the graticule are discarded (apparently when they fall exactly on one of the clipPolygon's vertices).

@espinielli
Copy link

espinielli commented Sep 9, 2017

little attempt at (pseudo) Airocean (bl.ock), no land rotation yet...

preview

...but I get artefacts

  • sphere clipping (maybe)
    clip-sphere-artefact

  • I expected a straight side from face[14] to face[22]...
    bent-side

  • zig-zag in projection after split of face[14]:
    graticule-zigzag

Any hints/helps is welcome ;-)

@Fil
Copy link
Member Author

Fil commented Sep 9, 2017

http://blockbuilder.org/Fil/6d2fe81f148aefb04dd1c1792068ff93

  • Use latest d3-geo and d3-geo-projection (solves the clipping issue)
  • Use face.centroid as the center of the gnomonic, instead of the centroid of the half-face (solves the zig-zag)
  • The midpoint for 14/22 is not the geodesic midpoint, but a planar one (solves the straight side issue)
  • split face[19] in two halves (19 and 23) in order to connect face[22]

@espinielli
Copy link

you are an ace!

mbostock added a commit that referenced this pull request Sep 13, 2017
This allows arbitrary implementations of spherical clipping and pixel clipping
to be specified on a projection. We’re still not achieving the generality of a
composable projection pipeline, but at least we can plug in new implementations,
such as the forthcoming spherical polygon clipping (#108).
@mbostock mbostock mentioned this pull request Sep 13, 2017
5 tasks
@Fil
Copy link
Member Author

Fil commented Sep 19, 2017

This is now living in https://github.com/d3/d3-geo-polygon ; let's discuss it there, beginning with some examples!

@Fil Fil closed this Sep 19, 2017
Fil added a commit to d3/d3-geo-projection that referenced this pull request Oct 1, 2017
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 deleted the clip-polygon branch October 1, 2017 21:37
Fil added a commit to d3/d3-geo-projection that referenced this pull request Feb 20, 2018
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
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.

3 participants