Dvt improvements 2020

[Jeffrey-P-McAteer]

Description of the Change

This adds a global cache for all WMS layers if the calling code sets the system property "worldwind.ImageRetriever.decodeUrlCacheDir". The cache stores WMS Capabilities as a .xml file as well as a .PNG tile for every URL processed through gov.nasa.worldwind.render.ImageRetriever.

Why Should This Be In Core?

I'm not 100% certain this should be in core, but there have been previous discussions on the worldwind forum which indicate this capability is desired:

https://forum.worldwindcentral.com/forum/worldwind-android-wwa/android-discussion/17902-android-offline-mode

This PR should open a discussion about what the most idiomatic way to add file caches to LayerFactory and ImageRetriever such that

1. Existing code does not need to be modified
2. Callers get some way to clear the cache (at the moment these changes do not handle invalidating the cache)

Benefits

Offline imagery is useful for a number of organizations (This change originated internally at https://devil-tech.com/, we are certainly interested in using offline data for an application we write).

Potential Drawbacks

This is a global cache which uses a system property to modify behavior. This is likely not idiomatic code for the WW community and I'd like to hear feedback on a more appropriate approach.

Applicable Issues

NASAWorldWind#3

NASAWorldWind#72

[EMaksymenko]

I have one question. Why do not you use MBTiles SQLite format for tile cache?

Storing tiles as separate PNG files will waste inodes in partition table on sdcard.

Are you able to replace separate file per tile approach with one MBTiles.db file per map source?

[Jeffrey-P-McAteer]

@Sufaev The first blocker I see is that we would need to add an MBTiles library as a dependency to WW Android just for processing. It looks like there is one available (https://github.com/imintel/mbtiles4j and an android-specific fork https://github.com/fullhdpixel/mbtiles4j). That particular library is licensed under Apache 2.0 and I do not know if the license WW uses would allow us to use that as a dependency.

After that there is an issue of metadata format. My initial research on MBTiles shows that it represents tiles as a tree, so the first thing we'd have to do is get the lat/lon/zoom from the WMS URL and normalize it to a set of zoom/column/row which would get stored in the MBTiles "tiles" table. (Everything I've learned about MBtiles comes from https://github.com/mapbox/mbtiles-spec/blob/master/1.2/spec.md).

I like the concern about inodes; filesystems can bite you in unforseen ways and I'm a huge fan of sqlite. I think this is a good long-term plan, and after looking through the WW codebase it looks like we can use sqlite without a dependency (android.database.sqlite.SQLiteDatabase is used to process gov.nasa.worldwind.ogc.gpkg.GeoPackage). The thing to do in my mind is first write a class similar to GeoPackage which handles reading and writing tiles to a MBTiles file, and implement the normalization from lat/lon/zoom to zoom/column/row in that class. This will make it straightforward to use in ImageRetriever as a cache backend.

@Sufaev do you mind opening an issue about this? It sounds worth pursuing (though I personally cannot promise a huge time investment, work has me busy until March). We can use that to list tasks and track progress a little better than using a closed PR.

[EMaksymenko]

MBTiles can be used without any library. It is only a table with x,y,z and tile columns.

I agree that it is designed for WebMercator tiles and x,y,z addressing paradigm.

I do not know if new task is required now. My product which use WorldWindAndroid will implement MBTiles soon, but we plan to use only WebMercator tile sources. So, may be this approach is not suitable for any WMS layer, but PNG files will definitely not work. It will fill up sdcard inodes very quickly.

[EMaksymenko]

Another critical issue of this PR. Any map source does not work by default when system property is not specified.

1. Method getGlobalCacheUrlFile in ImageRetriever returns null when system property is not defined.

public static File getGlobalCacheUrlFile(String urlString, String fileNameFormatStr) {
        String cacheDir = System.getProperty("worldwind.ImageRetriever.decodeUrlCacheDir");
        if (cacheDir == null) {
            return null;
        } else {

2. As the result new FileOutputStream(cacheFileFD) in addToGlobalCache method of ImageRetriever fall with NullPointerException.

    protected void addToGlobalCache(String urlString, Bitmap bitmap) {
        File cacheFileFD = getGlobalCacheUrlFile(urlString);

        try {
            FileOutputStream out = new FileOutputStream(cacheFileFD);

Please, add fix which correctly skip cache processing when worldwind.ImageRetriever.decodeUrlCacheDir is not specified.

P.S.: Generally System.getProperty is antipattern in Android.

[EMaksymenko]

Another proposition. Cache should be configurable to use JPG or PNG per map layer, because in case of maps like Google Satellite PNG will consume too much space comparing to JPG.

[EMaksymenko]

@Jeffrey-P-McAteer and @PJHogan I propose to follow common process with new features:

1. Develop each new feature in separate branch of own repository fork. Do not mix several features in one branch.
2. Create PR when feature is ready on opinion of developer.
3. Review and test new feature before merge by some other team member.
4. Merge PR only when feature is tested and reviewed.

Creating PR for each hot-fix and maintaining several features using one branch makes little chaos in commit history.
Do you agree?

[Jeffrey-P-McAteer]

@Sufaev completely, I was surprised this was merged so quickly. I'm not familiar with the development process WorldWindEarth follows, and I like the quality controls you proposed. I will make improvements to my fork (which include work-related needs that move quickly and break things) and the community can pull anything that looks good enough to be in core.

At the moment I'm thinking of adding method overloads to LayerFactory to take a cache configuration object. I know WW desktop uses the AVList pattern for passing configuration details around, does WW android have anything similar?

(Actually these thoughts would probably go best in an issue which tracks work done on network layers and their caches. I have a specific requirement but the stuff in core should be broad enough to apply to non-WMS network-backed layers.)

[EMaksymenko]

@Jeffrey-P-McAteer To make the correct process you need to create separate branch for each new feature in your repository fork. So it will be possible to create and append separate PRs for each feature till it will be completely tested.
You can merge them into your fork develop anytime to have one branch with all your features, but we need to test and merge all features separately here.
I do not know why @PJHogan merge your commits so fast.

Now I am thinking how to reorganize already committed code. We already have two unfinished features in one branch and cannot maintain them separately.

1. Placemark Label - looks fine except my comment about label and display name dependency.
2. Tile cache - had a NPE but you already fix it. I do not know if it is the last bug found in tile cache.

We need to revert somehow and make normal process of this two features implementation as both of them are still not completely tested.

@PJHogan and @Jeffrey-P-McAteer what is your propositions what to do with current situation?

[PJHogan]

I'm just trying to help by merging code when requested. Are you both able to merge? If so, I won't be merging anymore. If not, we'll need to get you merge capabilities.

[EMaksymenko]

@PJHogan I have rights to merge PRs and I can do review in Android repository if necessary.
Will it be Ok?

[PJHogan]

Eugene, You are the man! My apologies for just hitting 'Merge' For some reason I thought I was some kind of Admin, but as long as you guys also have the power, I will certainly not be doing that anymore! :~}}} On Tuesday, January 14, 2020, 12:02:25 PM PST, Eugene Maksymenko <notifications@github.com> wrote: @PJHogan I have rights to merge PRs and I can do review in Android repository if necessary. Will it be Ok? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[EMaksymenko]

@Jeffrey-P-McAteer I have reverted develop head.
Could you, please, cherry-pick all commits related to Offline map cache to some new branch e.g. "feature/offline-map-cache" in you repository fork starting with some commit already existed in our develop and create new PR with description copied from this PR, so I can review and merge only this feature?

[PJHogan]

Eugene,
WorldWind Android is now YOUR baby!
And we are as fortunate as delighted for that! :~}}}}}}
Help her to sing ever so clear and beautifully on key!!!