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

Add support for multiple mbtiles #2111

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

rwrx
Copy link
Contributor

@rwrx rwrx commented Nov 29, 2019

Add support for multiple mbtiles files through urls_mbtiles. This is only my little hack in order to support mutltiple mbtiles not only one as a data source. If this is not appropriate and you also want to add this feature I can modify it as you will want.

@rwrx
Copy link
Contributor Author

rwrx commented Dec 18, 2019

Reply to comment #2119 (comment) example of using this parameter in YAML scene is (on Android platform):

source:
    mapzen:
        urls_mbtiles: ["/storage/emulated/0/offline_maps/belgium.mbtiles", "/storage/emulated/0/offline_maps/netherlands.mbtiles", "/storage/emulated/0/offline_maps/germany.mbtiles"]

in my app I allow users to download map data stored in mbtiles format. They can download mbtiles for every country in the world. Firstly I have tried after downloading mbtiles to merge them into one mbtiles file on Android device and it was very very slow - counts in minutes. Then I added support for using mutltiple mbtiles and when trying to show a tile, check which mbtiles file contains this tile and use it from that mbtiles file.

I just needed this functionality and thought that it could be useful also for other developers which want to use Tangram-ES for offline maps stored in mbtiles.

@matteblair matteblair self-assigned this Dec 18, 2019
@matteblair
Copy link
Member

Aha, thanks for sharing your use case! So this data source is a set of local archives that can be used offline. This does seem like a reasonable scenario for us to support. I'll take a look at the changes and see how we can best enable this.

LOGE("Unable to open SQLite database: %s - %s", path.c_str(), e.what());
if (!m_dbs.empty()) {
m_dbs[m_dbs.size() - 1].reset();
}
return;
Copy link
Member

@tallytalwar tallytalwar Dec 20, 2019

Choose a reason for hiding this comment

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

Shouldn't you be doing a continue here to try the other set of mbtile path?

SQLite::Column column = stmt.getColumn(0);
const char* blob = (const char*) column.getBlob();
const int length = column.getBytes();
if (length > largestLength) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment about this check please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If multiple mbtiles contain the same tile, it will get tile which has more data.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great. Can you put this in the code comment please, will be helpful for anyone looking at the code in future. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have added it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rwrx

Copy link
Member

@tallytalwar tallytalwar left a comment

Choose a reason for hiding this comment

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

This overall looks good to me, just a couple of cleanup suggestions if you could address. Thanks for contributing to tangram-es and sharing these usecases.

@rwrx
Copy link
Contributor Author

rwrx commented Dec 20, 2019

Ok, I have tried to address your requests and also done some additional cleanup and fixes.

@rwrx
Copy link
Contributor Author

rwrx commented Dec 20, 2019

I am concerned about name of actual YAML paramter name of urls_mbtiles if this is correct, or if it should be somehow done together withing url parameter.

@matteblair
Copy link
Member

@rwrx I've been thinking about that as well - specifically whether we should have a different way of configuring these kinds of "offline archive" data sources, since they need to behave somewhat differently than "online" tile sources. Might try out some ideas this weekend but I'll be traveling for holidays soon so apologies if I'm slow to respond!

@rwrx
Copy link
Contributor Author

rwrx commented Dec 20, 2019

@matteblair I was thinking about extending url parameter to accept array of urls, but I wasn't sure if it will be consistent semantically, so I rather created new parameter.

Base automatically changed from master to main February 15, 2021 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants