-
Notifications
You must be signed in to change notification settings - Fork 214
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
[wip] add support for iTP curated-content/cesium endpoint #7436
base: master
Are you sure you want to change the base?
Conversation
if (!accessTokenAndEndpointUrl.token || !accessTokenAndEndpointUrl.url) { | ||
notifyTerrainError(IModelApp.localization.getLocalizedString(`iModelJs:BackgroundMap.MissingCesiumToken`)); | ||
/** @beta */ | ||
export async function createCesiumTerrainProvider(opts: TerrainMeshProviderOptions & Partial<CesiumContentAccessTileProps>): Promise<TerrainMeshProvider | undefined> { |
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.
hoping to get the displays team input here on what theyre preference is
should we expose a helper fn like this or just change the tag on CesiumTerrainProvider?
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.
@pmconne let me know if you have thoughts
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.
Why is this beta and how is somebody expected to use it? Not every curated Cesium asset represents "terrain".
This is intended for backport to 4.11, right? |
Correct, this will be one of the only new features in 4.11 |
Does this PR's current state represent the API+implementation you are actually proposing, or at least a step towards it? If not, what are you actually proposing? |
As you mentioned, not every Cesium Curated Content is a "terrain", adding a link for content types for the reference of others. I tried reviewing what we support today, let me know if you disagree with this, or have examples of where we support these different types.
Idk if we can add support for all of these super quickly but started off with Terrain as that is the easiest and most decoupled content type it seems, and the most popular cesium asset Applications use. Without adding a direct dependency on iTwin Platform within iTwin.js-core itself as well as the requirement to have an iTwinId to use the new iTP api, it seems the cleanest way to add support for Curated Cesium Terrain is have apps add a custom TerrainPovider to IModelApp.terrainProviderRegistry. We already have an internal CesiumTerrainProvider (which depends on a CesiumION token) that we register by default, and we create/add it using some helper functions. The proposal to support curated cesium terrain would be expose the same helper function, and have apps register the provider at either startup using their Account iTwin id, or on iModel open with the iTwinId of that iModel. For gLTF, not sure if we have a general solution for it, and didn't realize OSMBuildings from Cesium were rendered via the RealityDataSource. But if we are ok with the approach for Terrain above, we can apply it and propose apps add a CuratedCesium RealityDataSource provider. For imagery, I imagine Apps should be able to create and add a custom Background Map provider. There is a larger discussion about these providers to be had in general, see. At the end of the day, I don't believe it is the responsibility of iTwin.js core to be making requests to the new iTP apis for curated cesium content, and we should just be responsible for providing interfaces and helpers to allow app developers to view these custom content in itwin.js |
I guess I was under the impression that you had a much smaller scope for 4.11, which would simply be to change the handful of places (CesiumTerrainProvider and OSM buildings reality model) where we talk to ION directly using an ION token, to instead optionally talk to the new service using an iTP token. That seems like it would require very little if any new API, and whatever new APIs around the new service could be introduced more deliberately in 5.x. |
…n, need to remove hardcoding of base itp url
So I contemplated modifying the existing handful of places where we talk to ION, but its not a 1:1 replacement of an ION token for an iTP token, you also need an iTwinId. Majority of users should be ok wit this, as they don't use BlankConnections, but if you use a BlankConnection then you need to create add your own TerrainProvider to the registry, which maybe we can circle back to in 5.0. I modified the approach and pushed up changes fitting your point above about a smaller scope for 4.11, (only for terrain atm). The one wrinkle here is we need to allow users to override the iTP base url to handle other envs. We removed all references and hardcoding of envs in core in 3.0 and suggested apps inject this at the app level. We can add an alpha property to IModelApp for iTP baseUrl, but i feel thats messy. I think this comes down to: |
Why is an iTwin required to access curated global content? |
Adding @davidhjones to answer better than I could, but I believe the thought was these should be "repositories" tied to an iTwin |
This has been the approach from 3.0 onward and I like the approach taken in Display Test App here as it helps further the de-coupling. If we add a dependency on a service url in iTwin.js we are limited by our dependencies and require coordination when they change (such as the Bing Maps). |
@pmconne based on this, do you still object to the initial proposal? For the 4.11 release we would just want to expose some helpers to allow creating a CesiumTerrainProvider, and we would mark it as beta, potentially removed altogether in 5 |
@@ -0,0 +1,154 @@ | |||
import { CesiumTerrainAssetId } from "@itwin/core-common"; |
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.
Are the contents of this file intended to represent some public open-source package we will make available for any app to use?
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.
Ideally not, it would be sample code in our test apps and maybe a sample in the sandbox, as well as our docs, of how app teams can implement this themselves
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.
You expect every app to have to implement / copy-paste this big ugly blob of code just to be able to use the terrain that they currently get just by providing an API key? That seems like a huge downgrade and quite error-prone.
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.
Granted its very ugly right now, and needs to be cleaned up, but there isn't that much code that needs to be added. App developers can customize it further to meet their needs, eg not rely on IModelApp to obtain access token, they might use an http client that additional caching, etc. Introducing another package to our already growing number of packages, doesn't seem responsible, especially as this is a beta api and very likely to change
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.
You have effectively defined a client library for the new curated content API already. Do you really expect every consumer to write their own interfaces and enums representing types defined by that REST API? Write their own logic to rustle up an iTwin Id to pass to an API that does not give two hoots what iTwin Id you give it?
I understand not wanting to have a direct call to iTP from within itwinjs-core. But can we show a little regard for our users who will have to deal with all of this unnecessary new friction? Where's the value-add to justify it?
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 would like to disagree. While I have added the interfaces/enums they aren't really being used (except to cast for convenience but not required), and as they are tied to the service in tech preview, they are subject to change (potentially). At the end of the day it is a single HTTP get req to the new service, the need to create a separate pkg for this isnt justified imo. The "logic to rustle up an iTwin Id" should be the responsibility of an App not iTwin.js Core or any specific client imo.
The "value add" is from a business perspective. Now, you do not need to pay for Cesium ION to take advantage of global Cesium Assets if you are already an iTwin Platform Customer. The issue is how do they take advantage of this, and it should be as simple as we expose a helper for Apps to use to populate and use in their app.
Maybe long term this could be extracted out into a separate package, or even an extension, but at this point in time for the current objectives it should be seen as out of scope.
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.
it should be as simple
Can you please make it look simple then?
this.updateBackgroundMap({ applyTerrain: enable }); | ||
this.updateBackgroundMap({ | ||
// terrainSettings: { | ||
// providerName: "CuratedCesiumContent", |
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.
Is this supposed to be commented out or not?
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 commented out in a166d5c to demonstrate the other alternative solution. i will drop that commit and clean things up, give me a few.
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 ask because introducing a new providerName
raises questions about what happens to all the existing saved views that currently use CesiumWorldTerrain
as their provider name, once one or more apps decide to stop providing a Cesium ion key or we stop supporting it in core. Terrain would no longer display despite being enabled in the view. In the opposite direction, any saved views created once this change is merged will display no terrain when opened in a viewer that's on 4.10 or earlier.
Having each app implement their own curated terrain provider also introduces the risk that they do so with different provider names, which will cause a saved view created in one app to fail to display terrain when opened in a different app.
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.
Hmm this is a great catch.
I don't think we can ever remove the current default cesium ion key terrain provider, as that is the only way for users to be able to pull in their own custom cesium assets.
I guess core could export a default providerName to use for this scenario that others should use to register and also use for when changing background map props via vp.changeBackgroundMapProps
to use the specific provider.
Idk if there is a solution for providing fallbacks if apps register any custom properties in a saved view. Need to think about this more.
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 think the best we can do for backwards compatibility with the current default cesium provider and a move to a curated ceisum content provider, is we export a recommended key/provider name for apps to use, and in the display system when trying to open up a saved view that references that specific provider, attempt to load it, and if not found, attempt to fallback to the default cesium provider.
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.
That is only the best we can do if we accept the constraints that 1) we will not make a direct call to the service from core-frontend and 2) we won't provide a reusable package, instead requiring every app to reimplement it themselves and 3) we insist on requiring an iTwin ID to access data not associated with any iTwin. Even if we accept all those constraints, we can do at least somewhat better by permitting apps to overwrite the provider associated with the name "CesiumWorldTerrain" instead of introducing a new provider name.
To be clear: 99.9% of users neither know nor care where the terrain comes from. They see it as data that we provide, all they care about is that when they open their saved view, they see the terrain. To my knowledge, exactly one user has created their own custom terrain provider. I think we should interpret "CesiumWorldTerrain" to mean "terrain supplied by iTwin.js, wherever it comes from", unless they also specify a non-public asset Id.
…ck to ion, need to remove hardcoding of base itp url" This reverts commit a166d5c.
iTwin Platform is adding a new service, Cesium Curated Content, to allow iTwin developers to more easily consume public Cesium Ion Asset; eg Cesium World Terrain or Cesium World Bathymetry.
Please note the 2 iTP APIs and their Cesium Ion equivalents:
iTP List Contents <> Ion getAssets
iTP Access tiles <> Ion Access Tiles
This new service will allow users to not need to provide their own cesiumIonKey to access public Cesium Assets as long as they have a valid access token for iTP.
Notes: