Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug causing queryRenderedFeatures to crash on polygon features that have an id field. #4494

Closed
gmaclennan opened this issue Mar 24, 2017 · 9 comments

Comments

@gmaclennan
Copy link
Contributor

gmaclennan commented Mar 24, 2017

mapbox-gl-js version: v0.34.0

Steps to Trigger Behavior

  1. Add a GeoJSON polygon source with non-integer (string) ids
  2. Call map.queryRenderedFeatures() on mousemove

Expected Behavior

map.queryRenderedFeatures() should return an array of features under the mouse pointer

Actual Behavior

Uncaught Error: Unimplemented type: 7

I have created a JSFiddle of the failing code: https://jsfiddle.net/p78o8exf/6/

This code works fine with v0.32.1 but since v0.33 it is broken. This is valid GeoJSON (checked with @mapbox/geojsonhint).

@jfirebaugh
Copy link
Contributor

@gmaclennan I'm seeing "Uncaught ReferenceError: dataIndex is not defined" when loading that JSFiddle, which seems to be preventing it from getting to the subsequent error. Can you take a look?

@gmaclennan
Copy link
Contributor Author

@jfirebaugh thanks for following up, my apologies, I forgot to save changes as I was removing unrelated code. try here: https://jsfiddle.net/p78o8exf/3/

@gmaclennan
Copy link
Contributor Author

@gmaclennan
Copy link
Contributor Author

One more piece of information: I tried filtering the geojson to only show features with geometry.type === 'Polygon' or MultiPolygon and I get the same error with each, well, I get a different unimplemented type: 4

@indus
Copy link
Contributor

indus commented Mar 27, 2017

@jfirebaugh
Copy link
Contributor

Can you reduce the test case further, e.g. remove everything that's not essential to triggering the bug from the GeoJSON?

@gmaclennan
Copy link
Contributor Author

gmaclennan commented Mar 27, 2017

Ok, more pairing down done now, and I found the cause of the bug:
https://jsfiddle.net/p78o8exf/6/
It is triggered by leaving the id on the feature. Removing that fixes it.

@anandthakker
Copy link
Contributor

@gmaclennan thanks for isolating it!

Looks like the problem is that the feature.ids are strings here, which we currently don't handle correctly (it's a vestige of the fact that the vector tile spec only allows integer IDs, and GeoJSON sources share some implementation details with vector tile sources). Changing id to, e.g., Math.floor(Math.random() * 1000) prevents the crash.

This is being tracked at #2716, although the test case here demonstrates that (polygon) layers manifest the bug more severely -- crashing rather than simply stripping out the id.

Closing in favor of 2716

@anandthakker anandthakker changed the title Unimplemented type error with GeoJSON source from >v0.32.1 Fix bug causing queryRenderedFeatures to crash on polygon features that have an id field. Mar 29, 2017
@anandthakker
Copy link
Contributor

Reopening here to track the crashing bug, which @gmaclennan rightly pointed out is distinct from #2716, which is an enhancement.

@anandthakker anandthakker reopened this Mar 31, 2017
@anandthakker anandthakker self-assigned this Apr 11, 2017
anandthakker pushed a commit that referenced this issue Apr 12, 2017
anandthakker pushed a commit that referenced this issue Apr 12, 2017
anandthakker added a commit that referenced this issue Apr 12, 2017
* Add failing regression test for #4494

* Guard against non-numeric id in GeoJSON features

Closes #4494

* Add explanatory comment

* Update tests to conform to numeric-only feature.id

* Add explanatory comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants