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

DataSource refactoring #1014

Merged
merged 9 commits into from
Jan 12, 2017
Merged

DataSource refactoring #1014

merged 9 commits into from
Jan 12, 2017

Conversation

hjanetzek
Copy link
Member

@hjanetzek hjanetzek commented Oct 13, 2016

This PR adds the RawDataSource interface to enable chaining of tile data fetching.

struct RawDataSource {
    virtual bool loadTileData(std::shared_ptr<TileTask> _task, TileTaskCb _cb);
...
    void setNext(std::unique_ptr<RawDataSource> _next) {
        next = std::move(_next);
        next->level = level + 1;
    }
    std::unique_ptr<RawDataSource> next;
    int level = 0;
}

A DataSource holds a chain of RawDataSources. On a tile request loadTileData is called on the first RawDataSource, if it can't serve the tile data the request is passed on to the next source. The task is passed back through the chain via the TileTaskCb. This allows earlier sources to intercept the result e.g. for caching by passing their own TileTaskCb.

The in-memory cache and tile download steps are moved out of DataSource and now implemented as RawDataSources.

std::unique_ptr<RawDataSource> next;
};

class NetworkDataSource : public RawDataSource {
Copy link
Member

Choose a reason for hiding this comment

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

We might need another class for non tiled network data sources. As an MBTile source could be downloaded, or in future our own mapzen "zipped" source could be downloaded.

Copy link
Member Author

@hjanetzek hjanetzek Oct 13, 2016

Choose a reason for hiding this comment

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

Downloading bundles should be handled separately in my opinion, similar to what we do in ClientGeoJson source, or become part of scene loading.

Copy link
Member Author

@hjanetzek hjanetzek Oct 13, 2016

Choose a reason for hiding this comment

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

Downloads of huge MBTiles packs needs some more thought. Bundles should be stored on disk to not download a few hundred MB each time. Also better ask for user feedback before downloading when they are on mobile data ;)

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 that run-time download of new tile bundles is something that should be handled by a Mapzen platform SDK (Android or iOS) rather than in Tangram core.

Copy link
Member

Choose a reason for hiding this comment

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

I agree download of the bundles should be handled by the platform SDK.

I was mainly asking about about where does it fit in this new rawdata chaining architecture.

@hjanetzek hjanetzek force-pushed the raw-datasources branch 3 times, most recently from ce1d7d3 to 22732dd Compare October 14, 2016 10:04
@hjanetzek hjanetzek mentioned this pull request Oct 17, 2016
@matteblair matteblair self-assigned this Oct 17, 2016
@hjanetzek hjanetzek force-pushed the raw-datasources branch 3 times, most recently from 7e5e6d6 to 0150cc8 Compare October 21, 2016 13:29
@hjanetzek hjanetzek force-pushed the raw-datasources branch 7 times, most recently from efe58d5 to f0925aa Compare November 1, 2016 12:41
Copy link
Member

@matteblair matteblair left a comment

Choose a reason for hiding this comment

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

Looks great! I'm good with merging this.

@tallytalwar tallytalwar merged commit 6b23cc0 into master Jan 12, 2017
@tallytalwar tallytalwar deleted the raw-datasources branch January 12, 2017 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants