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

regression test for mapbox-gl-native#7241 #188

Merged
merged 1 commit into from
Dec 12, 2016

Conversation

ivovandongen
Copy link
Contributor

Fixes #187

@ivovandongen ivovandongen self-assigned this Dec 1, 2016
@ivovandongen ivovandongen force-pushed the 187-mapbox-gl-native#7241 branch from 9633a67 to 989fb6d Compare December 1, 2016 08:09
@ivovandongen
Copy link
Contributor Author

@jfirebaugh Strangely enough this test case also succeeds against master although this is exactly the case that mapbox/mapbox-gl-native#7241 fixes.

@jfirebaugh
Copy link
Contributor

@ivovandongen It looks like the unit test in #7242 also passes without the fix present. In this case, I'm seeing Style::onLayerVisibilityChanged() trigger both Update::RecalculateStyle and Update::Layout. The former results in enabled getting set to true, and then the tiles get loaded via style->updateTiles(parameters).

Is there some additional setup needed to trigger the issue that these tests are missing?

@ivovandongen
Copy link
Contributor Author

@jfirebaugh I've been looking into this. I've fixed the unit test, so it fails on master in the map test. It turns out there is a specific sequence that needs to happen so that the cache is filled with invalid tiles.

To reproduce this in a render test I need to be able to call render after certain steps so that the tiles are pruned and put into cache. However, I can't seem to do that right now as the render function takes a callback function as a second argument:

TypeError: Second argument must be a callback function

@jfirebaugh
Copy link
Contributor

@ivovandongen So the sequence is:

  • Add visible layer, render
  • Set non-visible, render
  • Set visible, render

?

You should be able to simulate that same sequence in a render test using the wait command, which has the same effect as test::render in unit tests.

@ivovandongen
Copy link
Contributor Author

@jfirebaugh That seems to be exactly the test I've implemented in 989fb6d. However, it doesn't seem to render on wait operations though (verified with some log statements).

I see in https://github.com/mapbox/mapbox-gl-native/blob/424c05be5e3fbd45e52d8e3a6cf048cbd5703b8d/platform/node/test/suite_implementation.js#L68 that there is a render call in wait. However, after a setVisiblity operation, the loaded state of the map/style/source doesn't change and render is never called, instead the next operation is performed immediately.

@jfirebaugh
Copy link
Contributor

Looks like there are a couple of issues:

  • The "wait" command is implemented using conditional logic that calls map.render() only if map.loaded() is false. I think we should change this to always use map.render() -- the expectation is that "wait" waits for a full render.
  • The maximum size of the source tile cache ends up being computed as 0 for small viewports.

I'll push a fix for these issues to the branch.

@jfirebaugh jfirebaugh force-pushed the 187-mapbox-gl-native#7241 branch from 989fb6d to 66397c8 Compare December 12, 2016 21:21
@jfirebaugh jfirebaugh merged commit 66397c8 into master Dec 12, 2016
@jfirebaugh jfirebaugh deleted the 187-mapbox-gl-native#7241 branch December 12, 2016 21:58
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