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

d3.geo.circle clipping is buggy #691

Closed
mbostock opened this issue Jun 29, 2012 · 10 comments
Closed

d3.geo.circle clipping is buggy #691

mbostock opened this issue Jun 29, 2012 · 10 comments
Labels
bug Something isn’t working
Milestone

Comments

@mbostock
Copy link
Member

Circle clipping seems to mostly work, but I think it's buggy.

For example, say you're viewing the default hemisphere centered around ⟨0,0⟩ and have a simple two-point LineString that starts at ⟨+60,0⟩ and ends at ⟨-60,0⟩. To render this correctly, you actually need to convert a LineString to a MultiLineString: you have one visible segment from ⟨+60,0⟩ to ⟨+90,0⟩ and another visible segment from ⟨-90,0⟩ to ⟨-60,0⟩. There's no way to represent the discontinuity here based on the current implementation; we'd need to change the clip(coordinates) function to return an array of coordinates to represent broken segments.

It's pretty easy to see this bug when you enable clipping with the rotating Azimuthal grid example.

Related: I also wonder if there is a more efficient way to enable clipping with animation. Currently, we resample after clipping. If we resampled before clipping, perhaps we wouldn't need to resample at each frame of the animation—we could just clip the coordinates directly. But maybe that's not possible.

/cc @jasondavies

@jasondavies
Copy link
Contributor

I started working on a fix for clipping a while ago, sorry it’s taken so long! I should have some time to look at this soon, as I need it to work for an upcoming project.

For clipping, I can see two possibilities:

  1. Essentially like we are doing now, project into Cartesian 3-space and clip based on distance from the clip circle origin. This can be optimised somewhat for an origin of [0, 0] as we can just check the z-coordinate (z=0 would be 90°). The advantage here is that these Cartesian coordinates can be cached and reused for the azimuthal projection itself, for different rotations.
  2. Alternatively, we can generate a convex polygon representing the clipping circle to a certain degree of precision in spherical coordinates. Then we could apply the clipping algorithm directly in spherical coordinates, before projecting each point. However, the time spent clipping now depends on the number of edges in the convex polygon, since I think it would take time O(mn) where m, n are the number of edges in the clipping and subject polygons.

In both cases I would use Weiler-Atherton as it handles holes correctly, and my preference is for no. 1 as I think it's faster and you can reuse the projected coordinates.

As for resampling, this was there mainly to handle the clipped edges, since most unclipped geo polygons are reasonably high precision already. At the moment, we just resample everything after clipping, regardless if it was a clipped or unclipped edge. So I think it would be reasonable to only resample clipped edges. If necessary, polygons can be resampled beforehand e.g. for the grid, but this wouldn't be necessary in most cases e.g. most country polygons.

Do you have any ideas about how reusing the Cartesian coordinates would look for the API? (It could also just be the sin/cos values if that's more usable). Perhaps the cached values could be piggy-backed onto the coordinates array, but that seems like a hack. Or we could expose projecting to Cartesian space so that it's explicitly cached, and this can be used for any animated projections. If a projection receives spherical coordinates, it can just convert on-the-fly instead.

Anyway, I guess fixing the clipping bug should be done first, then optimised afterwards. :)

@mbostock
Copy link
Member Author

mbostock commented Jul 2, 2012

Thanks for looking into this! I think the bug might be how the interpolation point is generated; I'm not sure (d0 - radians) / (d0 - d1) is the correct calculation.

Clipping to a circle sounds simpler than clipping to a convex polygon, and also more precise. I don't have any specific guidance on other improvements to make yet. Mainly I'd like to fix the clipping bug. :)

@mbostock
Copy link
Member Author

mbostock commented Jul 2, 2012

Here is a fixed version of the spinning globe that only clips points (no resampling): http://bl.ocks.org/3031319

It also uses d3.svg.line’s defined property to split clipped lines into multiple segments. Clever, eh? :)

@jasondavies
Copy link
Contributor

Ooh, very nice! See #695 for a fix, it turns out the last interpolation step was buggy.

@mbostock mbostock reopened this Jul 6, 2012
@mbostock
Copy link
Member Author

mbostock commented Jul 6, 2012

There’s still a bug. It's fairly easy to see if you reduce the geodesic subdivision to 1 and clip an icosahedron: http://bl.ocks.org/3061181. I suspect that the bug is still related to how we handle closed polygons. Would love if you could take another look, @jasondavies. :)

geodesic bug

Edit: Actually, you don't get this bug when using d3.geodesic.multilinestring (as in the grid example), because those aren't closed polygons. You only get it with a multipolygon or polygons.

@jasondavies
Copy link
Contributor

I have devised a solution for closed polygons (with holes). Now I just need to implement it. Watch this space. :)

@jasondavies
Copy link
Contributor

The icosahedron bug is actually due to a bug in the way d3.geodesic inserts the final point; it should be:

face.push(face[0])

Instead of:

face.push(face[2])

@jasondavies
Copy link
Contributor

The fixed version: http://bl.ocks.org/3072830

Although note that it's using my new clipping algorithm now, too. :)

@mbostock
Copy link
Member Author

mbostock commented Jul 9, 2012

Aw, man! Nice find on the bug, and sorry it was my fault. :) Let me know how the new clipping algorithm progresses.

@mbostock mbostock closed this as completed Jul 9, 2012
@boydgreenfield
Copy link

Hey guys -- I think I'm experiencing a related set of issues to the above with geo.circle.clip. But, I am unfortunately having a lot of trouble debugging it as it seems like an error in how the browser parses the svg paths.

Basically, I get an error Error: Problem parsing d="" when trying to rotate an orthographic azimuthal projection with a mouseover. For simple representations, it doesn't seem to cause any noticeable problems, but when I have a layer of points projected across a globe I then get random behavior like Argentina and Azerbaijan changing from the background country gray color I have specified to the red color of the points.

The error occurs for me on the latest versions of Chrome and Safari on OS X, though I can't seem to reproduce it in Firefox. It's also apparent in the above referenced Orthographic Clipping example and older examples ():

Notably, I can't get the console to squawk for this example in either Safari or Chrome:

  • http://bl.ocks.org/2974930 (I imagine this is because the shapes aren't actually being clipped, but I haven't looked at the code for it in detail)

I've also gone so far as to litter my own copy of the lastest master branch d3.js with console.log() statements and believe that the issue may only occur for Polygon and MultiPolygon GeoJSON shapes. That's mostly speculation however as I'm not sure how to catch and debug the actual SVG parsing error.

I hope the above details are helpful in tracking down some deeper bug, rather than simply dealing with some aberrant edge case.* Please let me know if there's anything I can do to help, though I'm basically brand new to the world of JS and know next-to-nothing about SVG as well (sorry!).

Thanks for all the work on this -- it's an awesome library.

Nick

  • I don't believe it's a data quality issue, as I've manually checked the coordinates for some of the bad Polygons in question and they close correctly, etc. It's also unlikely to be data-related given that the regular grid in the above examples also causes this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working
Development

No branches or pull requests

3 participants