search

perliedman / leaflet-routing-machine

Control for routing in Leaflet
https://www.liedman.net/leaflet-routing-machine/
Other
1.09k stars 352 forks source link

Auto re-route not firing routesfound/routingerrors #525

Open arminus opened 5 years ago

arminus commented 5 years ago

I currently observe a strange behavior: Sometimes, i.e. not sure under which conditions exactly, a map zoom/pan event triggers a re-route. There is no clear repro, most of the times it doesn't trigger the re-route some times it does. I think that's the first issue - why does it trigger apparently arbitrarily?

Second and more annoying issue is that if it triggers the re-route, it only appears to fire the routingstart event but not routesfound/routingerrors. Since I'm setting a progress indicator in my routingstart handler, I can't reliably clear it in this situation.

Routing engine for that case is OSRM.

perliedman commented 5 years ago

The re-route is intentional: the OSRM router first fetches a low resolution version of the route. If the map is later panned or zoomed so that a higher resolution is required, the same route will be fetched again, but this time with the high resolution parameter set. This is intended to save bandwidth. The logic for this is here: https://github.com/perliedman/leaflet-routing-machine/blob/master/src/control.js#L51

For the re-fetch, which is not a normal routing, I think neither routingstart or routesfound should be fired, so I think you have stumbled upon a bug!

perliedman commented 5 years ago

It could possibly be as easy to fix as adding a conditional to check if options.callback is set around this line: https://github.com/perliedman/leaflet-routing-machine/blob/master/src/control.js#L303

arminus commented 5 years ago

Empirically, it seems that routingstart should only be fired if options.callback is not set - but I'm really just debugging at that point and don't have the big picture...

i.e. the fix would be:

            if (!options.callback)
                this.fire('routingstart', {waypoints: wps});
perliedman commented 5 years ago

Yeah, exactly, sorry for expressing that in a confusing way.

arminus commented 5 years ago

Right. So far, that works for me without any sideeffects.

jcsimonin commented 4 years ago

Hi, I had the same issue and fixed it (so far) in _onZoomEnd function with this._updateLineCallback(err, this._routes); instead of this._updateLineCallback(err, routes);

Not sure of the potential side effects it might cause but i'd be glad if someone could review this.