mapbox / mapbox-gl-draw

Draw tools for mapbox-gl-js
https://www.mapbox.com/mapbox-gl-js/example/mapbox-gl-draw/
ISC License
953 stars 593 forks source link

draw_line_string from: is not usable due to float precision #788

Open hyperknot opened 6 years ago

hyperknot commented 6 years ago

mapbox-gl-js version: 0.45.0 mapbox-gl-draw version: master

Steps to Trigger Behavior

  1. Click on a vertex in a customised direct_select mode
  2. Call changeMode to continue the line, using draw_line_string's from option
  3. It won't work, due to float precision.

Expected Behavior

Continue line should work.

Actual Behavior

Continue line should not work.

OK, let me explain this in a bit. I'd like to continue a line using metaClick on a vertex at one of the endpoints endpoint in a customised direct_select.

onVertex = function(state, e) {
  const about = e.featureTarget.properties

  if (e.originalEvent.metaKey && state.feature.type === 'LineString') {
    const clickedIndex = Number(about.coord_path)
    if ([0, state.feature.coordinates.length - 1].includes(clickedIndex)) {
      return this.changeMode('draw_line_string', { featureId: state.feature.id, from: e.featureTarget })
    }
  }

There are 2 issues with this.

  1. The code is actually wrong, this line https://github.com/mapbox/mapbox-gl-draw/blob/bf3dac14a7439f9c99e179c37f1f59a218b3a319/src/modes/draw_line_string.js#L22 should be
from = from.geometry.coordinates
  1. The vertices are compared by geometric location, not by index. Comparing by geometric location doesn't work for some reason, I don't know if this is a bug in mapbox-gl core or in mapbox-gl-draw, but coordinates only match up till 6 digits.

    Anything over 6 digits shouldn't be used in GeoJSON (as per recommendation of the specs), and the 7th digit is for 11 mm, but still, inside memory these coordinates should match I believe.

  2. I actually liked the proposed API much better, where you submitted an index to from:, or at least that's what I thought was proposed here: https://github.com/mapbox/mapbox-gl-draw/issues/605#issuecomment-284491132

Why was that nice, logical proposal of using index changed into this passing of a full geometric feature and then comparing using location?

BTW, based on 1. and 2. I believe probably no one is using this feature and can be changed to use index without effecting users. Alternatively "fromIndex" could be introduced and "from" can be deprecated I guess.

JoshuaPerk commented 6 years ago

While I can't speak to @hyperknot's issue since I'm not using a custom mode, I wanted to record a quick solution that helped me continue drawing at the end of a completed line. In my case, I'm just grabbing the first feature, but in others applications, you'll want to validate it exists, is the right type, and is the feature you want to edit.

The part that's important is grabbing the features coords, skipping to the last pair set, and passing that in as the from.

draw.changeMode('draw_line_string', {featureId: draw.getAll().features[0].id, from: draw.getAll().features[0].geometry.coordinates[draw.getAll().features[0].geometry.coordinates.length-1]});