visgl / deck.gl

WebGL2 powered visualization framework
https://deck.gl
MIT License
12.05k stars 2.08k forks source link

Mitering bug: PathLayer produces uneven strokes if the path has acute angles #337

Closed dcposch closed 7 years ago

dcposch commented 7 years ago

Deck.gl verson: 46add427dd7d2f132f5699e38edcc90802f3fc30 (dev branch)

image

Steps to reproduce

const layer = new PathLayer({ data: [{path}], opacity: 1, miterLimit: 1, strokeWidth: 6e3 })


* Zoom in on Ohio

### Expected result

Smooth path.

### Actual result

Uneven-width path. See screenshot above.
dcposch commented 7 years ago

@ibgreen @Pessimistress

Pessimistress commented 7 years ago

@dcposch You can fix this by increasing miterLimit. This value indicates the max size of a corner in ratio to the width. It's meant to limit super long corners created by sharp angles. In other words any corner with angle smaller than asin(1 / miterLimit) will get capped and have uneven appearance.

In the latest beta miterLimit is default to 4. I will update the document for better clarity.

Pessimistress commented 7 years ago

@dcposch @ibgreen A proper miterLimit should create a bevel when exceeded:

miter limit

The reason I have not implement it in the current PathLayer is because it will double the vertex count and make perf even worse (this layer is already the slowest in all 3 GeoJSON sub layers).

dcposch commented 7 years ago

@Pessimistress thx!

Increasing miterLimit causes other issues on my dataset:

image

Three thoughts

Pessimistress commented 7 years ago

@dcposch the PathLayer is already using pre-allocated typed arrays.

Unfortunately the current deck.gl layers are designed to be projection-agnostic, which means we do not know which side of the vertex is going to be an acute angle when we generate the attributes. I'm doing some experiments on this.

I can see your third suggestion having cool use cases, though utilizing depth test will disable opacity blending, thus breaking some other scenarios. Seems like this could be done easily by subclassing the layer and overriding the shader.

dcposch commented 7 years ago

@Pessimistress one way or another, we both need a solution to the mitering bug, right?

I think I'm using Deck.gl in partly the same way Uber uses it internally: to show vehicle routes. That means paths with U-turns, 270-degree highway ramps, etc. Right now PathLayer can't really handle those, as seen in the screenshots above.

Just curious, is anyone using Deck.gl with a projection different from the Mapbox default?

ibgreen commented 7 years ago

Just curious, is anyone using Deck.gl with a projection different from the Mapbox default?

@dcposch It's absolutely something we are interested in. We've gradually been adding support for using deck.gl with non-mercator viewports (i.e. non-geospatial use cases), and are currently reviewing if this is something we can get ready for the upcoming release.

Is that what you were thinking about, or were you e.g. considering using deck.gl with non-Mapbox maps?

ibgreen commented 7 years ago

using Deck.gl with a projection different from the Mapbox default?

As to the technical challenges, it involves: 1) generalizing the viewport system to allow for viewports created from arbitrary view and projection matrices rather than a list of mercator properties (lat,lng,zoom,pitch,bearing, ...). 2) Updating the GLSL shader project/project64 modules that depend on uniforms generated by the above viewport to handle all cases 3) separating event handling from the map (possibly providing one or more reference interaction models). Also we have to produce good documentation on all the above to make all this power easy for users and layer writers to understand. And make sure not to break existing code. So it is a non-trivial amount of work.

dcposch commented 7 years ago

Is that what you were thinking about, or were you e.g. considering using deck.gl with non-Mapbox maps?

The opposite: I'm guessing every current user of deck.gl is using it with Mapbox. If that's true, it may give you an opportunity to simplify your code, fix mitering, and reduce scope.

I don't know about using deck.gl for "non-geospatial use cases". There are already lots of general graphing libraries, including some that use WebGL. Assuming it's strictly for geospatial data visualization, you could still argue there are edge cases where you need special projections -- for example, making a map of Antarctica with the South Pole in the middle -- but those sound like they add a lot of code complexity for few or zero users.

I think if you clarified that Deck.gl is strictly for geospatial data visualization on top of Mapbox, Mercator only, it might make it easier to get to a place where it's really solid and stable.

yocontra commented 7 years ago

@dcposch Yeah, I'd agree with you on that. It would clean up a lot of complexity if deck.gl focused on geospatial displays and dropped support for everything else. Also curious if anyone is using this for non-geospatial use cases - if so I'd imagine they would use something different.

ibgreen commented 7 years ago

I think if you clarified that Deck.gl is strictly for geospatial data visualization on top of Mapbox, Mercator only, it might make it easier to get to a place where it's really solid and stable.

@dcposch That's good feedback and I understand where you are coming from. At this point let me confirm that geospatial is absolutely the primary use case for deck.gl and any support for other use cases should not compromise this. That said, I'd like to see more concrete examples of things that will be technically significantly easier if we did this.

Yeah, I'd agree with you on that. It would clean up a lot of complexity if deck.gl focused on geospatial displays and dropped support for everything else.

@contra Really value your input especially since you've been an active user since the start. Apart from my comment in this ticket, do you find the messaging around deck.gl unclear? Is there anything you feel should be dropped at this point from the current releases? Or is this mainly forward looking comment? I would hate to muddy the waters.

Also curious if anyone is using this for non-geospatial use cases - if so I'd imagine they would use something different.

All I can say is that we are building some pretty amazing "hybrid" apps internally on top of deck.gl (unfortunately not something I can share more about at this point) and we are also finding that deck.gl's "reactive" programming model, and the ease of packaging WebGL visualizations as reusable layers extends well to other use cases. There is of course a debate about these topics here internally as well, so your input is timely!

yocontra commented 7 years ago

@ibgreen If you've got a non-map use case though that's enough justification to keep those knobs exposed and dedicate space/time to support them. Mostly approaching it from an API complexity/code complexity perspective - as an end user it doesn't matter much if it supports additional use cases, I don't think the messaging is muddy about the priorities.

ibgreen commented 7 years ago

Mostly approaching it from an API complexity/code complexity perspective - as an end user it doesn't matter much if it supports additional use cases, I don't think the messaging is muddy about the priorities.

@contra - Thanks. I agree that considering the impact on API and code complexity is a good compass, and keeping the API and source code of deck.gl and the layers easy to understand is a key concern for us. We'll tread carefully if and as we grow the scope. We also aspire to make the deck.gl roadmap more public so that we can have these discussions up front rather than post-facto. Stay tuned.

howtimeflies0 commented 7 years ago

@dcposch @contra Thanks a lot for your feedbacks. There are a lot of discussions around the same topic within the deck.gl team as well. We can assure you that opinions from users like you guys will get heard by the whole team and carefully considered as we move forward. Rest assure, geo-visualization will always be the most important goal for deck.gl.

As @ibgreen just mentioned, we have some internal use cases that led us to explore the non-map use cases. It's still in very early stage.

ibgreen commented 7 years ago

I think this issue is closed based on fixes from @Pessimistress

dcposch commented 7 years ago

thanks!