- 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.7k
 
Add Azure 2D imagery provider #13001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| 
           Thank you for the pull request, @lukemckinstry! ✅ We can confirm we have a CLA on file for you.  | 
    
| 
           ready for review @ggetz  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lukemckinstry! I did an initial pass on the code and have a few questions and comments.
I will follow up to test this shortly.
        
          
                packages/widgets/Source/BaseLayerPicker/createDefaultImageryProviderViewModels.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | style: Cesium.IonWorldImageryStyle.AERIAL_WITH_LABELS, | ||
| }), | ||
| baseLayer: Cesium.ImageryLayer.fromProviderAsync( | ||
| Cesium.IonImageryProvider.fromAssetId(3830183), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation for changing this away from Bing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was missed in the last release for Google imagery. This sandcastle corresponds to and is linked from the imagery tutorial https://cesium.com/learn/cesiumjs-learn/cesiumjs-imagery/ We updated the tutorial and sample code to use Google but forgot to update the sandcastle.
| this._subscriptionKey = | ||
| options.subscriptionKey ?? options["subscription-key"]; | ||
| //>>includeStart('debug', pragmas.debug); | ||
| Check.defined("options.tilesetId", options.tilesetId); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into this in https://github.com/CesiumGS/cesium/pull/13004/files#diff-8c9bd0da5a2315f5b1211e299d0d1517027f1bf8e7a3eab29a601b6761f548f1R46, but it looks like there is a discrepancy between the docs for Azure2DImageryProvider.ConstructorOptions which specifies a default and the implemented behavior.
In my PR, I implemented the default value. Please let me know if that assumption was wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, there was a discrepancy, implementing the default is good 👍
| this._tileCredits = resource.credits; | ||
| this._attributionsByLevel = undefined; | ||
| // Asynchronously request and populate _attributionsByLevel | ||
| this.getViewportCredits(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The promise kicked off by this function could reject unpredictably, and that can lead to confusing behavior in isolation, like when called in a unit test.
Ideally, we should not kick off any async processes in the constructor. See how I handled this in https://github.com/CesiumGS/cesium/pull/13004/files#diff-7142af57f22c9916aeeac920512bb9d8211e4a6b769201230de03902b119fa1bR487-R501 and let me know if you have any feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense if the requestImage function is a better place for it to fail gracefully, I'll do the same update for Azure2DImageryProvider
| level, | ||
| ), | ||
| ); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly required to merge this PR, but I wonder if we can more lazily request tile credits as needed based on the current zoom level, and potentially extents, alongside the image requests in requestImage.
This would also potentially apply to Google2DImageryProvider.
| 
           thanks for the review @ggetz, ready for another pass  | 
    
| 
           Thanks @lukemckinstry! The updates look good to me from a code standpoint. I'll do a final testing pass once we merge #13004 and the final asset IDs are in.  | 
    
c98e179    to
    fe54e66      
    Compare
  
    
Description
Switches on the Azure2DImageryProvider class. Also adds viewport attribution logic for Azure layers.
To Do
DO NOT MERGE until complete
Issue number and link
Testing plan
NOTE: Test using ion staging asset IDs until prod assets are ready
Sandcastles (to publish in prod)
To view the sandcastles, add the key to your Ion Staging account.
Test both ways to add Azure 2D data
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change