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 #8273: Layer#setPaintProperty returns true for CrossFadedDataDriv… #8289

Merged
merged 3 commits into from
May 31, 2019

Conversation

ahk
Copy link
Contributor

@ahk ahk commented May 28, 2019

This patch fixes a bug reported in issue #8273. Users were unable to update line-pattern property images via Map#setPaintProperty. There is only one change and it removes an unnecessary check that the previous value for this property didn't exist. As long as the value has changed (which is checked in Style#setPaintProperty before deferral to the layer) then the layer needs to be updated, or for example the image used for the property value isn't added to the texture atlas.

It's possible this affects performance, but benchmarks didn't seem to indicate that. I can imagine that some updates from a non-image value would only need re-layout on changing to or from a null value. Any opinions on that question are greatly appreciated.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page

@peterqliu
Copy link
Contributor

👍 want to add a new test ensuring this keeps working moving forward?

@ahk
Copy link
Contributor Author

ahk commented May 29, 2019

👍 want to add a new test ensuring this keeps working moving forward?

Is it correct to try to land a test in test/unit/style/style_layer.test.js ?

@peterqliu
Copy link
Contributor

@ahk yep, set it up to reproduce the erstwhile error state and make sure getPaintProperty returns what you expect

can also do a render test to make sure the proper pattern applies, but seems like a unit test should suffice

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.

I think adding a render test would be more useful than a unit test for this case.

@ahk
Copy link
Contributor Author

ahk commented May 29, 2019

@asheemmamoowala I read through a ton of the render tests, wanted to make sure my thinking was correct before I created one. I think I should land a test into the regressions directory. I feel this test is the best example of what I'm trying to do (exercise StyleLayer#setPaintProperty), I just want to switch out fill-pattern for line-pattern as the property I'm updating.

@mourner
Copy link
Member

mourner commented May 30, 2019

@ahk correct! One small note is that we're trying to add small-sized tests if possible (so not 512x512 like the one above, but e.g. 64x64 if it still can exercise the bug).

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.

🚢

@ahk
Copy link
Contributor Author

ahk commented May 30, 2019

fix-#8273-benchmarks

@ahk ahk merged commit 158ff5d into master May 31, 2019
@ahk ahk deleted the fix-8273 branch May 31, 2019 21:04
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.

4 participants