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

[core] Restore AnnotationSource maximum zoom range to 22 #5517

Merged
merged 3 commits into from
Jul 26, 2016
Merged

[core] Restore AnnotationSource maximum zoom range to 22 #5517

merged 3 commits into from
Jul 26, 2016

Conversation

brunoabinader
Copy link
Member

@brunoabinader brunoabinader commented Jun 30, 2016 •
edited
Loading

This is a work-in-progress PR.

So far:

  • Implemented a unit test that reproduces the missing annotation on z19: Annotations.AnnotationZoomRange
  • Fixed tolerance level when generating the GeoJSONVT shape tiler in ShapeAnnotationImpl::updateTileData - this change makes the subsequent query for shapeTile.features return a non-empty TileFeature.
  • However, TileFeature still contains an empty tile ring in its geometry. This is what makes queryRenderedFeatures return no features in the test case, and causes the annotations to disappear in zoom 19.

For the last item, I'm suspecting either 1) an erroneous parameter passed out to GeoJSONVT via Options struct or 2) an unknown bug inside geojson-vt-cpp.

Fixes #5505.

👀 @mourner @kkaefer @jfirebaugh

@brunoabinader brunoabinader added bug ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl annotations Annotations on iOS and macOS or markers on Android labels Jun 30, 2016
@brunoabinader brunoabinader force-pushed the 5505-annotation-maxzoom-22 branch from dc3f7cd to 184cf1f Compare June 30, 2016 11:45
@brunoabinader
Copy link
Member Author

I've compiled geojson-vt-cpp 4.1.2 (current version @ GL Native) in debug mode and got the following output:

tile z18-131071-131072 (features: 1, points: 2, simplified: 2)
creation: 0.01ms
drilling down: 0.05ms
shapeFeature.tileGeometry.shapeRing.size: 2
drilling down to z18-131071-131071
found parent tile z17-65535-65535
clipping: 0.01ms
tile z18-131071-131071 (features: 1, points: 2, simplified: 2)
creation: 0.01ms
drilling down: 0.05ms
shapeFeature.tileGeometry.shapeRing.size: 2
drilling down to z18-131072-131072
found parent tile z17-65536-65536
clipping: 0.01ms
tile z18-131072-131072 (features: 1, points: 2, simplified: 2)
creation: 0.01ms
drilling down: 0.04ms
shapeFeature.tileGeometry.shapeRing.size: 2
drilling down to z18-131072-131071
found parent tile z17-65536-65535
clipping: 0.02ms
tile z18-131073-131070 (features: 1, points: 2, simplified: 2)
creation: 0.01ms
tile z18-131072-131071 (features: 1, points: 2, simplified: 2)
creation: 0.01ms
tile z18-131072-131070 (features: 1, points: 2, simplified: 2)
creation: 0.00ms
drilling down: 0.07ms
shapeFeature.tileGeometry.shapeRing.size: 2
drilling down to z19-262144-262143
found parent tile z18-131072-131071
clipping: 0.03ms
tile z19-262145-262142 (features: 1, points: 2, simplified: 0)
creation: 0.01ms
tile z19-262144-262143 (features: 1, points: 2, simplified: 0)
creation: 0.01ms
tile z19-262144-262142 (features: 1, points: 2, simplified: 0)
creation: 0.01ms
drilling down: 0.11ms
shapeFeature.tileGeometry.shapeRing.size: 0
drilling down to z19-262144-262144
found parent tile z18-131072-131072
clipping: 0.01ms
tile z19-262144-262144 (features: 1, points: 2, simplified: 0)
creation: 0.01ms
drilling down: 0.04ms
shapeFeature.tileGeometry.shapeRing.size: 0
drilling down to z19-262143-262143
found parent tile z18-131071-131071
clipping: 0.01ms
tile z19-262143-262143 (features: 1, points: 2, simplified: 0)
creation: 0.01ms
drilling down: 0.04ms
shapeFeature.tileGeometry.shapeRing.size: 0
drilling down to z19-262143-262144
found parent tile z18-131071-131072
clipping: 0.01ms
tile z19-262143-262144 (features: 1, points: 2, simplified: 0)
creation: 0.01ms
drilling down: 0.03ms
shapeFeature.tileGeometry.shapeRing.size: 0

As it seems, tile generation for z19 returns empty simplified features. Interesting to check with latest geojson-vt-cpp to see if the same behavior happens (/cc @yhahn).

@brunoabinader brunoabinader force-pushed the 5505-annotation-maxzoom-22 branch 2 times, most recently from a8e2487 to 50ce31e Compare June 30, 2016 12:52
@mourner
Copy link
Member

mourner commented Jun 30, 2016

Capturing from chat:

@brunoabinader: given the unit test I added yesterday to geojson-vt-cpp doesn't capture the issue I see in #5517 (comment) - I'm ought to believe that this has been fixed between versions 4.1.2 and 6.0.0

@brunoabinader brunoabinader force-pushed the 5505-annotation-maxzoom-22 branch from 50ce31e to 040566a Compare June 30, 2016 13:21
jfirebaugh
jfirebaugh reviewed Jun 30, 2016
View reviewed changes
src/mbgl/style/source_impl.cpp
@@ -207,16 +207,23 @@ static Point<int16_t> coordinateToTilePoint(const UnwrappedTileID& tileID, const
}

std::unordered_map<std::string, std::vector<Feature>> Source::Impl::queryRenderedFeatures(const QueryParameters& parameters) const {
std::unordered_map<std::string, std::vector<Feature>> result;
if (renderTiles.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The algorithm works fine without checking these boundary conditions right? Why increase the amount of code?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an early return - avoids unnecessary creation of queryGeometry.

@brunoabinader brunoabinader mentioned this pull request Jun 30, 2016
Merged
7 tasks
@yhahn yhahn self-assigned this Jul 6, 2016
@brunoabinader brunoabinader mentioned this pull request Jul 22, 2016
Closed
@brunoabinader brunoabinader force-pushed the 5505-annotation-maxzoom-22 branch from 040566a to 253a3db Compare July 26, 2016 13:35
@brunoabinader brunoabinader added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jul 26, 2016
@brunoabinader brunoabinader merged commit 253a3db into master Jul 26, 2016
@brunoabinader brunoabinader deleted the 5505-annotation-maxzoom-22 branch July 26, 2016 14:37
@1ec5 1ec5 mentioned this pull request Oct 13, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@yhahn yhahn

Labels
annotations Annotations on iOS and macOS or markers on Android bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brunoabinader @mourner @jfirebaugh @yhahn @1ec5