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 data-driven property evaluation in queryRenderedFeatures #10074

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Fix data-driven property evaluation in queryRenderedFeatures #10074

merged 2 commits into from
Nov 13, 2020

Conversation

osvodef
Copy link
Contributor

@osvodef osvodef commented Nov 4, 2020

Bug description

Evaluated properties returned by queryRenderedValues have the same values across the same layers, even if they are data-driven and need to be different.

JSFiddle

https://jsfiddle.net/osvodef/tj8boaxd/17/
This queries all settlement labels and outputs their text fields in the console.

Solution

This bug was due to serialized layers being cached, and multiple features of the same layer sharing the same serialized layer object. I fixed this by taking a shallow copy of the cached layer for each individual feature.

Launch Checklist

  • briefly describe the changes in this PR
  • manually test the debug page

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.

Nicely spotted and thanks a lot for the fix! Could you also add a unit test for this to make sure this doesn't regress in future?

@osvodef
Copy link
Contributor Author

osvodef commented Nov 13, 2020

@mourner I couldn't write a unit test ☹️ I tried adding a case to Style#queryRenderedFeatures, but those tests mock out the modified code, so it doesn't run at all.

I managed to add a query test instead that properly covers the regression and fails in main as expected.

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.

Amazing, thanks so much! A query test is even better because GL Native will be tested against it as well.

@mourner mourner merged commit ee6136c into mapbox:main Nov 13, 2020
@osvodef osvodef deleted the query-rendered-features-evaluation branch November 14, 2020 11:25
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.

2 participants