Reimplement the marker component

[oscarlorentzon]

The marker component should comply with the API of the tag component described in #113.

There are also bugs in the current implementation of the marker component. The mapillaryObjects configuration should be removed. The component should not fetch any objects on its own.

[oscarlorentzon]

Currently it is not possible to change the mapillaryObjects configuration value and get a change in component behavior.

[oscarlorentzon]

There are a number of ways to implement the marker component API. We have to create an efficient implementation that can handle millions of markers. We also want the API to be as simple as possible.

Properties of the API that I propose should hold irrespective of the option below that we choose:

• Adding a marker with existing id should just remove the reference to the old one, insert the new one and render the new one if it is in visible bbox. Corresponds to behavior of Mapbox GL Draw. Adding with an existing id is therefore interpreted as an update.

All options would have the following API:

Component.get(markerId: string)
Component.getAll()
Component.getMarkerIdAt(pixelPoint: number[])
Component.remove(markerIds: string[])
Component.removeAll()

Apart from that, the important part is how to handle updates to the marker positions
efficiently (either through changes from outside the librare or inside).

None of the suggestions have an update(marker: Marker) method in the API because add with existing id is interpreted as update.

The following are some of the ways to implement the API:

OPTION 1:

LatLon can be updated directly on the Marker.

Marker.setLatLon(latLon)
Marker.latLon get property
Marker.latLonChanged event
Component.addMarkers(markers)

Component needs to listen to event for all markers.
Need to add potentially millions of event listeners in Component.

OPTION 2:

LatLon is usually set only on one Marker at a time / you only work
with/edit one marker at a time.

Marker.latLon get property
Marker.setLatLon(latLon: ILatLon)
Component.addMarkers(markers: Marker[])
Component.setLatLon(markerId: string, latLon: ILatLon)
Component.changed event { marker: Marker } for all updates

With this option we can take decisions inside the component if
latLon is set through the Component.setLatLon method. We still
need a publicly exposed Marker.setLatLon which is problematic
because library users may use this method without understanding
it will not affect the latLon spatial index and therefor not be
reflected in the rendering.

OPTION 3:

Marker.latLon get property
Marker.addToComponent(Component)
Marker.setLatLon(latLon) -> Marker._notifyComponent()
Component.changed event { marker: Marker } for external and internal changes

MarkerGroup.addToComponent(Component)

Similar to the leaflet marker api.
Need grouping functionality to add sets of markers for perf reasons.

OPTION 4:

Marker.latLon get property
Component.addMarkers(markers: marker[])
Component.changed event { marker: Marker } for internal latLon changes (e.g. dragging)

This is the simplest API with least methods. It is efficient because no event listeners
to changes of markers are needed in the component. External updates are triggered by calling addMarkers. For external updates the changed event is not triggered.
This corresponds to the behavior in Mapbox GL Draw

Proposal

I propose to go with OPTION 4.

@gyllen Thoughts?

[gyllen]

@oscarlorentzon I also like OPTION 4 make a lot of sense to me. Just to clarify

1. When addMarkers is run this will not delete old markers right? It will just update the set (it will update the position of a marker if it has the same id as an already added one).
2. In the changed event only one listener is needed and you will get updates on every single marker. Lets say you add a 1000 markers then this method will be called a 1000 times. Is that correct? We could consider having a list of changed markers instead (not sure however if it makes sense).

[oscarlorentzon]

1. markerComponent.add(markers: Marker[]) will not clear any old markers if the id does not correspond. So you can add e.g. 1000 markers first and then 1000 other markers and the marker component set will contain 2000 markers. If a marker with the same id already exists the old marker will be replaced with the newly added one.

When doing the following

let marker1a: Marker = new Marker("id", { lat: 1, lon: 1 });
let marker1b: Marker = new Marker("id", { lat: 2, lon: 2 });

markerComponent.add([marker1a]); 
markerComponent.add([marker1b]); 

the reference to marker1a will be replaced by a reference to marker1b in the marker component. The marker component will not have any knowledge of marker1a after this.

This behavior is required to be able to update things like latLon and appearance of the Markers because it is not possible to updated these properties through direct calls to the Marker.

If the latLon has been changed like in the example it will update the position because of the fact that the Marker reference is replaced.

If we want the MarkerComponent to keep the same reference then we need to expose methods on the Marker class like the one in OPTION 2 Marker.setLatLon(latLon: ILatLon) and Marker.setColor(color: string) for use internally when updating the existing reference. If the API user would call those methods directly we would not know about that update.

What this leads to is that if a marker position is changed on the map, the library user needs to create a new marker updatedMarker = new Marker(...), call component.addMarkers([updateMarker]) and then ensure to replace any existing references outside of mjs from oldMarker with updatedMarker.

We need to make it clear that add means add or replace and not add or update in this case.

Any option we take will be a compromise between simplicity, performance and usability. We need to ensure that the docs are very clear on the behavior.

2. For OPTION 4: The changed event will only fire when the latLon of a marker has been changed inside the library (by a drag operation). It is not related to the list of markers. So it will not fire when a marker is added or removed. There will be no event for this because it is never done internally, only externally by the API user which do not need to get an event based on what was just done. If we at a later point add internal functionality for selection, addition, and deletion we will have to expose added, selectionchanged and removed events but because that functionality is not there right now those events are not needed.

Based on the changed event a marker can be updated on a map according to the new latLon that was set by the library internally. This internal operation will call a special method updatePosition(position: number[], latLon?: ILatLon) that includes a topocentric 3D position that can not be used externally and will not replace any object references. This method should be excluded from the documentation.

We will have the

Component.get(markerId: string)
Component.getAll()

methods for retrieving markers from the component so it is not dependent on having an event for changes to the marker list.

A note is that it is always possible to change the marker.latLon.lat and marker.latLon.lon properties because it is just a javascript object even though the marker.latLon property just has a getter. Doing this externally will cause faulty positions in the spatial index.

[oscarlorentzon]

API for the 2.5.0 release is OPTION 4