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

[core] Implement TileCover for polygonal regions #11267

Merged
merged 11 commits into from
Apr 26, 2018

Conversation

asheemmamoowala
Copy link
Contributor

@asheemmamoowala asheemmamoowala commented Feb 21, 2018

The existing scanline-based tile cover implementation blocks when computing large rectangles. When extended to polygonal regions, this would further impact the performance and block interactivity in end-user applications.

This PR implements a streaming tile cover algorithm. An edge table-like data structure is created for all geometry, and then a modified scan-line is used for each tile row. Each row requires just one scan-line
while accumulating the x-extents for each edge within that row.

Usage:

    mbgl::util::TileCover tc(LatLngBounds::world(), zoom);
    while(tc.hasNext()) {
        UnwrappedtileID tile = *tc.next();
        ...
    };
  • Unit tests for polygons with holes and multi-polygons
  • A wrapper method util::tileCover that accepts arbitrary geometry and returns tiles.
  • Benchmark tests for tile cover

cc @ivovandongen @mourner

@asheemmamoowala asheemmamoowala added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Feb 21, 2018
static Point<double> project(const LatLng& latLng, uint8_t zoom) {
return project_(latLng, std::pow(2.0, zoom));
//Returns point on tile
static Point<double> project(const LatLng& latLng, int32_t zoom) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method projects to a tile fraction and not to a tile-size based world. I'm not sure if it should be called project or exposed from TileCoordinate

};

//These are all different from the js tile-cover lib
EXPECT_EQ(1742u, util::tileCover(spikyGeom, 10).size());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect that this test has different results based on floating point precision difference between the JS and CPP implementations. It's hard to reproduce with a small test case.

@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Feb 23, 2018
@asheemmamoowala asheemmamoowala changed the base branch from master to release-boba March 14, 2018 16:00
@asheemmamoowala asheemmamoowala changed the base branch from release-boba to master March 14, 2018 16:00
@asheemmamoowala asheemmamoowala changed the base branch from master to release-boba March 15, 2018 01:33
@asheemmamoowala asheemmamoowala changed the base branch from release-boba to master March 22, 2018 07:52
@asheemmamoowala asheemmamoowala added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 27, 2018
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.

So it did work after all? Awesome!


auto x_min = xps[0].x0;
auto x_max = xps[0].x1;
int8_t nzRule = xps[0].winding ? 1 : -1;
Copy link
Member

Choose a reason for hiding this comment

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

nzRule can overflow if the scan line intersects a ton of line strings, right? Maybe worth bumping int8_t to 16 or 32 just to be safe.

auto nw = Projection::project(bounds.northwest(), z);

Polygon<double> p({ {sw, nw, ne, se, sw} });
impl = new TileCoverImpl(z, p, false);
Copy link
Contributor

@ivovandongen ivovandongen Apr 10, 2018

Choose a reason for hiding this comment

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

Don't we want to use a smart pointer here instead?

Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

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

@asheemmamoowala Should we make the TileCover header public? That way we can use it, for exampleas a return type forOfflineRegionDefinition::tileCover()`.

@@ -3,6 +3,7 @@
#include <mbgl/tile/tile_id.hpp>
#include <mbgl/style/types.hpp>
#include <mbgl/util/tile_coordinate.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

This header is not used here atm right? (only non-public header included here)

@asheemmamoowala
Copy link
Contributor Author

@ivovandongen What is the use case for OfflineRegionDefinition::tileCover to be public?
The tileCover method is marked private currently.

If there is no public use case for the tileCover method, it could be moved into OfflineDownload or some other class where it can be used internally.

@ivovandongen
Copy link
Contributor

What is the use case for OfflineRegionDefinition::tileCover to be public?

Mostly to keep things encapsulated. Since there are wishes to expose tileCount on the offline region definitions (#11504 / #11108), it makes sense to keep both of them in the same place.

@ivovandongen
Copy link
Contributor

@asheemmamoowala I'm getting 3 test failures:

  • Point - Empty {}
  • MultiPoint - Empty {}
  • MultiLineString - Mismatch (This might be a wrong expectation on my part, but I remember changing it to a previous version and working)

See the test results here

}

} // namespace util
} // namespace mbgl
Copy link
Member

Choose a reason for hiding this comment

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

This set of files needs more documentation:

  • explanation of what every function does, and how the algorithm works
  • explanation of the cryptic variable names, or renaming those variable names to speak for themselves
    Additionally, there are some code style changes before merging:
  • adhere to Mapbox GL naming standards: CamelCase for class names, lowerCamelCase for members
  • apply principle of least access by making as many members private if possible
  • make as many variables const as possible if they're not modified after initialization

bool next();
//Invokes the ScanLine callback to indicate tiles coverd for the row from [x0,x1)
// ScanLine may be invoked with duplcaite or overlapping ranges.
bool getTiles(ScanLine&);
Copy link
Member

Choose a reason for hiding this comment

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

I think this API should work a bit differently, more like a generator, or C++ iterator rather than a function that accepts a callback so that the caller can drive iteration, rather than needing to process an entire scanline at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkaefer I considered implementing a ForwardIterator, but found that std::iterators are required to be CopyConstructible and CopyAssignable. Copying the TileCover::Impl state is non-trivial and would affect performance if the iterator was used incorrectly.

Per our chat, the streaming should be per-tile, and not per-row.

if (next_pt == points.end()) { next_pt = std::next(points.begin()); }
}
//Re-close the linear ring
points.erase(std::prev(points.end()));
Copy link
Member

Choose a reason for hiding this comment

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

This is what points.pop_back() does?

namespace util {

void start_list_on_local_minimum(point_list& points) {
assert(points.size() > 2);
Copy link
Member

Choose a reason for hiding this comment

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

Why can we assert that this is the case? What happens when iterating over a null geometry?

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Apr 17, 2018

Choose a reason for hiding this comment

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

This method is only called from build_edge_table when the point list is for a closed geometry. null geometry can be guarded against. It would be better to move the assert to a more appropriate method where the closed rings are being handled.

@asheemmamoowala
Copy link
Contributor Author

@ivovandongen Point and Multipoint should be fixed now. The MultiLineString issue was a good test case - that I hadn't considered. I incorporated all three cases into tilecover.test.cpp as well.

@ivovandongen
Copy link
Contributor

@asheemmamoowala Great. I've rebased on your changes and adapted to the streaming tile cover in #11447

Since you've included the test cases in tile cover, I removed the redundant ones on OfflineRegionDefinition and instead added tests for encode/decode.

@asheemmamoowala asheemmamoowala merged commit 4344812 into master Apr 26, 2018
@jfirebaugh jfirebaugh deleted the tile-cover-geometry branch April 30, 2018 18:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants