Feature/refactor mercator layer

[EMaksymenko]

Simplify MercatorTiledImageLayer code. Make it exdended from BasicTiledImageLayer.

[wcmatthysen]

@Sufaev, could you remove the SurfaceText changes from this pull-request? That particular commit is causing merge conflicts with the develop branch.

You can do it by removing the "Add SurfaceText rotation" commit as follows (assuming you are on the feature/refactor-mercator-layer branch):

git rebase -p --onto HEAD~1^ HEAD~1

Once removed, you can force-push the branch again:

git push origin feature/refactor-mercator-layer --force

This will update the branch and remove the conflicting commit.

When making pull-requests it is best if you work from the head of the develop branch. Thus, do a:

git checkout develop

And then do a:

git checkout -b feature/my-new-feature

When you work on a new feature.

[EMaksymenko]

It was my fault if I create this branch from surface text. I will clean up it tomorrow.

[EMaksymenko]

Done.

[wcmatthysen]

@Sufaev, I made a couple of changes. Let me know what you think. I'll just explain some of them here:

• I changed the formatting of the classes to fit in with the rest of the code in WorldWind.
• I changed the constructor of the BasicMercatorTiledImageLayer to allow the dataset-name and data-cache-name to be passed as parameters. We should allow these parameters to be set as different layers (i.e. sub-classes) might want to set these values differently.
• I added a default value for the firstLevelOffset in the MercatorTileUrlBuilder class. This is so that every subclass does not have to specify this parameter explicitly (they can rely on the default of 3).
• Correct me if I'm wrong, but the NUM_LEVELS entry should not be affected by firstLevelOffset. These are two separate parameters. If the user wants to download 16 tile levels with a firstLevelOffset of 6 then NUM_LEVELS should be specified as 16. The user should end up with 16 levels, not 10. I changed the code in the makeLevels() method of BasicMercatorTiledImageLayer to not remove the firstLevelOffset value from NUM_LEVELS.

I then refactored the OSMMapnikLayer class with the recommendations that you gave in Issue #38 near the bottom where you created an OSMLayer class. I tested out the refactor and the tiles basically looks the same except for a color-difference. However, the OSMMapnikLayer in the newly refactored code-base have a much better color-gradient compared to the old OSMMapnikLayer. You can see it here:

The tile above is an example of the old OSMMapnikLayer class. The same tile with the new class looks like:

I don't know why this is. It could be the image post-processing logic in MercatorDownloadPostProcessor. But, in any case, the newly refactored OSMMapnikLayer looks better and behaves the same.

As a last task, I removed the old OSMCycleMapLayer class as this doesn't work anymore. The URL that it points to (http://b.andy.sandbox.cloudmade.com/tiles/cycle/) is not responding.

[EMaksymenko]

@wcmatthysen NUM_LEVELS must be affected by first-level-offset, because num levels is a general amount of zoom levels available in map sourse and first-level-offset is an offset between source zero zoom and zero level in wwd levelset. Amount of levels in levelset should equals general num levels minus first-level-offset, because first-level-offset is than added to current rendered level in url builder and if you will not subtract first-level-offset from num_levels than program will try to get more levels than map source can provide.
For example, if OSM has 17 levels and we want to start our levelset from zoom = 3, then we pass numlevels = 17, offset = 3. As the result program will create levelset with 17-3 = 14 levels. When we render level number 0 it will request tiles of zoom 3. When we render level number 14 it will request zoom 17. In your current case we can request level number 17 (which will exists) and it will be zoom number 20, which is wrong.

I simplified dataset-name and data-cache-name to be set by short map name just for simplicity when using build-in levelset builder. This approach defines root folder for all mercator maps cache and only allows to configure last sub-folder name. But if you wish to specify them manually, than it is ok for me.

Default firstLevelOffset in MercatorTileUrlBuilder is ok. It was equals 3 in my source code, but when I have created pull request I have removed constant value and replace it by parameter. Good solution to have both parametrized and default constructors. I like it.

OSM URL Builder should request tiles from random sub-domain a, b or c to speedup downloading.

char abc = "abc".charAt(random.nextInt(2));
return new URL("https://"+abc+".tile.openstreetmap.org/"+z+"/"+x+"/"+y+".png");

[wcmatthysen]

@Sufaev, ok, I understand the NUM_LEVELS issue a bit better now. I'll remove that commit and push the changes again.

Nice, the "abc" optimisation can work great. It should just be random.nextInt(3). If you look at the javadoc for this function you'll see the value is the upper exclusive value (i.e. it is not included in the distribution). Do you think this approach is the best? We could also sequentially access the servers, as in "a", "b", "c", "a" ... Like:

index = (index + 1) % 3;
char abc = "abc".charAt(index);
return new URL("https://"+abc+".tile.openstreetmap.org/"+z+"/"+x+"/"+y+".png");

where index is a member variable of URLBuilder. Is the random-access better? I suppose for a large number of requests it would work out the same as the random numbers are uniformly distributed.

[EMaksymenko]

@wcmatthysen I do not remember why we start using random.
May be it was easer than handling local variable.
I just remember that some map servers ban if you are using always the same server instead of combining all of them.

You can try to use sequential access.

[wcmatthysen]

@Sufaev, I looked into this a bit and saw from the OpenStreetMap Wiki a description regarding the different server-names:

Generally several subdomains (server names) are provided to get around browser limitations on the number of simultaneous HTTP connections to each host. Browser-based applications can thus request multiple tiles from multiple subdomains faster than from one subdomain. For example, OSM, OpenCycleMap servers have three subdomains (a.tile, b.tile, c.tile), all pointing to the same CDN.

So, basically the a, b or c prefixes are meant for browser optimisation. It doesn't really affect us. You can test it by running a ping on the different server-names and you'll see they all point to the same IP-address.

I think for our purposes we can probably just work with: https://a.tile.openstreetmap.org

[EMaksymenko]

Ok. Than we do not need this optimization.
I just do the same way as for other type of maps.
I thought it was requred to access from different servers.

[wcmatthysen]

@Sufaev, I'm just going to modify the branch again. I think we can add the OSMCycleMapLayer class again. However, to use OpenCycleMap you need to provide an API key, else you'll see a watermark on the tiles. I'm not sure where we can let the user configure this key. It would be ideal if we could allow for something like this in the worldwind.layers.xml file:

...
<Layer className="gov.nasa.worldwind.layers.Earth.OSMCycleMapLayer" actuate="onRequest">
    <Property name="APIKey" value="--api-key--"/>
</Layer>
...

I just have to see if I can get this to work.

[EMaksymenko]

Most map apis will reqire a key. It is a good idea in general.

[wcmatthysen]

@Sufaev, I added the OSMCycleMapLayer again and refactored this layer the same way I refactored the OSMMapnikLayer. As mentioned before, this layer can now be configured with the correct API-key in the worldwind.layers.xml file as:

...
<Layer className="gov.nasa.worldwind.layers.Earth.OSMCycleMapLayer" actuate="onRequest">
    <Property name="APIKey" value="--api-key--"/>
</Layer>
...

I have tested this feature and it works.
@emxsys, I think we can merge these changes into the development branch. It would be great if you can just cross-check that everything is in order.

[wcmatthysen]

I see the build is failing. However, it looks like a certificate issue with Oracle's site: https://stackoverflow.com/questions/55878548/travis-ci-couldnt-install-openjdk11

[wcmatthysen]

In the original issue #38, there are additional layers such as OTMLayer, WikiLayer and GoogleLayer. I suppose we can also add the OTMLayer and WikiLayer as new layers. I'm not sure about the licensing around GoogleLayer. Would it be best just to leave that out? Also, is it actually ok (in terms of licensing) to include OTMLayer and WikiLayer?

[wcmatthysen]

@emxsys, I added OpenTopoMapLayer and WikimapiaLayer implementations that are based on @Sufaev's implementations in issue #38. I think everything looks OK. I can merge if there aren't any objections.

[wcmatthysen]

I didn't want to add the Google-layer as I'm unsure about the license restrictions.

[robmilton]

I've been using a build of WWJ-CE for almost a couple of months. Today I pulled the develop branch and rebuilt my jars in order to test GDAL changes. But I noticed errors in my project (built on top of WWJ) related to the MercatorTiledImageLayer class.

I updated my code to work with the changes in the Mercator layer and rebuilt my project. However, now when I try to run I am getting the following error on every layer that is attempting to be loaded:

java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer; at gov.nasa.worldwind.util.WWIO.readChannelToBuffer(WWIO.java:924) at gov.nasa.worldwind.util.WWIO.readStreamToBuffer(WWIO.java:855) at gov.nasa.worldwind.util.WWIO.readURLContentToBuffer(WWIO.java:509) at gov.nasa.worldwind.util.WWIO.readURLContentToBuffer(WWIO.java:478) at gov.nasa.worldwind.terrain.BasicElevationModel.makeBilElevations(BasicElevationModel.java:565) at gov.nasa.worldwind.terrain.BasicElevationModel.readElevations(BasicElevationModel.java:550) at gov.nasa.worldwind.terrain.BasicElevationModel.loadElevations(BasicElevationModel.java:503) at gov.nasa.worldwind.terrain.BasicElevationModel$RequestTask.run(BasicElevationModel.java:434) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:748)

I ran into this before (several weeks ago) when I was testing a build of WWJ-CE using Java 9. So I took a look at the gradle build information to verify that it is set up to build against Java 1.8... which it is.

I can't figure out what's going on. The symptoms are exactly like the ones I experienced when I built WWJ-CE using Java 1.9.

Any ideas about what the problem might be?

[robmilton]

This issue was that the platform being used by the gradle build was JDK 11. I hadn't noticed the "Platform for build scripts" section in "Gradle project". Once I unchecked the "Inherit" checkbox I was able to choose JDK 8 as the build platform. And after I built using JDK 8 the errors went away.