Leaflet / Leaflet.VectorGrid

Display gridded vector data (sliced GeoJSON or protobuf vector tiles) in Leaflet 1.0.0
606 stars 194 forks source link

Refactor Renderers to be more universally useful. #79

Open jkuebart opened 7 years ago

jkuebart commented 7 years ago

The new concept of a RenderService is introduced. There are two implementations, one for SVG/VML and one for Canvas. A RenderService provides methods for render-method agnostic initialisation and sizing of a target DOM element, drawing and styling of points, paths and polygons and mouse event handling for the generated features. It doesn't extend any class, but can be associated to a Map instance for the purpose of firing events.

The concept of Renderer remains unchanged: it extends Layer and reacts to changes in the map by updating its viewport. There are two implementations of Renderer, one utilising a RenderService for SVG and one for Canvas. A Renderer can be added and removed from a Map or serve as an implicit layer for vector layers added to the Map. It draws vector layers by forwarding their requests to its RenderService. The RenderService fires mouse events at the Layer subclasses as before.

The vector layers such as Polyline and siblings remain unchanged.

For VectorGrid, a Tile is an object that provides a DOM element to act as a map tile. Tile does not extend Layer or any class at all. There are two implementations, one utilising a RenderService for SVG and one for Canvas. A Tile receives its DOM element from its RenderService. The advantage over the current design of using Renderer subclasses for this purpose is that there is no need to work around undesirable actions of Renderer like overriding onAdd() to prevent inserting itself into a map pane, preventing it from reacting to the map's update event by adjusting its viewport or overriding Layer's addTo() method to prevent unnecessary actions like adding attributions or firing events.

For VectorGrid, a TileFeature is an object extending Evented that represents a single feature in a vector-tile. The advantage over the current design of subclassing Polyline and siblings is that there is no need to override code that projects latitude/longitude to screen coordinates, or using private methods of Renderer to do the actual drawing. TileFeatures use Symbolizers for their visual representations, which are rendered by the RenderService associated to their containing Tile. The RenderService fires mouse events at the TileFeature instances, providing access to the vector-tile features from the event handlers.

TileFeatures are added to a Map using the new Map.addInteractiveTarget(Evented, domElement) method.

jkuebart commented 7 years ago

Happy to hear your thoughts @perliedman @IvanSanchez ;)

IvanSanchez commented 7 years ago

So if I've understood this correctly, you want to switch from this...

d21f5b9d

Into something like... this?

9118ebb4

I don't quite grasp how this would look in the end.


[...] TileFeature the current concept of FeatureLayer [...]

Actually, a FeatureLayer is quite close to what I mean by a Symbolizer, because one tile/geojson feature, i.e.

{
  type: "Feature",
  geometry: { type: "point", coordinates: [0, 0] },
  properties: { name: "Null Island" }
}

... can have more than one symbolizer (e.g. a road has background, casing, and name).

I'm not fully understanding how the architecture should look like, I'm not sure this can be done without impacting core Leaflet, and I think this might conflict with https://github.com/Leaflet/Leaflet.VectorGrid/issues/76 - so, for now, I'm reluctant to this.

jkuebart commented 7 years ago

I think your diagrams are correct. As can be seen, nothing really changes in the structure on the left (L.Renderer, L.Path, L.SVG, L.Canvas) while the L.VectorTile subclasses lose their relationship with Renderer.

The idea here is to factor out the render-method agnostic vector drawing currently provided by the Layer subclass Renderer into RenderService so it can also be used for vector tiles that don't really work like Layer. Renderer adds functionality related to updating the viewport in response to map panning and zooming, which is undesirable and needs to be deactivated for tile renderers.

Of course it will have an impact on Leaflet core in so far as there will be the new RenderService classes. However, the API of existing Leaflet classes like Renderer and L.Path will remain unchanged.

IvanSanchez commented 7 years ago

Renderer adds functionality related to updating the viewport in response to map panning and zooming, which is undesirable and needs to be deactivated for tile renderers.

So if this is the problem that the refactor would be solving, I'd then go for just adding a

getEvents: function() { return {}; },

method to both L.SVG.tile and L.Canvas.Tile

jkuebart commented 7 years ago

Not exactly, the refactor is supposed to solve the problems that there is no need to work around undesirable actions of Renderer like

For FeatureLayers (or TileFeatures) it would solve problems like

I realise it is quite a big reshuffle, but I think it would be worth it in gained clarity.

jkuebart commented 7 years ago

I have updated my comment to the new Symbolizer nomenclature because I think it still addresses valid concerns:

  1. There is no »is a« relationship between Tiles and Renderers in that Tiles are not supposed to react to update events from the Map by adjusting their viewport, but are in contrast moved around along with the VectorGrid layer. There is also no »is a« relationship between Tiles and Layers in that
    1. Tiles mustn't add themselves to panes on the map as done by Renderer.onAdd,
    2. Tiles don't represent entities that can be individually added or removed from the map like a Layer and
    3. Tiles are not visible to clients and therefore don't need to be able to emit events.

This implies that SVG.Tile and Canvas.Tile extend L.SVG and L.Canvas primarily for code reuse. What is more, since they don't require much of the functionality provided by their superclasses Renderer and Layer, they go to great lengths to override some of that functionality and avoid using the rest.

Similar reasoning holds for Symbolizers. There is no »is a« relationship between a Symbolizer and a Polyline since the latter provides functionality such as latitude/longitude projection which is meaningless in the context of Symbolizers. There is also no »is a« relationship between a Symbolizer and a Layer because Symbolizers don't represent entities that can be individually added or removed from a map, even less so than Tiles.

This implies that Symbolizers inherit from Polyline merely for code reuse. This is made more problematic by the fact that Polyline and siblings use private Renderer API for drawing, which makes it necessary for Symbolizer to heavily rely on that API and couples it tightly to Renderer.

By exposing the render-target agnostic API as a RenderService, this proposal simplifies the implementation of vector drawing components other than those provided by core Leaflet. It also greatly reduces the interdependence between components by favouring composition over inheritance.

This change would greatly simplify the implementation of new functionality such as that suggested in #74.