search

hotosm / imagery-coordination

Coordination of Imagery Requests for HOT Activations and other humanitarian mapping
8 stars 8 forks source link

Map position parameters for /imagery-search URL #104

Closed tombh closed 7 years ago

tombh commented 7 years ago

Note: the optimal event to listen for changes is moveend but the Mapbox Synch plugin seems to continuously trigger it. So if the code to update the URL is also bound to moveend then there is significant excess code execution -- browser slow down and URL constantly updates. Fortunately falling back to using a combination of dragend and zoomend seems sufficient.

Fixes #87

tombh commented 7 years ago

As @smit1678 pointed, zooming seems to have a bit of jitter/perf issues. Will see if what I can do...

tombh commented 7 years ago

So reverting to listen to moveend fixes the zoom problem. However, I could only test that by disabling map syncing -- so basically the 2 things don't play well together :/

There may be another event we can use. Eg; mouseup works well for dragging but not zooming. Or maybe there's some kind of other bad interaction going on when all maps are listening to dragend and zoomend. It which case a somewhat hacky solution would be to only set listeners once a touchstart or mousedown happens inside a specific map. But it just seems inelegant and feels like it will lead to unexpected behaviour and difficult to maintain code.

The best compromise appears at the moment to replace the map sync code with something that doesn't constantly sync the maps during manipulation, but rather syncs immediately after the manipulation has happened. Eg; you start dragging a little map and nothing happens, but as soon as you let go of the mouse, all the other maps instantly sync.

smit1678 commented 7 years ago

@tombh Thanks for the write up and testing ways to improve the performance. So it seems that we're in a spot where we may need to compromise on one of the following:

  1. Jittery zoom (through scroll and controls) but we keep the maps fully synced.
  2. More stable, performant zooming but maps only synced after panning or zooming is complete.

cc @danielfdsilva @nbumbarger for thoughts or insight on the compromise? or other ideas for work arounds?

danielfdsilva commented 7 years ago

@smit1678 @tombh You could try using the moveend method with some debounce or throttle. That would solve the excess updates, while keeping everything in sync.

tombh commented 7 years ago

Nice idea, so like updating the URL only once every 50ms of active movement for example?

tombh commented 7 years ago

I reverted back to using the superior moveend event and added a 500ms throttle to the URL updater. It seems to work better except for a tiny niggle that the cursor flashes between states when the throttle resets. Does it seem to effect the UX, or is it purely aesthetic?

smit1678 commented 7 years ago

👍 tested and works well. I don't see the cursor flash on Chrome or Firefox on OSX. I don't think it's a huge issue.