Vectors 2.0 #2290
Vectors 2.0 #2290
Conversation
|
Polyline works (with SVG renderer) now! The work will go pretty smooth from now on I think. |
|
@danzel @tmcw @jfirebaugh This is pretty much finished except some minor stuff — all tests pass and previous vector examples work, so I would really appraciate some feedback — feel free to review. |
|
OK, this epic refactoring is done. Everything seems to work. Now waiting for feedback. |
|
@jacobtoye Jacob, would love to have you review this epic pull! It may affect Leaflet.draw a lot. |
|
An expected bug: |
|
It is very cool being able to have multiple canvas/svg renderer layers! |
| // click on polygon border | ||
| if (L.Polyline.prototype._containsPoint.call(this, p, true)) { return true; } | ||
|
|
||
| // ray casting algorithm for detecting if point is in polygon |
There was a problem hiding this comment.
It is more likely the mouse will be inside the polygon than on the line, could move this before the polyline check.
|
Have played with this and had a rough read over, haven't noticed anything obviously wrong and it seems to work :) |
|
Actually, found something that doesn't work: If you remove and re-add canvas or svg it doesn't reappear (This might not be supported, just thought I'd try) Removing path makes a crash |
|
Sweet refactor @mourner. It breaks Leaflet,draw slightly but shouldn't be too hard to fix. I'll create a new branch to fix. When do you think you'll merge to Leaflet master? (roughly) |
|
@jacobtoye awesome! I'll merge in a few days maybe, after figuring out the issues Dave pointed out. |
|
|
||
| if (this._latlngs.length) { | ||
| this._pxBounds = new L.Bounds( | ||
| this._map.latLngToLayerPoint(this._bounds.getNorthWest()).subtract(p), |
There was a problem hiding this comment.
If you create an empty L.Polyline without adding to the map, add latlngs then then try to add to the map this._bounds is an empty L.LatLngBounds object. Therefore this._bounds.getNorthWest() will error.
This is because L.Path.redraw doesn't call L.Renderer._update since this._map doesn't exist (as it hasn't been added to the map yet).
E.g.
var a = L.polyline([]);
a.addLatLng([0,0]);a.addLatLng([1,1]);a.addLatLng([1,2]);
map.addLayer(a);There was a problem hiding this comment.
No, it's not because of redraw, it's because this._bounds is extended when doing _convertLatLngs, and addLatLng doesn't use this, so we need to extend there too.
There was a problem hiding this comment.
Should be fixed now, please check it out.
There was a problem hiding this comment.
I am wondering if this is somehow related to an issue(this._map not existing) that I am having in Draw + 0.8-dev where moving the mouse after "draw-created" or "draw-stop" throws Uncaught TypeError: Cannot call method '_fireMouseEvent' of null from https://github.com/Leaflet/Leaflet/blob/master/src/layer/marker/Marker.js#L231 .
I have a branch here to illustrate this with, just check out the basic demo.
|
@danzel OK fixed everything that you've mentioned (unclickable SVG, adding/removing, etc.) — we're good! |
|
@jacobtoye also pay attention to 5af4f0e, you'll have to fix this across Leaflet.draw code. |
|
Looks good from the diffs |
|
OK Leaflet.draw should be fine with this branch. Only issues were the one I mentioned in the commit comment, one around dash arrays and the |
|
I'll check Leaflet.label too. I have a feeling that it will also be broken ;) |
|
I think the branch is ready to merge :) Excited! |
|
Sweet Leaflet.label works :) Really like this refactor. Hopefully there is not too many complaints about moving support for multipolys from |
|
Ha, not yet ready! Wait till I commit an even more awesome improvement — partial redraw of canvas vectors that makes things like interactive choropleth maps with shape hover many times faster! |
|
@danzel @jacobtoye now check the last commit — it should make a massive performance boost when using lots of canvas layers that update on hover (tested on choropleth of ~1800 zip code shapes, looking smooth). |
|
@jacobtoye @danzel btw, what do you think about getting rid of |
|
I like having If the classes were merged would this mean that and would achieve the same thing? I can see how users might find that confusing. |
|
@patrickarlt we could specifically include a note on Multi-features in documentation so that people could immediately find it in the reference. Yeah, visually those two lines would lead to the same, but I don't see that as a problem for users. My main point for ditching user-facing Multi-classes is that API should reflect implementation, we don't want to have bogus classes just for the sake of "Multi" in the name — that would be confusing for sure. |
|
@mourner I agree with removing the Multi* classes. However it will obviously effect people who have updated to the newest Leaflet without being aware of the changes. Perhaps you could deprecate the classes for a version |
|
@jacobtoye |
|
TOTAL APOCALYPSE NOW |
|
Some people just want to watch the world burn... ;) Great work @mourner |
Major rewrite of Leaflet vector layers, #155. The goal is to refactor vector layers code in Leaflet to make it possible to switch between rendering backends (Canvas, SVG, etc.) dynamically, have more than one physical pane for vector layers for advanced stuff (layering etc.), get rid of ugly hacks, and make the code much cleaner and more transparent.
Another nice additions include massive performance boost for Canvas layers (in cases like updating shape style on hover), retina support for Canvas, proper MultiPoly implementations (one path with several rings instead of
FeatureGroup-based), polygetCenterthat returns proper centroids and is used bypath.openPopupand much faster (cached) polygetBounds.This also potentially opens it up for interesting future extensions, like custom shapes, indexing layers with RBush for fast interaction features, etc.
Renderer— extendsLayer, handles positioning, bounds and zoom animation for an SVG/VML/Canvas rootSVG— extendsRenderer, adds SVG root and handles generic SVG stuff (elements, styles, events, etc.)SVGmultidimentional points to SVG paths for polygons/polylines/multipolySVGpoint/radius into SVG circle pathSVGmouse eventsSVGbrintToBack/bringToFrontSVG.VML— patches SVG with VML-specific code (since other code is pretty similar)Canvas— extends Renderer, adds Canvas root and handles generic Canvas stuffCanvasdrawing polylines/polygons/multipoly from multidimensional points on canvasCanvasdrawing circles from point/radius on canvasCanvasmouse eventsPath— extendsLayer, base class for all vector layers (without renderer-specific code)Pathpopup methodsPolyline— extendsPath, projects/clips/simplifies latlngs, delegates drawing to rendererPolylineclosestLayerPointPolygon— extendsPolyline, clips polygon latlngs, delegates drawing to rendererMultiPolyline— turned out to be unnecessaryMultiPolygon— turned out to be unnecessarytoGeoJSONfor polylines/polygons/multipolysRectangle— extendsPolygonCircleMarker— extendsPath, delegates drawing to rendererCircle— extendsPath, scales radius, delegates drawing to rendererCanvasbrintToBack/bringToFrontopenPopupfor polylines/polygons/multipolys?getBoundsthat takes rings into account