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

MBTiles DataSource #1015

Merged
merged 7 commits into from Feb 8, 2017
Merged

MBTiles DataSource #1015

merged 7 commits into from Feb 8, 2017

Conversation

@hjanetzek
Copy link
Member

hjanetzek commented Oct 14, 2016

No description provided.

@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 07e95b7 to 5f1c73c Oct 17, 2016
@hjanetzek hjanetzek mentioned this pull request Oct 17, 2016
@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from c7c023b to 4de0c68 Oct 18, 2016
@hjanetzek
Copy link
Member Author

hjanetzek commented Oct 18, 2016

Testing with OSM2Vectortiles http://osm2vectortiles.org/downloads/
shot-2016-10-18_13-19-20

@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 9002568 to a9319e6 Oct 18, 2016
@hallahan
Copy link
Contributor

hallahan commented Oct 18, 2016

This is awesome! I will be looking through this (and trying out with osm2vectortiles).

Copy link
Contributor

hallahan left a comment

Overall, I'm happy to see the direction you are bringing MBTiles support. I would have been hesitant to do such a refactor on my own, since I didn't want to make such drastic changes to the overall architecture of Tangram ES. Very cool!

/* vector of raster sources (as raster samplers) referenced by this datasource */
std::vector<std::shared_ptr<DataSource>> m_rasterSources;

std::unique_ptr<RawDataSource> m_sources;

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

I see what you're doing with adding MBTiles and Network data sources, but semantically, this is confusing. Data source, and all of the things derived from it, like MVT, GeoJSON, TopoJSON, are different in nature from MBTilesDataSource, NetworkCacheDataSource, etc.

I would say doing this makes the code overall less clear, however, coming up with a different name for RawDataSource and DataSource and the respective subclasses will likely add clarity.

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

Yes, I'm also not pleased with the RawDataSource and -DataSource naming. I thought of renaming DataSource to TileDataSource since the result of DataSource::parse is a TileData object. Still open for suggestions and thinking about further refactoring of DataSource

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 19, 2016

Contributor

TileSource sounds good.

@@ -6,15 +6,16 @@ namespace Tangram {

class GeoJsonSource: public DataSource {

public:
using DataSource::DataSource;

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

👍

@@ -27,19 +46,23 @@ class DataSource : public std::enable_shared_from_this<DataSource> {
* each of '{x}', '{y}', and '{z}' which will be replaced by the x index, y index,
* and zoom level of tiles to produce their URL.
*/
DataSource(const std::string& _name, const std::string& _urlTemplate,
DataSource(const std::string& _name, std::unique_ptr<RawDataSource> _sources,

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

What if you refactored DataSource and its subclasses to just be Data? MBTilesDataSource indeed describes the data source. MVTSource and the like describes the data that is retrieved from the source.

Also, should _sources be plural?

*
* https://github.com/mapbox/node-mbtiles/blob/4bbfaf991969ce01c31b95184c4f6d5485f717c3/lib/schema.sql
*/
const char* SCHEMA = R"SQL_ESC(BEGIN;

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

Do we want SCHEMA in the Tangram namespace globally?

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

no, just missing a static..


MBTilesQueries(SQLite::Database& _db, bool _offline)
: getTileData(_db, "SELECT tile_data FROM tiles WHERE zoom_level = ? AND tile_column = ? AND tile_row = ?;"),
putMap(_db, _offline ? "REPLACE INTO map (zoom_level, tile_column, tile_row, tile_id) VALUES (?, ?, ?, ?);" : "SELECT 1;" ),

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

What is the purpose of SELECT 1;?

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

Do we want to do a noop at the SQL level?

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

This is a quick workaround I put in yesterday to test with our Mapzen tilepacks which do not have a map table - needs cleanup.

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 19, 2016

Contributor

What are tilepacks? No map table?

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

The map table is not required by the spec - As you wrote somewhere, it's an optimization to store duplicate tile data only once. I'm currently working on initMBTiles to support all MBTiles following the spec.

Tilepacks is our working name for MBTile bundles.

@@ -80,10 +88,11 @@ styles:
sources:
osm:
type: MVT
url: https://vector.mapzen.com/osm/all/{z}/{x}/{y}.mvt
#url: https://vector.mapzen.com/osm/all/{z}/{x}/{y}.json
mbtiles: nyc.mbtiles

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

When you uncomment url, we're getting tiles from Mapzen and populating an MBTiles with data from osm2vectortiles. Since the data comes from different places, there's going to be many inconsistencies. We should somehow check for this.

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

yes, I think we should only write to mbtiles that were created by Tangram. How about adding a metadata entry for tangram_tile_cache and check for it when opening the db?

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 19, 2016

Contributor

Well, we might want to cache non-Mapzen sources too. What if I had a web service with vector tiles I wanted cached?

What if we had some sort of field in the metadata saying what the origin is? @pnorman might have some insight.

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 19, 2016

Contributor

Disable cross-origin tile sharing 😉

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

Having the tile url in the metadata would be an option to allow updating of externally created files. At the moment I'm adding the check to only write to the db when it contains description:MBTiles tile container created by Tangram ES. :)

This comment has been minimized.

Copy link
@pnorman

pnorman Nov 17, 2016

Contributor

Yes, you'll need to track that a mbtiles file has the same source if you want to avoid writing garbage to some other mbtiles file.

if (m_offlineMode) {
// Need to explicitly open a SQLite DB with OPEN_READWRITE
// and OPEN_CREATE flags to make a file and write.
mode = SQLite::OPEN_READWRITE | SQLite::OPEN_CREATE;

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

Perhaps we want to have a m_cacheMode. That way we permit creating and / or writing to a database allowed in this circumstance. Otherwise, we should be read only.

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

yeah, there are a few modes to consider.

  1. only use existing tiles
  2. try existing tiles, download and save
  3. try download, fallback to existing tiles
  4. try download and save to mbtiles, fallback to existing tiles

So far I intended to recreate the logic of the core-mbtiles branch.

oh - in this case I just wanted to make sure not to overwrite mbtiles packs :)


// OSM2Vectiles does not contain a 'compression' field
// Try deflate by default..
if (inflate(blob, length, _data) != 0) {

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

👍

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

Maybe when we open the database we should look and see if there is a compression field. If there is no compression field, I agree to try deflate by default and fall back to identity.

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

Sounds good!


namespace Tangram {

int inflate(const char* _data, size_t _size, std::vector<char>& dst);

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

This is awesome! Perhaps we should have a deflate function as well for when we populate the cache? As I was developing, I noticed that the cache got large pretty quickly.

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

Also, should inflate be in the Tangram global namespace?

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

👍, also moving to a separate namespace

int32_t minDisplayZoom = -1;
int32_t maxDisplayZoom = -1;
int32_t maxZoom = 18;

std::string mime;

if (type == "GeoJSON") {

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 18, 2016

Contributor

Perhaps we can discern the mime type automatically from the RawDataSoure by just returning the mime type of the DataSource? We will know the mime type of the DataSource based on the implementation of the DataSource. Then, you can put the logic of figuring out the mime type for raster sources in RasterSource instead of in here?

This comment has been minimized.

Copy link
@hjanetzek

hjanetzek Oct 19, 2016

Author Member

yes, this should be improved - I was not sure which way to go: when passing mime to MBTilesDataSource constructor *Source::getMime(string url) must be static.

This comment has been minimized.

Copy link
@hallahan

hallahan Oct 19, 2016

Contributor

Mime type for a tile source should be standard and well-known. Perhaps just have *Source::getMime(string url) return a constant string?

@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 33f47ef to 5867158 Oct 21, 2016
@lygstate
Copy link

lygstate commented Oct 23, 2016

Wonderful.

@lygstate
Copy link

lygstate commented Oct 23, 2016

We do not need add the mbtiles attribute,
just support for a new URL schema,
url: mbtiles://file_path;
That's would reduce much code effort.
And also suggest merge all the UrlWorker into a single place.
Do not implement them in platform code.
but in tangram-es/core directly.
I''ve done that in
#836

@matteblair
Copy link
Member

matteblair commented Oct 24, 2016

Hi @lygstate

I don't think we want to invent a new non-standardized URL scheme. An 'mbtiles' scheme wouldn't be usable with existing libraries the way that 'http' and 'file' are. A separate 'mbtiles' entry also allows us to add optional caching in a backwards-compatible way.

The design of UrlWorker isn't strictly related to this PR, but I'd gladly review those changes separately. (By the way, #836 hasn't been forgotten - thanks for staying active in the project!)

@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 33078a8 to 006881a Oct 31, 2016
@hallahan
Copy link
Contributor

hallahan commented Nov 15, 2016

Hi @hjanetzek - how is progress on this PR coming?

@hjanetzek
Copy link
Member Author

hjanetzek commented Nov 16, 2016

Hey @hallahan, I think the branch is in a good shape. DataSource refactoring needs to be reviewed first though and everybody is busy with things we need for the mobile SDKs - I hope we will get back to this soon.

@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 006881a to 9686185 Nov 16, 2016
@pnorman
Copy link
Contributor

pnorman commented Nov 17, 2016

This includes code for md5 sums. Is there something already included in one of the other libraries required that could be used, like sha256?

How can we make sure we only write to mbtiles files which are written in a way we understand? There is no guarantee that we can write to an arbitrary mbtiles file, even if it is storing vector tiles from the same source, because they could implement the mbtiles tables interface in a different way.

@bcamper
Copy link
Member

bcamper commented Nov 17, 2016

Good questions. @blair1618 will make a proposal as to Tangram syntax for handling the different read/write/fallback scenarios.

@hallahan hallahan mentioned this pull request Nov 30, 2016
@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 9686185 to 4e76ce7 Jan 2, 2017
@hallahan
Copy link
Contributor

hallahan commented Jan 5, 2017

Hey, how are things going? Any word on this PR and MBTiles support?

@tallytalwar
Copy link
Member

tallytalwar commented Jan 5, 2017

Yes, @hallahan. We were busy with a project which we finished in december. Will be giving this a good shot now, expect more action on this now :D.

Thanks for the patience and sorry for the delay on this.

@hjanetzek hjanetzek force-pushed the core-mbtiles-datasource branch from 4e76ce7 to 38963ef Jan 12, 2017
@tallytalwar tallytalwar self-requested a review Jan 17, 2017
@matteblair
Copy link
Member

matteblair commented Jan 18, 2017

I have not reviewed all of this yet, but so far it's looking mostly solid.

One problem I've noticed is that SQLiteCpp/sqlite3 seems to have its own implementation for opening and reading a file path. This has two results for us:

  • We need to resolve the mbtiles path from a scene file into an absolute path before we give it to sqlite (paths in the scene file might be absolute, but more often they are relative to the scene file's location).
  • Android applications currently can't bundle an mbtiles file. Android application bundles are effectively zip archives, and so there is no file path that identifies any individual file within the bundle. One could work around this by sneakily copying a bundled mbtiles database to a location on disk at run-time (given suitable permissions and with a start-up delay).
@matteblair
Copy link
Member

matteblair commented Jan 19, 2017

Configuration seems to still be an open question here. I see that suggestions have been made for new parameters in the scene file, which is the typical way that we introduce new features, but in this instance I feel that some of these configuration options go beyond "styling a scene" and enter the territory of application implementation. For example, the location of an mbtiles file will probably need to be different on Android and iOS because of their storage mechanisms (as in my previous comment). Even more generally, I don't know whether we would expect to ever support mbtiles files as a data source in the JS engine, though @bcamper may have thoughts here.

With these considerations in mind, I would suggest that an interface for configuring MBTiles data sources/caches should be part of the tangram-es API and not part of the scene file.

@nvkelso
Copy link
Member

nvkelso commented Jan 19, 2017

@tallytalwar
Copy link
Member

tallytalwar commented Jan 19, 2017

We do have an tangram ES level API to add an MBTile DS in this PR. But yes it utilizes updating the scene ultimately adds a DS via scene yaml load!

If JS has a use case of having offline MBTile Datasource, I am fine keeping it in the scene file description only.

@matteblair
Copy link
Member

matteblair commented Jan 19, 2017

@nvkelso I can imagine that an offline data cache of some kind would be useful for tangram-js, but it is unclear to me whether a SQLite database file is a good fit for this purpose. It seems undesirable to add a SQL library to the JS engine for just this use. And even if tangram-js did support reading MBTiles files, the issue would remain that it would likely use a different configuration (file location, timeouts, etc) than, say, an Android application.

@nvkelso
Copy link
Member

nvkelso commented Jan 19, 2017

@matteblair I recall @bcamper's opinion being similar. But it remains a valid use case that I think we should either support out of the box or provide documentation around the suggested workaround (e.g. running a local tileserver reading from the SQLite DB and masquerading it as a local tile endpoint).

@matteblair
Copy link
Member

matteblair commented Jan 19, 2017

@nvkelso Totally! It is a valid use case. I think there was also talk of using zip archives for offline use - tangram-js already has a zip reader so that might get us to a solution faster.

@pnorman 's concern about different access modes to an MBTiles file seems sensible to me - we will want our interface to encourage/enforce safe usage patterns.

I can currently see two use patterns for MBTiles files:

  1. Pre-downloaded data set, read only.

  2. Data cache created by tangram, read+write.

As long as we keep these roles distinct, we can be confident that we won't accidentally write incompatible entries to a database. The files used for these roles could be specified in clearly different ways.

Are there other use patterns that I'm not thinking of?

@tallytalwar
Copy link
Member

tallytalwar commented Feb 8, 2017

Will give the updates a look in a bit and merge if all good.

@bcamper
Copy link
Member

bcamper commented Feb 8, 2017

Agreed this is the right initial scope, thanks!

Copy link
Member

tallytalwar left a comment

There is a small bug in here. See the review comment below. Good otherwise.

Can you also add something in the tangram-docs (at least ES only for the time being) to make sure our users are aware of the current implementation.

Thanks


};

MBTilesDataSource::MBTilesDataSource(std::shared_ptr<Platform> _platform, std::string _name,

This comment has been minimized.

Copy link
@tallytalwar

tallytalwar Feb 8, 2017

Member

nitpick here ... can we get rid of the indent here?

This comment has been minimized.

Copy link
@matteblair

matteblair Feb 8, 2017

Member

Fixed. That's what I get for editing in Xcode haha

const char* extStr = ".mbtiles";
const size_t extLength = strlen(extStr);
const size_t urlLength = url.length();
isMBTilesFile = urlLength > extLength && (url.compare(urlLength - extLength, urlLength, extStr) == 0);

This comment has been minimized.

Copy link
@tallytalwar

tallytalwar Feb 8, 2017

Member

Hmm I think this should be:
url.compare(urlLength - extLength, extLength, extStr) == 0

To check if substring [arg1, arg1+arg2) in url is same as arg3!

This comment has been minimized.

Copy link
@tallytalwar

tallytalwar Feb 8, 2017

Member

Also probably make sense for tiled check here. If a url defines a tiled datasource (specified x,y,z) it can not be a mbtiles datasource.

This comment has been minimized.

Copy link
@matteblair

matteblair Feb 8, 2017

Member

You are correct, fixed!

This comment has been minimized.

Copy link
@matteblair

matteblair Feb 8, 2017

Member

Right - your comment about tiled. In the case of something like url: /w/{x}/{y}/{z}.mbtiles I would lean towards treating it as a path to an MBTiles file - curly braces are valid in paths and are somewhat common on Windows. I would not make it a recommended practice though ;)

This comment has been minimized.

Copy link
@tallytalwar

tallytalwar Feb 8, 2017

Member

Sure, again good to document.

@matteblair matteblair force-pushed the core-mbtiles-datasource branch from 3d4c657 to 6e87519 Feb 8, 2017
@tallytalwar
Copy link
Member

tallytalwar commented Feb 8, 2017

Will be merging this when CI builds are done.

@matteblair
Copy link
Member

matteblair commented Feb 8, 2017

Rebasing - hang tight

Update: all systems go

hallahan and others added 7 commits Sep 3, 2016
rebased+squashed mbtiles branch from Nicholas Hallahan

Added SQLiteCpp

Added mbtiles parameter to data sources and scene.yam #960

Created MBTilesTileTask. #960

Basic MBTiles functionality working! #960

No need to make 2 vectors. Writing SQLite blob directly into rawTileData #960

Database and Statements are class members.

Now caching network tiles in MBTiles. #960

Now creating fresh MBTiles caches. Using schema from node-mbtiles. #960

Working on dynamically setting MBTiles from Tangram::Map interface. #960

Remove setMBTiles functions. Going with Scene Updates. Also isOfflineOnly is now hasNoURL. #960

MBTiles Scene Update working on Android when commenting out fonts. #978 #960

Create Map::setMBTiles and exposing it through JNI #960

Added MD5 hash for join key on MBTiles tile and images tables #960

Write to metadata table when setting up MBTiles #960

ClientGeoJsonSource will never have MBTiles #960

fixup

disable failing cpplint.py checks

Implemented mimeType method in TestDataSource

compression: identity when there is no compression

linux target_link_libraries includes dl in core

Cleaner way of setting target_link_libraries to include dl for Linux.
squashed:
- wip: async loading for MBTiles
- implement MBTiles offline fallback mode
- wip: support compressed MBTiles
  - testing with osm2vectortiles planet
  - try deflate by default
- support general MBTiles schema
  - check metadata compression option
  - differentiate caching and offline-fallback mode
  - only write to MBTiles if the db was created by tangram
This removes the 'mbtiles' field from data sources and effectively hides the MBTiles caching interface until it can be refined further.
@matteblair matteblair force-pushed the core-mbtiles-datasource branch from 6e87519 to f6452aa Feb 8, 2017
@matteblair matteblair merged commit 08090f0 into master Feb 8, 2017
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matteblair matteblair deleted the core-mbtiles-datasource branch Feb 8, 2017
@tallytalwar tallytalwar mentioned this pull request Feb 9, 2017
@tallytalwar tallytalwar mentioned this pull request Mar 2, 2017
@rwrx
Copy link
Contributor

rwrx commented Jun 9, 2017

Is this MBTiles sources support usable for offline maps on Android? Have you tested it? I am just asking, I didn't tried nor looked into it. Thank you.

@tallytalwar
Copy link
Member

tallytalwar commented Jun 9, 2017

Hi @rwrx

Using MBTiles as an offline source of map tiles is working on all platforms we support. There are other usecases of MBTiles discussed in the (merged) PR which are not fully supported.

To use a MBTiles set in your scene you can refer to this comment above.

@rwrx
Copy link
Contributor

rwrx commented Jun 10, 2017

It is also possible to change that path to mbtiles file at runtime? I mean not only hardcode it into yaml scene file. And it is possible to have multiple mbtiles paths defined? I mean a use case where user has for example downloaded multiple mbtiles for countries and I need to show them on the map.

@rwrx
Copy link
Contributor

rwrx commented Jun 22, 2017

I had look at Tangram-ES code and now I understand as mbtiles file usage is meant there. So I have an answer to my question now. Thank you.

@kueda
Copy link

kueda commented Sep 11, 2017

Has this functionality been documented anywhere for use in Android, or better yet implemented in a demo? Would be great to have a working example.

@matteblair
Copy link
Member

matteblair commented Sep 12, 2017

@kueda There's an open issue to add this in our docs repo: tangrams/tangram-docs#192, thanks for the reminder to add this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.