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 queryRenderedFeatures for symbol features #3459

Merged
merged 3 commits into from
Oct 25, 2016

Conversation

brunoabinader
Copy link
Member

Use the query geometry as polygon for intersecting with symbol features collision box. Fixes situations where e.g. unseen features were returned or vice-versa.

This PR is a port from GL native (see mapbox/mapbox-gl-native#6773).

Query test symbol-features-in/pitched-screen (#3447) should be passing now, but because the feature order differs from GL native, the test fails. We should probably sort features by latitude/longitude prior to JSON comparison.

Benchmark reports are within standard deviation:
screen shot 2016-10-25 at 4 51 33 pm

/cc @ansis @jfirebaugh @mourner

@brunoabinader brunoabinader added perspective GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform labels Oct 25, 2016
@brunoabinader
Copy link
Member Author

Possible fix for #2647.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, let's 🚢 after addressing my minor remarks.

const sourceLayer = blocking.sourceLayerIndex;
const featureIndex = blocking.featureIndex;
if (sourceLayerFeatures[sourceLayer] === undefined) {
sourceLayerFeatures[sourceLayer] = {};
}
return sourceLayerFeatures[sourceLayer][featureIndex] === true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this function does not need any variables from the outer scope (it gets everything it needs from arguments), move it out to the top level scope. This way you avoid the cost of setting up a closure on every call of queryRenderedSymbols.

// Check if feature is rendered (collision free) at current scale.
function visibleAtScale(blocking, roundedScale) {
return roundedScale >= blocking.placementScale && roundedScale <= blocking.maxScale;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this function.

new Point(x1, y2)
];
return intersectionTests.polygonIntersectsPolygon(rotatedQuery, bbox);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one too, if you pass bbox as an argument.

@@ -0,0 +1,52 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be consolidated with util/intersection_tests.js?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my - totally missed this - yes we can certainly reuse it 👍

@brunoabinader
Copy link
Member Author

Thank you @mourner and @jfirebaugh for the review - I've addressed the requested changes.

@brunoabinader
Copy link
Member Author

@mourner it turns out those functions made more sense in GL native context because we were using them as predicates for boost::geometry::index::query. I guess for GL JS we can just get rid of the extra overhead and drop its contents directly into the main function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants