Added HereMaps as an additional ImageryProvider #6241

themikelester · 2018-02-21T06:17:02Z

This is based on Kevin Ring's initial implementation mentioned in this discussion: https://groups.google.com/forum/#!topic/cesium-dev/-JZK_zVhYNA.

I've cleaned it up, added some user-exposed options and added full support for credits. The HereMaps attribution system is rectangle-based and very similar to BingMaps, so I tried to copy that style as much as possible.

Doarama will be using this for an upcoming event with a client, so I thought it might be useful to submit back upstream. I'll also write some tests similar to BingMaps and update this PR.

cesium-concierge · 2018-02-21T06:17:03Z

Welcome to the Cesium community @themikelester!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

…lf as a contributor

themikelester · 2018-02-21T06:50:12Z

Fixed eslint errors, updated CHANGES and CONTRIBUTORS

themikelester · 2018-02-21T06:56:30Z

Submitted Contributor License Agreement

pjcozzi · 2018-02-21T12:27:46Z

Thanks @themikelester, we received your CLA.

We'll find someone to review this. In the meantime, two quick things:

Could you please add unit tests? Here is the Testing Guide and you could check out any of the imagery provider tests in the Specs/Scene directory.
Please don't leave any TODO comments in the code. Address them or submit an issue to this repo so we can consider it for the future.

Also, if you let us know what else you plan to contribute (post processing, other effects, etc.), we are happy to help point you in the right direction.

BTW, are you going to GDC this year?

themikelester · 2018-02-21T22:36:36Z

I'll write some unit tests as soon as I can. Regarding that TODO, I don't fully understand the use of TileDiscardPolicy yet. It sounds like Bing occasionally sends invalid tiles when they temporarily don't have imagery for a specific location. Is that the case? If so I haven't used Here enough to know if they do the same thing.

Unfortunately I won't be able to get to GDC this year, I'm currently in Sydney at Doarama HQ for a few months. I'll start a forum thread for other contributions, I've got some ideas.

pjcozzi · 2018-02-21T22:55:17Z

All sounds good.

@tfili can advise on TileDiscardPolicy.

…by adding pickFeatures()

…credit

themikelester · 2018-02-22T05:29:02Z

I added tests based on those written for the BingMapsImageryProvider, and switched all the url strings to use the new Resource interface.

shunter · 2018-02-22T14:29:50Z

The TileDiscardPolicy is to detect and reject the placeholder images that Bing returns when they don't have imagery at the requested zoom level.

They return this image for any request, even with an absurd level of detail:

https://ecn.t1.tiles.virtualearth.net/tiles/a030130012123122.jpeg?g=6299
https://ecn.t1.tiles.virtualearth.net/tiles/a030130012123122000000000.jpeg?g=6299

So we need to detect and ignore this image. Other imagery providers may not have this need.

tfili · 2018-02-23T03:05:11Z

Source/Scene/HereMapsImageryProvider.js

+     * @constructor
+     *
+     * @param {Object} options Object with the following properties:
+     * @param {String} [options.baseUrl="aerial.maps.api.here.com"] The URL for accessing tiles, without the Load Balancing prefix.


This parameter is usually name url on the other providers. It should also be a {Resource|String}. You can use Resource.createIfNeeded method to create or clone the resource.

I did notice that's what the new ImageryProviders were doing. The problem is that HereMaps uses a subdomain [1,2,3, or 4] before all their requests. E.g. all urls start with http://2.aerial.... I couldn't see a good way of doing that with Resource, which seemed like it only wanted to handle Subresources (i.e. appending to the end of the url). That's also why I named it baseUrl, because there's no way of specifying a full url that would be useful.

Should I just have options.url expect something like "http://aerial.maps.api.here.com" and then manually stick the subdomain in the middle there?

tfili · 2018-02-23T03:08:51Z

Source/Scene/HereMapsImageryProvider.js

+     * @param {String} [options.baseUrl="aerial.maps.api.here.com"] The URL for accessing tiles, without the Load Balancing prefix.
+     *        You may use the *.maps.cit.api.here.com URLs during development. The first part of the URL determines which tileTypes
+     *.       are available.
+     * @param {String} [options.tileType="maptile"] Determines which type of image will be delivered. E.g. Map, Labels, Streets, etc.


Is there a list of all valid values we can provide?

tfili · 2018-02-23T03:10:07Z

Source/Scene/HereMapsImageryProvider.js

+     *        You may use the *.maps.cit.api.here.com URLs during development. The first part of the URL determines which tileTypes
+     *.       are available.
+     * @param {String} [options.tileType="maptile"] Determines which type of image will be delivered. E.g. Map, Labels, Streets, etc.
+     * @param {String} [options.scheme="satellite.day"] Specifies the view scheme, e.g normal.day, normal.night, terrain.day, etc.


Do we know all the valid values here too? (Sorry to ask a bunch of questions. I don't know anything about here maps).

tfili · 2018-02-23T03:12:16Z

Source/Scene/HereMapsImageryProvider.js

+        this._ready = false;
+        this._readyPromise = when.defer();
+
+        //@TODO: Do we need to support DiscardMissingTileImagePolicy?


Obviously figure out if this is needed. Add a DiscardMissingTileImagePolicy or remove the to do. Is there a way Here tells us that there isn't a tile here?

I'm looking into this. I can't find any mention of what they do in the case of missing tiles in their documentation, but I'll start searching Antarctica and see if I can find one 😄

After zooming into some remote parts of the world, it appears that they always return a valid tile for any zoom level. If they don't have true imagery for it they use a rescaled version of one of the lower zoom levels. I assume this means we should just use the default DiscardMissingTileImagePolicy?

tfili · 2018-02-23T03:14:27Z

Source/Scene/HereMapsImageryProvider.js

+         */
+        proxy : {
+            get : function() {
+                return this._proxy;


This isn't set anywhere. It is actually a property of the resource. You can remove this property. Other providers have it for backwards compatibility reasons from pre-Resource days and will probably be deprecated.

This gets set on line 86. But I assume this is no longer part of the ImageryProvider interface? I'll remove it.

Oops. Forgot about the interface. That's why we haven't deprecated it yet. Keep it in there, but pass it into the resource as an option to createIfNeeded.

OK I'll revert that change. It is passed in to the Resource on line 105. That resource is used to create all other requests. There is also a test case that covers usage of the proxy, and it's passing.

I see that. If you look at Bing and the other providers, they no longer have a _proxy member. They do a return this._resource.proxy;

tfili · 2018-02-23T03:16:30Z

Source/Scene/HereMapsImageryProvider.js

+    }
+
+    return HereMapsImageryProvider;
+});


Add newline at end of file.

tfili · 2018-02-23T03:27:29Z

Source/Scene/HereMapsImageryProvider.js

+    var HereMapsImageryProvider = function HereMapsImageryProvider(options) {
+        options = defaultValue(options, {});
+
+        //>>includeStart('debug', pragmas.debug);


appId and appCode aren't in the doc section as options.

tfili · 2018-02-23T03:29:57Z

Source/Scene/HereMapsImageryProvider.js

+        var mapId = 'newest';
+
+        var baseResource = new Resource({
+            url: '//{subdomain}.' + this._baseUrl,


This is an odd setup. We can change the URL, but all are required to have 4 subdomains?

That's right. HereMaps forces us to always use one of the four subdomains. Which is why I chose not to accept a Resource or full url in the options object. We basically have to always construct the URL.

What are the possible URLs? Are they all like aerial.maps.api.here.com? Do they always end in maps.api.here.com? I'm not sure of the cleanest way to handle this.

There's documentation in baseUrl description in the constructor docs. Basically the url is *.maps.api.here.com, where * is one of <aerial|base|pano|traffic>. They've got good API documentation here: https://developer.here.com/documentation/map-tile/topics/request-constructing.html.

The url construction is more complicated than Bing, and the *.maps.api subdomain that you pick determines a lot of the other settings that are viable. Because of this I chose to put more impetus on the user to read the docs and determine what options are usable, rather than try to enumerate all the options and their dependencies like BingMapsStyle. The documentation has links to all the HereMaps API documentation that a user might need. I hope this is an acceptable level of complexity for a Cesium user who wants to use Here. The default settings should cover the common case.

Ok. So essentially the subdomain and type are part of the REST API even though they are in the domain, which is a little odd. What do you think of this? Take a options.url that is a string or resource (http://maps.api.here.com) and a options.type. Then from there you build the URL.

I think this is the best option, since that is how the URL is formatted. Also, for parsing the url we have a ThirdParty/Uri that might help.

That scheme sounds good to me. ThirdParty/Uri looks nice, thanks for pointing me to it.

This reverts commit 6730c6e.

It appears that HereMaps will always return a valid tile, it just may be a scaled up version of a lower zoom level.

…aseUrl options.url is now accepts a Resource or String

pjcozzi · 2018-06-18T11:58:07Z

@themikelester I see that you made a bunch of commits to this after the last round of feedback. Is this ready for another review or do you still have more to do here?

pjcozzi · 2018-10-06T15:25:14Z

@themikelester is this ready for another review? If not, do you still plan to finish this or should we close it?

cesium-concierge · 2018-12-11T22:00:28Z

Thanks again for your contribution @themikelester!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

hpinkos · 2018-12-12T20:17:07Z

@themikelester I'm going to close the PR since we haven't heard from you. Please re-open this if you are still interested in getting this merged! Thanks for contributing =)

cesium-concierge · 2018-12-12T20:17:12Z

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/-JZK_zVhYNA

If this issue affects any of these threads, please post a comment like the following:

The issue at #6241 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

Added HereMaps as an additional ImageryProvider

7fe9dc0

themikelester added 3 commits February 21, 2018 17:41

Fix eslint errors to conform to Cesium style guidelines

967bbd2

Updated CHANGES.md

57fe47c

Updated CONTRIBUTORS.md with Ayvri as a contributing company and myse…

9e64848

…lf as a contributor

pjcozzi assigned tfili Feb 21, 2018

themikelester added 3 commits February 22, 2018 12:01

Added very basic test suite. Fixed ImageryProvider interface failure …

04ee6f2

…by adding pickFeatures()

Switched to the new Resource interface rather than url strings

88e3866

Added similar tests as BingImageryProvider, and added a default Here …

01ad7d0

…credit

tfili requested changes Feb 23, 2018

View reviewed changes

tfili reviewed Feb 23, 2018

View reviewed changes

themikelester added 2 commits February 23, 2018 14:28

Remove unused proxy member of HereMapsImageryProvider

6730c6e

Added new line at end of file to conform to Cesium style

4544ccf

tfili reviewed Feb 23, 2018

View reviewed changes

themikelester added 7 commits February 23, 2018 14:30

Revert "Remove unused proxy member of HereMapsImageryProvider"

03fad66

This reverts commit 6730c6e.

Added documentation for appCode and appId

0940ef3

Remove _proxy member, use _tileResource.proxy in the getter

919e8a7

Remove DiscardMissingTilePolicy TODO.

f90aaf8

It appears that HereMaps will always return a valid tile, it just may be a scaled up version of a lower zoom level.

Accept options.url and options.mapType (with defaults), rather than b…

ecca50c

…aseUrl options.url is now accepts a Resource or String

Removed some trailing spaces that the linter caught

2adaa97

Fix some documentation problems

2c48ae9

hpinkos closed this Dec 12, 2018

AnalyticalGraphics unassigned tfili Feb 18, 2020

Added HereMaps as an additional ImageryProvider #6241

Added HereMaps as an additional ImageryProvider #6241

Conversation

themikelester commented Feb 21, 2018

cesium-concierge commented Feb 21, 2018

themikelester commented Feb 21, 2018

themikelester commented Feb 21, 2018

pjcozzi commented Feb 21, 2018

themikelester commented Feb 21, 2018

pjcozzi commented Feb 21, 2018

themikelester commented Feb 22, 2018 • edited

shunter commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jun 18, 2018

pjcozzi commented Oct 6, 2018

cesium-concierge commented Dec 11, 2018

hpinkos commented Dec 12, 2018

cesium-concierge commented Dec 12, 2018

themikelester commented Feb 22, 2018 •

edited