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

[regression] 0.44.0 map.queryRenderedFeatures() completely ignores symbols #6074

Closed
mike-marcacci opened this issue Jan 28, 2018 · 12 comments
Closed

Comments

@mike-marcacci
Copy link
Contributor

mike-marcacci commented Jan 28, 2018

Upon upgrading to 0.44.0, map.queryRenderedFeatures() fails to return any symbol layers that use GeoJSON sources specified in the second argument. I've gone through my layers' layout properties one-by-one to see if there is one particular trigger, and there doesn't seem to be.

Do note, though, that rarely map.queryRenderedFeatures will return a symbol layer, but after just another second of map interaction, stops again. Downgrading to 0.43.0 fixes this.

VIEW SCREENCAST

Check out #6075 for a screencast of "expected" behavior.

@pathmapper
Copy link
Contributor

pathmapper commented Jan 31, 2018

Running into the same issue with overzoomed vector tile sources hosted by Mapbox.

Steps to reproduce using this jsfiddle:

  1. Hover over the green circle -> Properties are shown on the right side of the map (using the queryRenderedFeatures API)

  2. Zoom further in

  3. Hover again over the green circle -> Properties are NOT shown on the right side of the map

Please note:
Sometimes it is necessary to repeat steps 2 and 3 to reproduce.
Zoom extend of the used vector tileset for the point: z11-z12

query_symbol

Edit:
Is a z12 tile at zoom 12.1 overzoomed or not?

@anandthakker
Copy link
Contributor

@mike-marcacci Thanks for reporting and for the screencast. Is there any chance you'd be able to share a live page that reproduces the issue? So far, I haven't managed to reproduce it locally.
(@pathmapper's jsfiddle above does reliably show an issue [thanks!], but it's not clear that it's the same as the one you're seeing)

@mike-marcacci
Copy link
Contributor Author

Hi @anandthakker, thanks for the reply. I don't have anything live running this version (I'm just running the previous one instead), but I'll try to get a simplified working demo live today.

@anandthakker
Copy link
Contributor

I believe the overzooming problem that @pathmapper described above happens like so:

  • [render frame 1] Map starts at z12, so initial symbol placement occurs using tile 12/1283/1587.

  • Zoom to z13; this means that we have a new, overzoomed version of the above tile (13/1283/1587 in the debug view)

  • [render frame 2] We start a new placement, but it gets paused. Thus this code updating map.style.collisionIndex doesn't run:

    if (this.pauseablePlacement.isDone()) {
    const placement = this.pauseablePlacement.placement;
    placementChanged = placement.commit(this.placement, browser.now());
    if (!this.placement || placementChanged || symbolBucketsChanged) {
    this.placement = placement;
    this.collisionIndex = this.placement.collisionIndex;
    }
    this.placement.setRecent(browser.now(), placement.stale);
    }

  • (btw, there might be some other issue at play here: it's not clear why we'd ever have to pause placement when we're only placing a single symbol, as in this example)

  • [render frame 3] We continue the paused placement, and complete it. However, since the crossTileSymbolIndex got updated in the previous frame, symbolBucketsChanged is false this time. And (I think?) because we're placing the same symbol again this time, placementChanged also comes back false. As a result, we still don't update map.style.collisionIndex.

The consequence: even though map.style.sourceCache is now operating off of the overzoomed (13/1283/1587) tile, the feature entry in map.style.collisionIndex refers to the original (12/1283/1587) tile, and thus gets filtered out of the query results.

cc @ansis @ChrisLoer

@anandthakker
Copy link
Contributor

@mike-marcacci I'd be curios to know if this branch still exhibits the problem for you: #6104

@mike-marcacci
Copy link
Contributor Author

mike-marcacci commented Feb 8, 2018 via email

anandthakker pushed a commit that referenced this issue Feb 8, 2018
anandthakker pushed a commit that referenced this issue Feb 8, 2018
anandthakker pushed a commit that referenced this issue Feb 9, 2018
anandthakker pushed a commit that referenced this issue Feb 9, 2018
anandthakker pushed a commit that referenced this issue Feb 9, 2018
@mike-marcacci
Copy link
Contributor Author

Hey team, sorry again for the delay – finally back on the grid. I can confirm that the branch 6074 (from PR #6104) restores the behavior from 0.43.0! Thanks for the quick fix!

@anandthakker
Copy link
Contributor

Thanks for confirming, @mike-marcacci

anandthakker pushed a commit that referenced this issue Feb 12, 2018
anandthakker pushed a commit that referenced this issue Feb 12, 2018
anandthakker pushed a commit that referenced this issue Feb 12, 2018
@anandthakker
Copy link
Contributor

Fixed in #6104

pathmapper pushed a commit to pathmapper/mapbox-gl-js that referenced this issue Feb 13, 2018
pathmapper pushed a commit to pathmapper/mapbox-gl-js that referenced this issue Feb 13, 2018
@gmaclennan
Copy link
Contributor

gmaclennan commented Mar 19, 2018

@anandthakker I am still seeing this bug in 0.44.1 - just realized that the map we built for our partners is not working just before they have a major launch this week! Returning to 0.43.0 works. Live example here: https://wherewework.amazonfrontlines.org/ source code: https://github.com/digidem/alianza-ceibo-maps/

@anandthakker
Copy link
Contributor

Thanks @gmaclennan. I suspect this might be a different underlying issue, but it's hard to be sure without a minimal reproduction -- could you either describe the details of the querying that's happening in your example or reduce it to a smaller example free of extraneous application code?

@gmaclennan
Copy link
Contributor

Hey @anandthakker thanks for the quick response. This looks like it was a weird caching issue both on the published version and on my local dev machine, which means that the update to 0.44.1 was not coming through correctly. Anyhow, it seems to have started working again now.

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

5 participants