Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] increase padding in CollisionIndex for MapMode::Tile #15880

Merged
merged 3 commits into from
Dec 6, 2019

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Nov 1, 2019

When rendering image tiles using MapMode::Tile we need to avoid clipped labels at tile boundaries. Clipped labels happen when one tile decides a label can be shown and the other tile decides that it collides with something and needs to be hidden.

When rendering overscaled image tiles (rendering z17 image tiles for a vector tile source that only goes up to z16) we should use all the data from the tile instead of just the data that fits within the image tile.

This increases the padding to 1024 for MapMode::Tile which should be enough to completely include all the data two zoom levels past the data source's max zoom level. Beyond that, 1024 should be enough of a padding to make clipped labels unlikely.

An alternative would be to calculate the max zoom levels of all the sources used and dynamically set the padding on all sides but increasing the constant seems as good and less complex.

@springmeyer
Copy link
Contributor

Thanks for putting this up as a PR @ansis. As a prerequisite to reviewing, on my end, I just tried to build locally to check the test coverage. But 1) I'm not seeing a way to see test coverage locally - is there an established best practice here? and 2) I hit #15881 so currently I'm blocked on running the unit tests: is there an existing test for this behavior or do we need to add one?

src/mbgl/text/collision_index.hpp Outdated Show resolved Hide resolved
@pozdnyakov
Copy link
Contributor

@ansis is it fine to be merged?

@springmeyer
Copy link
Contributor

I'm not seeing a way to see test coverage locally - is there an established best practice here?

@pozdnyakov do you know the answer to this? I've finally gotten my local build working, so unless there is already a way to do this, I'm planning on trying to get local coverage generation working.

The reason I'm interested in local coverage is for branch coverage details. It seems essential to me that we ensure there is there is test coverage for both mapMode == MapMode::Tile and mapMode != MapMode::Tile in the code changed in this PR. Currently the codecov.io based coverage reporting does not display branch information. So it is not possible for me to verify that both of the scenarios at https://codecov.io/gh/mapbox/mapbox-gl-native/src/7665aa8073b77346bcb50c8299d854f663259ba7/src/mbgl/text/collision_index.cpp#L31 are covered by tests (without testing locally).

So, I do not think this is not ready to merge yet. Before merging we should have confidence that the logic in this PR is tested under both MapMode::Tile and non-tiled modes.

@pozdnyakov
Copy link
Contributor

@springmeyer sorry for missing your question, IMO a render test could suffice for verifying this PR's intended behavior. Actually, render tests provide even better QA than unit tests. Our test runner supports tile mode (e.g. pls see mapbox/mapbox-gl-js#8899)

@springmeyer
Copy link
Contributor

Thanks @pozdnyakov - sounds good then to merge this if it has a render test covering it. Does it?

Also, I never found time to run coverage locally, but I'll create a ticket documenting my setup if I ever get around to it.

ansis added 2 commits December 6, 2019 17:11
Increase the CollisionIndex's padding so that more of a tile's data gets
considered when checking for collisions. This should fix clipped
variable placement text labels.
@ansis ansis force-pushed the collision-padding-mapmode-tile branch from cea1aa6 to f94d2d4 Compare December 6, 2019 22:12
@ansis ansis force-pushed the collision-padding-mapmode-tile branch from f94d2d4 to d733f53 Compare December 6, 2019 22:23
@ansis ansis merged commit cdee5b8 into master Dec 6, 2019
@ansis ansis deleted the collision-padding-mapmode-tile branch December 6, 2019 23:11
@springmeyer
Copy link
Contributor

@ansis does this have a render test covering it?

@springmeyer
Copy link
Contributor

Bump @ansis @pozdnyakov @tmpsantos - does this code have render-test coverage or not?

@pozdnyakov
Copy link
Contributor

Bump @ansis @pozdnyakov @tmpsantos - does this code have render-test coverage or not?

The existing text-variable-anchor/all-anchors-tile-map-mode and map-mode/tile render tests will run the modified code, but there is no a dedicated test that would fail without this change. The work on enabling the necessary infra for such test is ongoing: https://github.com/mapbox/mapbox-gl-native-team/issues/94#issuecomment-564550546

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants