-
Notifications
You must be signed in to change notification settings - Fork 215
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?
Changes from all commits
5efdb7b
0ba3dfb
01d9963
b0cbd23
a166d5c
e66dd66
3277a8e
a8c6ddf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
import { CesiumTerrainAssetId } from "@itwin/core-common"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Can you please make it look simple then? |
||
import { | ||
createCesiumTerrainProvider, | ||
IModelApp, | ||
type TerrainMeshProviderOptions, | ||
type TerrainProvider, | ||
} from "@itwin/core-frontend"; | ||
|
||
interface ITPReq { | ||
accessToken: string; | ||
baseUrl?: string; | ||
} | ||
|
||
interface CuratedCesiumContentOptions extends ITPReq { | ||
assetId: number; | ||
iTwinId: string; | ||
} | ||
|
||
function genHeaders(accessToken: string) { | ||
return { | ||
headers: { | ||
/* eslint-disable @typescript-eslint/naming-convention */ | ||
Authorization: accessToken, | ||
Accept: "application/vnd.bentley.itwin-platform.v1+json", | ||
Prefer: "return=representation", | ||
/* eslint-enable */ | ||
}, | ||
}; | ||
} | ||
|
||
async function getCuratedCesiumContentProps({ | ||
assetId, | ||
iTwinId, | ||
accessToken, | ||
baseUrl, | ||
}: CuratedCesiumContentOptions): Promise<{ token?: string; url?: string }> { | ||
const apiUrl = `${ | ||
baseUrl ?? "https://api.bentley.com" | ||
}/curated-content/cesium/${assetId}/tiles?iTwinId=${iTwinId}`; | ||
|
||
try { | ||
const res = await fetch(apiUrl, genHeaders(accessToken)); | ||
if (!res.ok) { | ||
return {}; | ||
} | ||
const accessTileProps = (await res.json()) as { | ||
accessToken: string; | ||
url: string; | ||
}; | ||
return { token: accessTileProps.accessToken, url: accessTileProps.url }; | ||
} catch (e) { | ||
// eslint-disable-next-line no-console | ||
console.error(e); | ||
return {}; | ||
} | ||
} | ||
|
||
// https://developer.bentley.com/apis/itwins/operations/get-my-primary-account/ | ||
async function getAccountITwin({ | ||
accessToken, | ||
baseUrl, | ||
}: ITPReq): Promise<string> { | ||
const apiUrl = `${ | ||
baseUrl ?? "https://api.bentley.com" | ||
}/itwins/myprimaryaccount`; | ||
|
||
try { | ||
const res = await fetch(apiUrl, genHeaders(accessToken)); | ||
if (!res.ok) { | ||
return ""; | ||
} | ||
|
||
const { iTwin } = (await res.json()) as { iTwin: { id: string } }; | ||
return iTwin.id; | ||
} catch (e) { | ||
// eslint-disable-next-line no-console | ||
console.error(e); | ||
return ""; | ||
} | ||
} | ||
|
||
export async function registerCesiumCuratedContentProvider({ | ||
iTwinId, | ||
baseUrl, | ||
}: { | ||
iTwinId?: string; | ||
baseUrl?: string; | ||
}): Promise<void> { | ||
const providerName = "CuratedCesiumContent"; | ||
|
||
const provider: TerrainProvider = { | ||
createTerrainMeshProvider: async (options: TerrainMeshProviderOptions) => { | ||
const accessToken = await IModelApp.authorizationClient?.getAccessToken(); | ||
if (!accessToken) { | ||
return undefined; | ||
} | ||
|
||
iTwinId = iTwinId ?? (await getAccountITwin({ accessToken, baseUrl })); | ||
aruniverse marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const { token, url } = await getCuratedCesiumContentProps({ | ||
assetId: +(options.dataSource || CesiumTerrainAssetId.Default), | ||
iTwinId, | ||
accessToken, | ||
baseUrl, | ||
}); | ||
return createCesiumTerrainProvider({ | ||
...options, | ||
accessToken: token, | ||
url, | ||
}); | ||
}, | ||
}; | ||
|
||
IModelApp.terrainProviderRegistry.register(providerName, 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.
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".