mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.09k stars 2.21k forks source link

`touchmove` event should preserve Touch.identifier for multitouch. #6222

Open andest01 opened 6 years ago

andest01 commented 6 years ago

Mapbox's touch events are quite useful because have projection already computed as lngLats[]. Not only are they already projected, they're already offset to the container's bounds, so if you have a margin: 20vmin on your map so it suspends delicately like a picture in the Louvre, all of your lat lngs are already computed with the offset taken into account! Very nice work!

However, the current implementation of touchmove doesn't support multiple touches (fingers) continuously. This is because the lngLats does not preserve the original touch event's identifier property. What is so important about the identifier property? The identifier property allows the consumer of the event to create continuous multi-touch lines without jumps or breaks when a single finger leaves the canvas.

Steps to Trigger Behavior

  1. attach a touchmove event to your map.
  2. Change Google's sensors to Force Enabled to simulate touch events.
  3. Observe a touchmove event, specifically the lngLats array.

Expected Behavior

Each lngLat object preserves the touchmove identifier https://developer.mozilla.org/en-US/docs/Web/API/Touch/identifier

Actual Behavior

Each lngLat object is missing the touchmove identifier.

mapbox-gl-js version: 0.44.1 and prior

Imagine you placed four of your fingers on a touch surface, and you begin to wipe from left to right, as if you were clearing dew off your window. Four fingers create four distinct lines on your canvas. (Obviously, blood and treasure has been invested into turning our touch events into MultiPoint and MultiPolygon GeoJSON objects, but please ignore that technical detail). As your fingers track across the surface, you lift one finger off the canvas. The raised finger should no longer track, and the remaining fingers should draw as normal. While still tracking, you drop your raised finger onto the canvas AGAIN. The finger should draw naturally along with its siblings.

Here is the desired behavior: 2018-02-23_10-33-34

From my understanding, this is only made possible by Touch.identifier. https://developer.mozilla.org/en-US/docs/Web/API/Touch/identifier

Here is the current behavior, if we use as the identifier the index position of the array in lngLats. 2018-02-23_10-37-09

Yes, this bug is a bit complex, and I'll provide you with some code, but only if this bug is viewed to be possibly valid, rather than covered outright. Remember, we're talking about multiple fingers of contact, effectively drawing on a surface with multiple points coming in and out of the originalEvents's changedTouches property.

Let me know if you would like to investigate this further.

jfirebaugh commented 6 years ago

MapTouchEvent exposes the original TouchEvent on the originalEvent property, so I believe you can correlate (by array index) e.lngLats with e.originalEvent.touches in order to obtain the identifier. If I understand what you're requesting correctly, it's to assign e.lngLats[i].identifier = e.originalEvent.touches[i].identifier for each index of the array. However, it feels awkward to me to add an identifier property to LngLat object specifically for this one purpose. I think a better API (albeit one not compatible with the existing one) would be to replace the lngLats and points arrays of MapTouchEvent with a touches array whose elements have the properties of Touch plus lngLat and point.

andest01 commented 6 years ago

Thanks for your consideration! I think your proposed (non-compatible) solution would be ideal.

There's a deal breaker with the work around you described. I'm using changedTouches, which does not have the same items as touches, unfortunately. The MapBox e.lngLats is incompatible with e.originalEvent.changedTouches.

I am able to overcome the issue with originalEvents -- however, I DO lose the wonderful offset you folks have built into your event properties!

Here is a repro of that issue: The first part uses mouse events. Works perfectly, right out of the box! No fancy footwork needed! This is the ideal.

The second uses touch, and forces me to use e.originalEvent.changedTouch and then unproject them, but sadly now I also have to fold in a new offset object for the client rectangle's left and top. Doable, but no fun.

untitled 4 mov

Here's a segment of code, in case you're interested.

// Mouse events were handled already a couple lines above this comment. 
// Now handle touch events....
const { changedTouches } = e.originalEvent
  const points = []
  for (let i = 0; i < changedTouches.length; i++) {
    const changedTouch = changedTouches[i]
    // the all important `identifier` property
    const key = changedTouch.identifier
    const screenPoint = [
      changedTouch.clientX,
      changedTouch.clientY,
    ]

    // now we do (yet another) unproject, but the offset is wrong :(
    const lngLat = unproject(screenPoint)
    const item = {
      ...lngLat,
      id: key,
    }
    points.push(item)
  }

Here's what I propose I'll do my work-around where I fold in the offset to my React component. I'll make it work just fine on my end. Since I'm here, I'll check to see if changedTouches vs touches is really an important distinction. Maybe I'll luck out and there's no difference between the two. Either way, I'll make it work.

Meanwhile, Mapbox considers tightening up their touchmove events. Mark as suggestion and move on!

andest01 commented 6 years ago

Well I'll be.

Moving from changedTouchs to touches seems to work just fine, with a very negligible amount of bloat.

Now my code doesn't need unproject to be passed in as a functor, and my code is simpler now.

  const { touches } = e.originalEvent
  const points = []
  for (let i = 0; i < touches.length; i++) {
    const changedTouch = touches[i]
    const key = changedTouch.identifier
    const mapboxTouch = e.lngLats[i]
    const item = {
      ...mapboxTouch,
      id: key,
    }
    points.push(item)
  }

I wrote you a card.

2018-02-23_13-27-20