mapbox / mapbox/mapbox-gl-draw

draw_line_string from: is not usable due to float precision

Open
#788 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

api bug
Dominant language
JavaScript
Stars
1.1k
Forks
612
Avg merge
8d 9h
Merged PRs (30d)
5

Description

**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
```

2. 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](https://gis.stackexchange.com/questions/8650/measuring-accuracy-of-latitude-and-longitude/8674#8674), but still, inside memory these coordinates should match I believe.

3. 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/modes/draw_line_string.js at the linked line and trace how the draw_line_string from option compares vertices. Reproduce continuation from a customised direct_select mode, then determine whether the existing from behavior or a separate fromIndex API should be retained. Done means continuing a line works without the reported float-precision failure and the chosen API behavior is documented by the implementation.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.