itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

Mark Bing Maps as deprecated, re-enable support for Azure Maps

Open hl662 opened this issue 1 year ago • 10 comments

closes #7009

  • Marks BingLocationProvider and BingElevationProvider as deprecated.
  • Mark BingMap in MapLayerOptions interface as deprecated.
  • Extend BackgroundMapProviderName to allow AzureProvider
  • Creates a new AzureLocationProvider component.
  • Uncomment AzureMapsMapLayerFormat in MapLayerImageryFormats.ts
  • Adds option to provide a IMJS_AZURE_MAPS_KEY in display test app.
  • Add a new Azure Maps object in MapLayerSources.json

Tested Azure Maps support on display test app, using an Azure Maps Key generated on my sandbox resource. Enable support for AzureMaps in MapLayerSettings. Azure Maps provides different tilesets for their imagery. BackgroundMapType.Street uses microsoft's base.hybrid.road, which doesn't show water or green spots denoting parks or fields: Contrast that to base.road, which BackgroundMapType.Hybrid will use: I have to note that there's no replacement for AerialWithLabels, which is what we use often in our Bing Maps implementation. The closest is microsoft.imagery, which has satellite imagery, but no road labels. I set BackgroundMapType.Aerial to use this:

hl662 avatar Jul 26 '24 15:07 hl662

@mdastous-bentley is it necessary and desirable to introduce another hard-coded map imagery provider as "special" the way we did MapBox and Bing? Especially when the new provider can't support our API contract like "aerial+labels"? BackgroundMapProps.providerName and BackgroundMapProps.providerData.mapType are long-deprecated.

pmconne avatar Jul 30 '24 16:07 pmconne

@mdastous-bentley is it necessary and desirable to introduce another hard-coded map imagery provider as "special" the way we did MapBox and Bing?

@pmconne They are not necessary, but without those special types you have to provide a complete 'ImageMapLayerSettings' definition to DisplayStyleState.MapImagerySettings.backgroundBase, and this include the service endpoint URL. So it depends if we want to keep this 'level' of functionality in itwin.js or not.

mdastous-bentley avatar Jul 30 '24 17:07 mdastous-bentley

They are not necessary, but without those special types you have to provide a complete 'ImageMapLayerSettings' definition to DisplayStyleState.MapImagerySettings.backgroundBase, and this include the service endpoint URL. So it depends if we want to keep this 'level' of functionality in itwin.js or not.

It would be nice if the BackgroundMapProvider could disclose what different types of imagery it provides, and allow user to select among them; rather than pretending that every provider supports all of and only the 3 hard-coded possibilities enumerated by BackgroundMapType. And if applications could register whatever BackgroundMapProviders they want, rather than being constrained to either using our small hard-coded possibilities or providing a complete ImageMapLayerSettings definition.

We're trying to fit a square peg into a round hole here with Azure maps.

pmconne avatar Jul 30 '24 18:07 pmconne

They are not necessary, but without those special types you have to provide a complete 'ImageMapLayerSettings' definition to DisplayStyleState.MapImagerySettings.backgroundBase, and this include the service endpoint URL. So it depends if we want to keep this 'level' of functionality in itwin.js or not.

It would be nice if the BackgroundMapProvider could disclose what different types of imagery it provides, and allow user to select among them; rather than pretending that every provider supports all of and only the 3 hard-coded possibilities enumerated by BackgroundMapType. And if applications could register whatever BackgroundMapProviders they want, rather than being constrained to either using our small hard-coded possibilities or providing a complete ImageMapLayerSettings definition.

We're trying to fit a square peg into a round hole here with Azure maps.

Actually this should be MapLayerFormatRegistry's job... but you still have have to provide a complete 'ImageMapLayerSettings' definition.

mdastous-bentley avatar Jul 30 '24 18:07 mdastous-bentley

@pmconne I noticed we this PR is not using map-layers-formats package.. I guess it would be hard to move because we want to to keep AzureMaps a "well-known" format right?

mdastous-bentley avatar Aug 01 '24 12:08 mdastous-bentley

@pmconne I noticed we this PR is not using map-layers-formats package.. I guess it would be hard to move because we want to to keep AzureMaps a "well-known" format right?

I am not sure exactly what you are asking, but the point of my questioning thus far has been: do we really want to introduce a new "well-known format", when I think we all agreed quite long ago that denoting Bing and MapBox as well-known formats was probably a mistake. Did you all discuss that privately and decide to move forward with the approach in this PR?

pmconne avatar Aug 01 '24 12:08 pmconne

@pmconne I noticed we this PR is not using map-layers-formats package.. I guess it would be hard to move because we want to to keep AzureMaps a "well-known" format right?

I am not sure exactly what you are asking, but the point of my questioning thus far has been: do we really want to introduce a new "well-known format", when I think we all agreed quite long ago that denoting Bing and MapBox as well-known formats was probably a mistake. Did you all discuss that privately and decide to move forward with the approach in this PR?

I don't know what was the initial thought process of the actual design, but if the goal is to assist the API consumer in creating a MapLayerSettings object, I'm sure we could create helper functions that doesn't hardcoding anything in core-frontend.

The other thing that feels wrong to me, is that we store the URL of the azure service endpoint in the view definition... as soon as Azure changes its API, any saved view get invalidated. Technically if we know the format id (AzureMaps), and style id (currently not stored), the provider should have enough information to generate the URL.

mdastous-bentley avatar Aug 01 '24 12:08 mdastous-bentley

if the goal is to assist the API consumer in creating a MapLayerSettings object, I'm sure we could create helper functions that doesn't hardcoding anything in core-frontend.

That would be my preference.

pmconne avatar Aug 01 '24 12:08 pmconne

if applications could register whatever BackgroundMapProviders they want, rather than being constrained to either using our small hard-coded possibilities or providing a complete ImageMapLayerSettings definition.

I would prefer this, I wasn't familiar with how background map tiles are fetched coming into working on this PR. On the side, AzureMapsLayerImageryProvider already existed in core-frontend prior to this PR, but it was disabled. Based on the discussion, do you want to immediately move that out of core-frontend, maybe to map-layer-formats in the extensions folder?

I don't know what was the initial thought process of the actual design, but if the goal is to assist the API consumer in creating a MapLayerSettings object, I'm sure we could create helper functions that doesn't hardcoding anything in core-frontend.

We do have an existing helper function to create an ImageryProvider in MapLayerFormatRegistry, given a MapLayerSettings input, although that function is marked as internal. Not to mention MapLayerFormats containing a function of the same name. Perhaps moving the former away from @internal, adding some examples of how to create a ImageMapLayerSettings (using the fromJSON method for example) and documenting how to register your own ImageryProvider to be used for background mapping via iModelApp can help here. If the guide already exists then my bad.

hl662 avatar Aug 01 '24 20:08 hl662

This pull request is now in conflicts. Could you fix it @hl662? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

mergify[bot] avatar Aug 02 '24 05:08 mergify[bot]