Add option to select which geocoder is used #12299

angrycat9000 · 2024-11-11T20:45:13Z

Description

Adds the ability to change the geocoder used.

Both Bing and Google have legal requirements that their geocoders should not be used with other data. Eg. Using Bing geocode results while showing Google 3D Tiles is not allowed.

Adds GoogleGeocoderService for standalone usage of Google geocoder
Adds IonGeocodeProvider enum to list the possible geocode providers when using Cesium ion.
Updates Viewer constructor to also take an IonGeocoderProvider value
Updates Sandcastles to specify Google or Bing geocoders where appropriate, or remove the geocoder if other map data is used.

Testing plan

Run npm run start
Open http://localhost:8080/Apps/Sandcastle/index.html
Type in Philadelphia in the geocoder
- Verify that multiple results are returned for Philadelphia
Open http://localhost:8080/Apps/Sandcastle/index.html?src=Google%20Photorealistic%203D%20Tiles%20with%20Building%20Insert.html
Type in Philadelphia in the geocoder
- Verify that only a single result is returned
- Verify that there is only a single google attribution at the bottom of the screen
Open http://localhost:8080/Apps/Sandcastle/index.html?src=Bing%20Maps%20Labels%20Only.html
Type in Philadelphia in the geocoder
- Verify that multiple results are returned for Philadelphia
Run the following code snippets in the local sandcastle and verify the results. Replace 'your-key-here` with a valid Google API key with Geocoding enabled

const viewer = new Cesium.Viewer("cesiumContainer", {
  geocoder: new Cesium.GoogleGeocoderService({
    key: 'your-key-here'
  })
});
const googleTileset = await Cesium.createGooglePhotorealistic3DTileset({
  onlyUsingWithGoogleGeocoder: true
});
viewer.scene.primitives.add(googleTileset);

Type Philadelphia in the geocoder
Verify that a single result appears and takes you to the correct location
Verify that a single Google logo is shown on screen in the data attributions

const viewer = new Cesium.Viewer("cesiumContainer");
const googleTileset = await Cesium.createGooglePhotorealistic3DTileset();
viewer.scene.primitives.add(googleTileset);

Verify there is a warning in the console about using the Google geocoder

const viewer = new Cesium.Viewer("cesiumContainer");
const googleTileset = await Cesium.createGooglePhotorealistic3DTileset('your-key-here');
viewer.scene.primitives.add(googleTileset);

Verify that the tileset loads
Verify there is a deprecation warning

const viewer = new Cesium.Viewer("cesiumContainer");
const googleTileset = await Cesium.createGooglePhotorealistic3DTileset({
  key:'your-api-key-here', 
  onlyUsingWithGoogleGeocoder: true
});
viewer.scene.primitives.add(googleTileset);

Verify that the tileset loads
Verify that there is no deprecation warning.

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code
Update ion to use consistent Google attribution
Figure out test failure - CesiumWidgetSpec leaves requests pending after tests finish #12306 Fixes unhandled rejection in Skybox.update #12306 #12307
Figure out one time warning

github-actions · 2024-11-11T20:45:33Z

Thank you for the pull request, @angrycat9000!

✅ We can confirm we have a CLA on file for you.

ggetz · 2024-11-13T14:21:54Z

Thanks @angrycat9000! We're hoping to take a review pass today.

angrycat9000 · 2024-11-13T16:53:12Z

Thanks @ggetz . Two things that were still pending:

Duplicate Google attributions. The fix requires a patch for ion which will probably be deployed next week.
Random test failure which I had started to look into. I don't think is is related to any of my changes at this point.

ggetz

This is looking really good @angrycat9000! All of my comments are just code and documentation cleanup.

CHANGES.md

ggetz · 2024-11-13T21:13:08Z

packages/engine/Source/Core/GoogleGeocoderService.js

+/**
+ * Provides geocoding through Google.
+ *
+ * @see {@link https://developers.google.com/maps/documentation/geocoding/policies|Google Geocoding Policies}


Great that you included the link! Can we call out explicitly the "TLDR"– that Google Geocoding should only be used when using Google data in the scene? And is there a similar disclaimer we should put in BingMapsGeocoderService?

packages/engine/Source/Scene/createGooglePhotorealistic3DTileset.js

packages/engine/Source/Core/GoogleGeocoderService.js

packages/engine/Source/Core/IonGeocoderService.js

ggetz · 2024-11-13T21:35:16Z

packages/engine/Source/Core/IonGeocodeProvider.js

+ *
+ * @enum {string}
+ */
+const IonGeocodeProvider = {


Suggested change

const IonGeocodeProvider = {

const IonGeocodeProviderType = {

This may be a bit nit-picky, but type may help distinguish this from from the *GeocoderService instance. I got a bit confused when looking through the specs which was which.

ggetz · 2024-11-13T21:35:46Z

packages/engine/Source/Core/IonGeocoderService.js

+   * @memberof IonGeocoderService.prototype
+   * @type {IonGeocodeProvider}
+   */
+  geocodeProvider: {


Suggested change

geocodeProvider: {

geocodeProviderType: {

This may be a bit nit-picky, but type may help distinguish this from from the *GeocoderService instance. I got a bit confused when looking through the specs which was which.

ggetz · 2024-11-13T21:41:51Z

packages/widgets/Source/Viewer/Viewer.js

@@ -280,7 +281,7 @@ function enableVRUI(viewer, enabled) {
 * @property {boolean} [baseLayerPicker=true] If set to false, the BaseLayerPicker widget will not be created.
 * @property {boolean} [fullscreenButton=true] If set to false, the FullscreenButton widget will not be created.
 * @property {boolean} [vrButton=false] If set to true, the VRButton widget will be created.
- * @property {boolean|GeocoderService[]} [geocoder=true] If set to false, the Geocoder widget will not be created.
+ * @property {boolean|IonGeocodeProvider|GeocoderService[]} [geocoder=IonGeocodeProvider.DEFAULT] If set to false, the Geocoder widget will not be created.


Suggested change

* @property {boolean|IonGeocodeProvider|GeocoderService[]} [geocoder=IonGeocodeProvider.DEFAULT] If set to false, the Geocoder widget will not be created.

* @property {boolean|IonGeocodeProvider|GeocoderService|GeocoderService[]} [geocoder=IonGeocodeProvider.DEFAULT] The geocoding service or services to use when searching with the Geocoder widget. If set to false, the Geocoder widget will not be created.

Wow, this option was not well documented prior to this PR! Let's clean it up if it's not too much trouble.

Co-authored-by: Gabby Getz <[email protected]>

ggetz · 2024-11-21T16:18:44Z

Hi @angrycat9000, I wanted to check in on the status of this PR as we are coming up on our next release soon.

Are there any remaining TODO's for this PR?
Is it at a point where it can come out Draft?

angrycat9000 · 2024-11-21T16:44:52Z

Hi @angrycat9000, I wanted to check in on the status of this PR as we are coming up on our next release soon.

Are there any remaining TODO's for this PR?

Is it at a point where it can come out Draft?

Just the discussion of how assertive to make the one time warning discussion. Added a to-do for that. Will be ready once that is decided.

angrycat9000 · 2024-11-21T22:22:38Z

This is updated and ready for review again @ggetz

ggetz

This looks great @angrycat9000! Thanks!

Just two small comments and this should be good to merge.

The last step would be to open an issue to track removing the deprecated API in 1.126.

ggetz · 2024-11-22T14:34:50Z

Apps/Sandcastle/gallery/development/3D Tiles Picking.html

@@ -44,7 +44,10 @@
            onselect: async () => {
              scene.primitives.remove(tileset);
              try {
-                tileset = await Cesium.createGooglePhotorealistic3DTileset();
+                tileset = await Cesium.createGooglePhotorealistic3DTileset({


Should the geocoder: false viewer option be set for this example?

I would say yes since it isn't valid for the non-georeferenced models.

The use with the Maxar data is more of a grey area so it is probably good to remove it on that account too.

ggetz · 2024-11-22T14:39:08Z

packages/engine/Source/Core/IonGeocoderService.js

+    options.geocodeProviderType,
+    IonGeocodeProviderType.DEFAULT,
+  );
+  validateIonGeocodeProviderType(geocodeProviderType);


Since this function throws a developer error, we should surround it with the debug pragmas so it is stripped out in production builds.

Suggested change

validateIonGeocodeProviderType(geocodeProviderType);

//>>includeStart('debug', pragmas.debug);

validateIonGeocodeProviderType(geocodeProviderType);

//>>includeEnd('debug');

angrycat9000 · 2024-11-22T15:11:40Z

Thanks for the feedback @ggetz . Updated and ready for another look

ggetz · 2024-11-22T15:27:07Z

Fantastic! Thanks for contributing this @angrycat9000!

Add option to select which geocoder is used

9557a3e

angrycat9000 added 6 commits November 11, 2024 16:50

Fix bug with gecoder:undefined getting passed in the query

3bd5d72

Update documentation

5f849b1

Fix documentation for IonGeocodeProvider

096e67b

IonGeocoderProvider -> IonGeocodeProvider

0b2e65e

Add GoogleGeocoderService

79cb58b

Update for Google Geocoder

fa0da08

ggetz requested changes Nov 13, 2024

View reviewed changes

angrycat9000 and others added 9 commits November 14, 2024 10:15

Update CHANGES.md

f04ee84

Co-authored-by: Gabby Getz <[email protected]>

Minor PR feedback

4e91c99

Rename IonGeocodeProvder -> IonGeocodeProviderType

a781121

Merge branch 'main' into choose-your-own-geocoder

0c382fd

Merge branch 'main' into choose-your-own-geocoder

31e10f7

Remove geocoder from sandcastles with non bing/google map data

b511f59

Add links to terms of service

5014ecb

More renames of geocodeProvider -> geocodeProviderType

c05d122

Merge branch 'main' into choose-your-own-geocoder

6a43470

ggetz added the priority - next release label Nov 21, 2024

angrycat9000 added 2 commits November 21, 2024 16:59

Add one time warning and deprecation

8a20ef0

Merge branch 'main' into choose-your-own-geocoder

d1fb2e0

angrycat9000 marked this pull request as ready for review November 21, 2024 22:22

ggetz requested changes Nov 22, 2024

View reviewed changes

Minor PR feedback

154c705

ggetz approved these changes Nov 22, 2024

View reviewed changes

ggetz added this pull request to the merge queue Nov 22, 2024

Merged via the queue into main with commit 124fd43 Nov 22, 2024
9 checks passed

ggetz deleted the choose-your-own-geocoder branch November 22, 2024 15:27

ggetz mentioned this pull request Nov 22, 2024

Remove deprecated createGooglePhotorealistic3DTileset key option in 1.126 #12328

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to select which geocoder is used #12299

Add option to select which geocoder is used #12299

angrycat9000 commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024

ggetz commented Nov 13, 2024 •

edited

Loading

angrycat9000 commented Nov 13, 2024

ggetz left a comment

ggetz Nov 13, 2024

ggetz Nov 13, 2024

ggetz Nov 13, 2024

ggetz Nov 13, 2024

ggetz commented Nov 21, 2024

angrycat9000 commented Nov 21, 2024

angrycat9000 commented Nov 21, 2024

ggetz left a comment

ggetz Nov 22, 2024

angrycat9000 Nov 22, 2024

ggetz Nov 22, 2024

angrycat9000 commented Nov 22, 2024

ggetz commented Nov 22, 2024

	const IonGeocodeProvider = {
	const IonGeocodeProviderType = {

	* @property {boolean\|IonGeocodeProvider\|GeocoderService[]} [geocoder=IonGeocodeProvider.DEFAULT] If set to false, the Geocoder widget will not be created.
	* @property {boolean\|IonGeocodeProvider\|GeocoderService\|GeocoderService[]} [geocoder=IonGeocodeProvider.DEFAULT] The geocoding service or services to use when searching with the Geocoder widget. If set to false, the Geocoder widget will not be created.

Add option to select which geocoder is used #12299

Add option to select which geocoder is used #12299

Conversation

angrycat9000 commented Nov 11, 2024 • edited Loading

Description

Testing plan

Author checklist

github-actions bot commented Nov 11, 2024

ggetz commented Nov 13, 2024 • edited Loading

angrycat9000 commented Nov 13, 2024

ggetz left a comment

Choose a reason for hiding this comment

ggetz Nov 13, 2024

Choose a reason for hiding this comment

ggetz Nov 13, 2024

Choose a reason for hiding this comment

ggetz Nov 13, 2024

Choose a reason for hiding this comment

ggetz Nov 13, 2024

Choose a reason for hiding this comment

ggetz commented Nov 21, 2024

angrycat9000 commented Nov 21, 2024

angrycat9000 commented Nov 21, 2024

ggetz left a comment

Choose a reason for hiding this comment

ggetz Nov 22, 2024

Choose a reason for hiding this comment

angrycat9000 Nov 22, 2024

Choose a reason for hiding this comment

ggetz Nov 22, 2024

Choose a reason for hiding this comment

angrycat9000 commented Nov 22, 2024

ggetz commented Nov 22, 2024

angrycat9000 commented Nov 11, 2024 •

edited

Loading

ggetz commented Nov 13, 2024 •

edited

Loading