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

MBTiles Support Sprint #982

Closed
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@hallahan
Contributor

hallahan commented Sep 19, 2016

I think the MBTiles work is ready for review. This PR is the implementation of #960 & #931. In short, you can now have your tiles cached and stored in an MBTiles SQLite3 database for offline usage.

Overview

You can declare a data source to have an mbtiles file path both programmatically and via your scene.yaml.

If you want to add tiles to your cache online, you can put both a url and mbtiles in your data source, and http tile requests will be written to your MBTiles db. If the MBTiles file does not exist, Tangram ES will attempt to create a fresh MBTiles file all set up with the proper schema at that path given.

sources:
    osm:
        type: GeoJSON
        url:  https://vector.mapzen.com/osm/all/{z}/{x}/{y}.json
        mbtiles: /Users/njh/osm-data/mbtiles/winters-mapzen-geojson.mbtiles
        max_zoom: 16
        url_params:
            api_key: vector-tiles-tyHL4AY

You don't need a URL, however. You can have your data source simply fetch tiles from the MBTiles and not request anything from the network.

sources:
    osm:
        type: GeoJSON
        mbtiles: /Users/njh/osm-data/mbtiles/winters-mapzen-geojson.mbtiles
        max_zoom: 16

This works nicely when you have full knowledge and access to your filesystem. On Android, however, you might need to get a file path to your MBTiles programmatically first.

File storageDir = Environment.getExternalStorageDirectory();
File mbtilesFile = new File(storageDir, "tangram-geojson-cache.mbtiles");
map.setMBTiles("osm", mbtilesFile);

This would set the file tangram-geojson-cache.mbtiles to be your MBTiles in the osm datasource. It will use a URL from your scene.yaml to populate your cache just like before (or not). Similarly, Map::setMBTiles is exposed from the native C++ Map interface as well. We will also want to add Objective C bindings.

MBTiles Spec

The MBTiles database schema is built to adhere to @pnorman 's MBTiles 2.0 specification. The metadata table is being populated with the required values declared. In addition to adhering to the spec itself, I'm also creating MBTiles databases with identical schema to the most widely used reference implementation, node-mbtiles. You can see the creation of the database in mbtiles.h. One addition that I'd like to make is that we check the metadata table and make sure that the data format is consistent with what we have declared in our scene.yaml.

SQLiteCpp

I'm quite happy with the use of SQLiteCpp C++ SQLite3 wrapper library. It integrates easily with the CMake build process. It works without any issue on Mac OS X and Android, and it's usage is part of the core library. One thing that sets this MBTiles implementation apart from the others is that it is done in the core C++ library. We do not re-invent the wheel and re-implement for Android, iOS, etc.

MD5 Checksum Hash

One clever feature found in node-mbtiles that is not mentioned in the spec is that the underlying database schema has a map and images table that are joined to get you your tile data. The key that joins these tables is an MD5 checksum of the actual tile data. If you have many tiles with duplicate or empty data structures, rather than having a repeat entry in the images table, the checksum will join you to a single entry in images to all of the tiles pointing to that same data.

Rather than integrating a big encryption library like OpenSSL, I just wanted the MD5 function. So, after a bit of searching, I found a C++ hash library that was built to be portable and easy to integrate. I was able to bring in only the MD5 function, therefore saving us from adding extra clutter to the code base. I have a readme that goes into greater detail. I did manually test the hash values with several other MD5 generators and found that the output values matched with the same input.

MBTiles Tile Task

You'll notice that reads and writes to the database are being done inside of an MBTilesTileTask. This class is a subclass of DownloadTileTask, and it is run a tile worker thread, so we can do our blocking call to the database without trouble.

FontContext::fetch Segfault Bug

The way in which we are adding an MBTiles file to a data source is via a scene update. One bug I kept running into is that if we have a url font resource in the scene.yaml, we get segfaults on android when startUrlRequest is called to fetch a remote font. This will always happen in a scene update unless we remove that remote font URL. See #978 .

Future Work

Compression

I held off on implementing compression of the vector data in the database for now. It's definitely something that I'd like to have on by default, but it isn't an essential functionality. I'm thinking it will be good to wait until we've decided on a compression library to work with multi-file scene archives. I'd like to use the same library to compress vector data here.

Documentation

We definitely will want a tutorial and documentation to explain the usage of this feature. It will be important to explain how the mbtiles parameter behaves in the scene.yaml. Also, we'll want an example in the tangram-android-examples project.

MBTiles Spec Merge

It sounds like there still needs to be more discussion to convince Mapbox to merge the pull request. We may need to do some tweaks to our implementation if the spec has to change.

Unit Tests

It would be a good idea to include unit tests, particularly as this feature matures. I've used Catch in projects before, and it is should be pretty straightforward to add some tests with an MBTiles fixture. This should become more important when there is a Mapzen backend providing MBTiles files with data of various formats. Checking to make sure that we are reading and writing data in the format that we think it is will become important. Also, making sure that the data matches what is being stated in the metadata will become more important.

iOS Objective C API

We will want to expose this functionality in the TGMapViewController as well. That should be simple.

Mapzen MBTiles Metro Extracts

One exciting endeavor will be to include MBTiles with Mapzen's Metro Extracts. I believe that this will be a valuable resource for people to obtain offline maps.

cc/ @tallytalwar @blair1618 @pnorman @bcamper

hallahan added some commits Sep 3, 2016

Merge remote-tracking branch 'tangrams/master' into core-mbtiles
# Conflicts:
#	android/demo/build.gradle
#	android/gradle/wrapper/gradle-wrapper.properties
#	android/tangram/build.gradle

Also changed #include "platform.h" to #include "log.h" where appropriate.
@nvkelso

This comment has been minimized.

Show comment
Hide comment
@nvkelso

nvkelso Sep 19, 2016

Member

@hallahan We're hoping to have some support for offline tile packages thru Metro Extracts as you describe in 2016Q4, thanks for proposing the Tangram changes to support this on the client side!

Member

nvkelso commented Sep 19, 2016

@hallahan We're hoping to have some support for offline tile packages thru Metro Extracts as you describe in 2016Q4, thanks for proposing the Tangram changes to support this on the client side!

@hallahan

This comment has been minimized.

Show comment
Hide comment
@hallahan

hallahan Sep 20, 2016

Contributor

Looks like the Linux setup on Travis is failing. I just tried it out on my Ubuntu 16.04 box, and it's working fine.

Looking at SQLiteCpp's CMakeLists.txt, it looks like they add some special logic for Linux in the example build:

if (UNIX)
        target_link_libraries(SQLiteCpp_example1 pthread)
        if (NOT APPLE)
            target_link_libraries(SQLiteCpp_example1 dl)
        endif ()
        ...

https://github.com/SRombauts/SQLiteCpp/blob/b6512c4c6fbb964c6889387234d8a13a71b2a817/CMakeLists.txt#L237-L241

Would we want to put this in the linux.cmake? Looking at the linux.cmake, it seems to include the dynamic link library, but in a different way:

target_link_libraries(${EXECUTABLE_NAME}
    ${CORE_LIBRARY}
    -lcurl glfw
    # only used when not using external lib
    -ldl
    -pthread
    ${GLFW_LIBRARIES}
    ${OPENGL_LIBRARIES})
Contributor

hallahan commented Sep 20, 2016

Looks like the Linux setup on Travis is failing. I just tried it out on my Ubuntu 16.04 box, and it's working fine.

Looking at SQLiteCpp's CMakeLists.txt, it looks like they add some special logic for Linux in the example build:

if (UNIX)
        target_link_libraries(SQLiteCpp_example1 pthread)
        if (NOT APPLE)
            target_link_libraries(SQLiteCpp_example1 dl)
        endif ()
        ...

https://github.com/SRombauts/SQLiteCpp/blob/b6512c4c6fbb964c6889387234d8a13a71b2a817/CMakeLists.txt#L237-L241

Would we want to put this in the linux.cmake? Looking at the linux.cmake, it seems to include the dynamic link library, but in a different way:

target_link_libraries(${EXECUTABLE_NAME}
    ${CORE_LIBRARY}
    -lcurl glfw
    # only used when not using external lib
    -ldl
    -pthread
    ${GLFW_LIBRARIES}
    ${OPENGL_LIBRARIES})
@hjanetzek

This comment has been minimized.

Show comment
Hide comment
@hjanetzek

hjanetzek Sep 21, 2016

Member

Hey Nicholas -- thanks for pushing offline support forward!
I just had a quick look over the changes and it looks all good so far.

At this point I think DataSource needs some restructuring that I want to discuss with the team when they are back next week.

To fix the linux build, adding dl to core (instead of tangram executable) should fix it.

Member

hjanetzek commented Sep 21, 2016

Hey Nicholas -- thanks for pushing offline support forward!
I just had a quick look over the changes and it looks all good so far.

At this point I think DataSource needs some restructuring that I want to discuss with the team when they are back next week.

To fix the linux build, adding dl to core (instead of tangram executable) should fix it.

@hallahan

This comment has been minimized.

Show comment
Hide comment
@hallahan

hallahan Sep 22, 2016

Contributor

🎉 Travis is happy now! 🎉

@hjanetzek - Thanks for taking a look. Adding dl to the core CMake for UNIX AND NOT APPLE works, but I bet I could have specified this in a cleaner way. 6d7b7cb

Contributor

hallahan commented Sep 22, 2016

🎉 Travis is happy now! 🎉

@hjanetzek - Thanks for taking a look. Adding dl to the core CMake for UNIX AND NOT APPLE works, but I bet I could have specified this in a cleaner way. 6d7b7cb

@hallahan

This comment has been minimized.

Show comment
Hide comment
@hallahan

hallahan Oct 11, 2016

Contributor

Does anyone have any thoughts on how to move forward? Is this pull request something Mapzen would like to merge?

Contributor

hallahan commented Oct 11, 2016

Does anyone have any thoughts on how to move forward? Is this pull request something Mapzen would like to merge?

@tallytalwar

This comment has been minimized.

Show comment
Hide comment
@tallytalwar

tallytalwar Oct 11, 2016

Member

@hallahan Most of us were out for last 2 weeks, including 1 week of all mapzen conference.

We will be having a discussion on this, this week and get back to you.

Member

tallytalwar commented Oct 11, 2016

@hallahan Most of us were out for last 2 weeks, including 1 week of all mapzen conference.

We will be having a discussion on this, this week and get back to you.

@bcamper

This comment has been minimized.

Show comment
Hide comment
@bcamper

bcamper Oct 11, 2016

Member

Yes, thanks for all your work here @hallahan. We are definitely interested, it's just been a matter of time available to think through the architecture (probably refactoring data sources, considering a plugin system, etc.). We expect to be able to support this one way or another though.

Member

bcamper commented Oct 11, 2016

Yes, thanks for all your work here @hallahan. We are definitely interested, it's just been a matter of time available to think through the architecture (probably refactoring data sources, considering a plugin system, etc.). We expect to be able to support this one way or another though.

@nvkelso

This comment has been minimized.

Show comment
Hide comment
@nvkelso

nvkelso Oct 11, 2016

Member

We're also moving forward with offline vector tile package support via our Metro Extracts product.

Please for @iandees to post a link to the New York City sample here.

Member

nvkelso commented Oct 11, 2016

We're also moving forward with offline vector tile package support via our Metro Extracts product.

Please for @iandees to post a link to the New York City sample here.

@hallahan

This comment has been minimized.

Show comment
Hide comment
@hallahan

hallahan Oct 11, 2016

Contributor

I agree that some refactoring of the data source architecture might clean things up. In particular, some of the constructors are getting pretty gnarly.

Contributor

hallahan commented Oct 11, 2016

I agree that some refactoring of the data source architecture might clean things up. In particular, some of the constructors are getting pretty gnarly.

@tallytalwar

Just some initial review coming in here from me. Will continue this tomorrow.
So far looking great @hallahan 👍

url.find("{y}") != std::string::npos &&
url.find("{z}") != std::string::npos;
// If we have MBTiles, we know the source is tiled.
if (mbtiles.size() > 0) {

This comment has been minimized.

@tallytalwar

tallytalwar Oct 12, 2016

Member

nitpick.

We can club this in the above bool tiled setting.

@tallytalwar

tallytalwar Oct 12, 2016

Member

nitpick.

We can club this in the above bool tiled setting.

This comment has been minimized.

@hallahan

hallahan Oct 18, 2016

Contributor

👍

@hallahan

hallahan Oct 18, 2016

Contributor

👍

auto task = std::make_shared<DownloadTileTask>(_tileId, shared_from_this(), _subTask);
std::shared_ptr<DownloadTileTask> task;
if (hasMBTiles()) {
task = std::make_shared<MBTilesTileTask>(_tileId, shared_from_this(), _subTask);

This comment has been minimized.

@tallytalwar
@tallytalwar
// If the data did not come from the in-memory cache, it is new.
// It should be added to the MBTiles.
if (!dataFromCache) {
putMBTilesData();

This comment has been minimized.

@tallytalwar

tallytalwar Oct 12, 2016

Member

Generic question, how much of performance gain we get when we grab the date from cache, vs when we run the query to grab the tile data every time. Just debating on memory vs performance benefit here?

@tallytalwar

tallytalwar Oct 12, 2016

Member

Generic question, how much of performance gain we get when we grab the date from cache, vs when we run the query to grab the tile data every time. Just debating on memory vs performance benefit here?

This comment has been minimized.

@hallahan

hallahan Oct 18, 2016

Contributor

Good question. I implemented it as you see because I didn't want to change the in-memory functionality.

@hallahan

hallahan Oct 18, 2016

Contributor

Good question. I implemented it as you see because I didn't want to change the in-memory functionality.

@hjanetzek

This comment has been minimized.

Show comment
Hide comment
@hjanetzek

hjanetzek Oct 17, 2016

Member

Hey @hallahan I've started the DataSource refactoring and also ported your MBTiles code over to the new interface at #1015. Please have a look if you find something missing or have ideas for improvement.
#1014

Member

hjanetzek commented Oct 17, 2016

Hey @hallahan I've started the DataSource refactoring and also ported your MBTiles code over to the new interface at #1015. Please have a look if you find something missing or have ideas for improvement.
#1014

@lygstate

This comment has been minimized.

Show comment
Hide comment
@lygstate

lygstate Oct 23, 2016

I suggest using the following schema:

sources:
    osm:
        type: GeoJSON
        # for posix
        url: mbtiles:///Users/njh/osm-data/mbtiles/winters-mapzen-geojson.mbtiles
        # for Windows
        #url: mbtiles://C:/Users/njh/osm-data/mbtiles/winters-mapzen-geojson.mbtiles
        max_zoom: 16

lygstate commented Oct 23, 2016

I suggest using the following schema:

sources:
    osm:
        type: GeoJSON
        # for posix
        url: mbtiles:///Users/njh/osm-data/mbtiles/winters-mapzen-geojson.mbtiles
        # for Windows
        #url: mbtiles://C:/Users/njh/osm-data/mbtiles/winters-mapzen-geojson.mbtiles
        max_zoom: 16

@hallahan hallahan referenced this pull request Nov 30, 2016

Closed

Offline Maps #1115

@tallytalwar

This comment has been minimized.

Show comment
Hide comment
@tallytalwar

tallytalwar Feb 7, 2017

Member

Probably make sense to close this PR in favor of #1015. #1015 already squashes all the commits in here into a single commit: b5c4dc1.

Nice description though in this PR :D.

Member

tallytalwar commented Feb 7, 2017

Probably make sense to close this PR in favor of #1015. #1015 already squashes all the commits in here into a single commit: b5c4dc1.

Nice description though in this PR :D.

@matteblair matteblair closed this Feb 7, 2017

@ivangorelow ivangorelow referenced this pull request Feb 9, 2017

Closed

offline map android #1281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment