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

Set tile#queryPadding per layer #6909

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

ryanhamley
Copy link
Contributor

@ryanhamley ryanhamley commented Jul 3, 2018

Launch Checklist

  • briefly describe the changes in this PR
    • fixes Regression: mouseenter event is fired on wrong layer #6896
    • queryPadding was always being calculated based on the first layer in a bucket which resulted in a bug in which two layers using the same source could introduce the bug seen in the ticket
    • this PR changes queryPadding to be calculated for each layer in a bucket
  • manually test the debug page

@ryanhamley ryanhamley requested a review from ansis July 3, 2018 19:38
mourner
mourner previously requested changes Jul 5, 2018
@@ -434,7 +435,8 @@ class Tile {

bucket.update(sourceLayerStates, sourceLayer);
if (painter && painter.style) {
this.queryPadding = Math.max(this.queryPadding, painter.style.getLayer(bucket.layerIds[0]).queryRadius(bucket));
const index = bucket.layerIds.indexOf(id);
this.queryPadding = Math.max(this.queryPadding, painter.style.getLayer(bucket.layerIds[index]).queryRadius(bucket));
Copy link
Member

Choose a reason for hiding this comment

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

layerIds[layerIds.indexOf(id)] is simply id, so no need to do indexOf lookups in both places.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Please add a query integration test for this as well.

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

The fix looks good but I have a question about the query test.

What is the query test trying to cover? Would it need multiple layers with different line-widths or something like that in order to test the bug this pr fixes?

@ryanhamley
Copy link
Contributor Author

The query test isn't working as is @ansis. I committed it just to have my most up to date work in the PR. I haven't written one before and was confused about some of the behavior and what would be the best way to write it. I'm definitely open to suggestions on the best way to test this.

I assumed that actual.json would have state: { "hover": true } but state is actually an empty object. I'm guessing that I'm not fully understanding the nature of writing a query test.

@ryanhamley ryanhamley force-pushed the 6896-queryrenderfeatures-regression branch from 1a885bc to 1f19d34 Compare August 3, 2018 22:24
@ryanhamley
Copy link
Contributor Author

@ansis I finally got around to fixing the query test for this. It's just checking that a feature is found under the query geometry in the case where you have two features in the same bucket. I verified that this test is correct by reverting the fix and confirming that the test fails. So I think this PR is good to go now.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Query Tests are 👍

@ryanhamley ryanhamley force-pushed the 6896-queryrenderfeatures-regression branch from 9c613f6 to a99c168 Compare August 7, 2018 20:24
@ryanhamley ryanhamley changed the title Set tile#queryPadding per feature Set tile#queryPadding per layer Aug 7, 2018
@ryanhamley ryanhamley merged commit a99c168 into master Aug 7, 2018
@ryanhamley ryanhamley deleted the 6896-queryrenderfeatures-regression branch August 7, 2018 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: mouseenter event is fired on wrong layer
5 participants