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

queryRenderedSymbols does not sort features #6184

Closed
mollymerp opened this issue Feb 16, 2018 · 4 comments
Closed

queryRenderedSymbols does not sort features #6184

mollymerp opened this issue Feb 16, 2018 · 4 comments
Assignees
Labels

Comments

@mollymerp
Copy link
Contributor

mapbox-gl-js version:

Steps to Trigger Behavior

  1. load a symbol layer with collision detection disabled + overlapping symbols
  2. use on('click', layer, ... to invoke queryRenderedFeatures on the symbol layer

Expected Behavior

e.features[0] is the topmost rendered featured (and subsequent features are sorted in z order) per

mapbox-gl-js/src/ui/map.js

Lines 747 to 748 in 38c7250

* The topmost rendered feature appears first in the returned array, and subsequent features are sorted by
* descending z-order. Features that are rendered multiple times (due to wrapping across the antimeridian at low

Actual Behavior

e.features[0] is not always the topmost feature

motivate

looks like we sort non-symbol features, but don't sort symbol features:

const matching = this.grid.query(minX - additionalRadius, minY - additionalRadius, maxX + additionalRadius, maxY + additionalRadius);
matching.sort(topDownFeatureComparator);
this.filterMatching(result, matching, this.featureIndexArray, queryGeometry, filter, params.layers, styleLayers, args.bearing, pixelsToTileUnits);
const matchingSymbols = args.collisionIndex ?
args.collisionIndex.queryRenderedSymbols(queryGeometry, this.tileID, args.tileSize / EXTENT, args.collisionBoxArray, args.sourceID, args.bucketInstanceIds) :
[];
matchingSymbols.sort();
this.filterMatching(result, matchingSymbols, args.collisionBoxArray, queryGeometry, filter, params.layers, styleLayers, args.bearing, pixelsToTileUnits);

@ChrisLoer
Copy link
Contributor

@ansis Do you know the origins of that documentation? I'm not sure what it means by "topmost rendered feature" -- it looks like the sorting is based on the order in which items are inserted into the FeatureIndex by *Bucket.populate... which is closely related to the draw/z-order but not strictly the same especially across tiles?

But the recent divergence of symbol behavior from non-symbol behavior is because of the sorting here:

sortFeatures(angle: number) {

I think we'll need to do something like store the sort indices generated there and use them for sorting the matchingSymbols results in queryRenderedFeatures

@ansis
Copy link
Contributor

ansis commented Feb 26, 2018

@ChrisLoer yep, you're right about it being similar but not always right. The tile boundary bug might have existed when it was added, or we might have changed how querying works at tile boundaries (do we index points outside of tile edges or do we do padded queries across tile boundaries).

Your suggested fix sounds right

@ChrisLoer
Copy link
Contributor

I think I can build a good fix for this on top of PR #6497...

ChrisLoer added a commit that referenced this issue Apr 13, 2018
@ChrisLoer ChrisLoer mentioned this issue Apr 13, 2018
5 tasks
ChrisLoer added a commit that referenced this issue Apr 13, 2018
ChrisLoer added a commit that referenced this issue Apr 13, 2018
@ChrisLoer
Copy link
Contributor

Fixed in #6497.

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

No branches or pull requests

3 participants