Add support for OpenCage geocoder #7015
Conversation
|
Thank you so much for the pull request @marrouchi! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
Thanks for the pull request @marrouchi! At a glace, everything looks great. I'll do a more thorough review after we receive your CLA. |
|
@marrouchi we received your CLA, thanks again. |
CHANGES.md
Outdated
| @@ -223,6 +223,7 @@ Change Log | |||
| * Added ability to invoke `sampleTerrain` from node.js to enable offline terrain sampling | |||
| * Added more ParticleSystem Sandcastle examples for rocket and comet tails and weather. [#6375](https://github.com/AnalyticalGraphicsInc/cesium/pull/6375) | |||
| * Added color and scale attributes to the `ParticleSystem` class constructor. When defined the variables override startColor and endColor and startScale and endScale. [#6429](https://github.com/AnalyticalGraphicsInc/cesium/pull/6429) | |||
| * Added `OpenCageGeocoderService`, which provides geocoding via [OpenCage](https://opencagedata.com/). | |||
There was a problem hiding this comment.
This needs to be moved up to the 1.50 section
| Check.defined('apiKey', apiKey); | ||
| //>>includeEnd('debug'); | ||
|
|
||
| this._url = Resource.createIfNeeded(url); |
There was a problem hiding this comment.
Instead of using this._url three times, do
url = Resource.createIfNeeded(url);
...
this._url = url;
| * @example | ||
| * // Configure a Viewer to use the OpenCage server hosted by https://geocode.earth/ | ||
| * var viewer = new Cesium.Viewer('cesiumContainer', { | ||
| * geocoder: new Cesium.OpenCageGeocoderService(new Cesium.Resource({ |
There was a problem hiding this comment.
This example code doesn't work. It doesn't pass in the API key param
| * @param {String} [options.proximity] Provides the geocoder with a hint to bias results in favour of those closer to the specified location (For example: 41.40139,2.12870). | ||
| * @returns {Promise<GeocoderService~Result[]>} | ||
| */ | ||
| OpenCageGeocoderService.prototype.geocode = function(query, params) { |
There was a problem hiding this comment.
This doesn't match the interface for GeocoderService.geocode https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/GeocoderService.js#L28-L32
The GeocoderViewModel for the geocoder widget will only pass in the search string and the second param is either GeocodeType.SEARCH or GeocodeType.AUTOCOMPLETE. Should all of these optional params be passed into the constructor instead?
There was a problem hiding this comment.
Yes, i should probably pass these optional params into the constructor. OpenCage does not offer auto-complete geocoding so there is no need to use the second param.
There was a problem hiding this comment.
Yep, that's no problem. Neither does the BingMapsGeocoderService: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/BingMapsGeocoderService.js#L75
That geocode function just accepts the first query param
|
Just those comments. Thanks @marrouchi! |
|
Thank you @hpinkos for the code review. Please let me know if there's others issues. |
|
Works great! Thanks for the contribution @marrouchi! |
OpenCage is an API for forward/reverse geocoding. This adds OpenCageGeocoderService for use with the Geocoder widget.
To test this, you'll need to open an account in order to get the API Key.
PS : I'll send the Signed CLA tomorow.