-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
iModel integration #12289
base: main
Are you sure you want to change the base?
iModel integration #12289
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
@ggetz just pushed an update to restructure the api a bit. I moved the itwin API functions under the Looking at the API more it seems exports can be created for the same "model id + changeset" with different export parameters which could mean multiple CESIUM type exports for the same combo. I think this is probably less likely and when it's needed then users can use the load function with the export id directly and manage their own handling of the list. |
@aruniverse would you or another iTwin colleague be able to take a look at this? |
Thanks for sharing, this is exciting to see! I'll take a pass over it. fyi @danieliborra |
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.
Did a quick first pass, Josh this looks pretty good so far. I will look at trying to run this locally tomorrow.
Added some minor nit comments and notes. I've tagged some of the other leads from iTwin Platform working on the services you are using here for visibility.
"use strict"; | ||
//Sandcastle_Begin | ||
// must be created for the Start export route if you want to create new exports | ||
// Needs to have the mesh-export:modify scope not just mesh-export:read |
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.
We recently consolidated all scopes into a single one, "itwin-platform". That should be the recommended scope to use. Please see https://developer.bentley.com/itwin-platform-scope-introduction/
The "try it out" feature in the developer portal is just still using granular scopes.
// Needs to have the mesh-export:modify scope not just mesh-export:read | |
// Needs to have the `itwin-platform` scope |
fyi @dsmailys
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 saw the pages about the itwin-platform
but that's not what's documented on the mesh-export routes. Those all still state they require the more granular scopes. If this is no longer true then the docs should be updated to reflect that.
The "try it out" feature in the developer portal is just still using granular scopes.
This is the primary way I'm generating these tokens for now as I have not found a way to generate long living API keys/tokens. That's why I called out the scope to make sure to generate it on the correct "try it out" page.
If every route should be using the general itwin-platform
scope then the "Try it out" should probably switch to always generating with that scope instead of the granular ones.
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.
Hey, thanks for noticing and reporting this. I've let the leadDeveloper of mesh-export know about this inconsistency and will hopefully get resolved. itwin-platform scope will still work/be accepted for mesh-export.
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 "try it out" feature in the developer portal is just still using granular scopes.
Reported this back to the developer portal team. Thanks for bringing this up.
packages/engine/Source/Core/ITwin.js
Outdated
/** | ||
* Default settings for accessing the iTwin platform. | ||
* | ||
* Keys can be created using the iModels share routes {@link https://developer.bentley.com/apis/imodels-v2/operations/create-imodel-share/} |
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.
Please note, this API has the following note:
Important : Share operations are in closed preview mode currently. Hence only selected applications can utilize the Share API.
fyi @robertas-kerpys, we will need to work with Cesium team here
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 don't think we plan to use the model share api here at all, I don't think it gives us what we need. This was an early comment when I thought we would be.
We definitely will need a good strategy for auth in general though. it's one of the biggest open questions we have right now. I think that discussion should live outside this specific thread.
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.
For demo purposes, we're working right now on adding shared keys to the mesh export service. I expect to have it deployed in the next 4 weeks, if all goes as planned.
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.
Hi @danieliborra! We're targeting these changes for the end of November of so that we can (ideally) ship them with the December 1 CesiumJS release. Given that, what is the recommended approach?
/** | ||
* Gets or sets the default iTwin access token. | ||
* | ||
* TODO: I'm not sure we can even do this kind of access token. Each route seems to need it's own scopes |
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 should be ok now that we've consolidated all scopes to "itwin-platform"
|
||
const headers = { | ||
Authorization: ITwin.defaultAccessToken, | ||
Accept: "application/vnd.bentley.itwin-platform.v1+json", |
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 is fine for this specific use case where youre using just a single API (mesh-export), but might prove to be an issue when you are mixing APIs(iTwins, iModels, etc)
We use request headers to denote which verison of an API to use. So far, most APIs are still on v1, but there are some for example the iModels APIs which are on v2, and the accept
header would need to be application/vnd.bentley.itwin-platform.v2+json
Please see API Version Policy
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 was going off the sample code provided to us and the documentation which states for every route in the mesh export API that:
Setting to
application/vnd.bentley.itwin-platform.v1+json
is recommended
If the docs are wrong then they should be updated. On top of that I'm currently setting this per request so different routes could definitely use different values if needed for different versions as you say. As a consumer of the API I would fully expect all the routes on the same API to still work with each other if versioning is done this way.
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.
Yes this will work, the entire mesh-export service apis are still on v1 so the accept header here is right.
My apologies, I jumped the gun when I had initially commented this thinking you were going to use multiple api routes in a single function.
|
||
// This link is only valid 1 hour | ||
let tilesetUrl = exportObj._links.mesh.href; | ||
const splitStr = tilesetUrl.split("?"); |
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, im not sure if you need to do all this wth splitting the link url.
This is how I do it in my test app
const baseUrl = new URL(meshExportUrl.export._links.mesh.href);
baseUrl.pathname = baseUrl.pathname + "/tileset.json";
tilesetUrl = baseUrl.toString()
// must be created for the Start export route if you want to create new exports | ||
// Needs to have the mesh-export:modify scope not just mesh-export:read | ||
const accessToken = | ||
"Bearer eyJhbGciOiJSUzI1NiIsImtpZCI6IkJlbnRsZXlJTVNfMjAyNCIsInBpLmF0bSI6ImE4bWUifQ.eyJzY29wZSI6WyJtZXNoLWV4cG9ydDpyZWFkIiwibWVzaC1leHBvcnQ6bW9kaWZ5Il0sImNsaWVudF9pZCI6Iml0d2luLWRldmVsb3Blci1jb25zb2xlIiwiYXVkIjpbImh0dHBzOi8vaW1zLmJlbnRsZXkuY29tL2FzL3Rva2VuLm9hdXRoMiIsImh0dHBzOi8vaW1zb2lkYy5iZW50bGV5LmNvbS9hcy90b2tlbi5vYXV0aDIiLCJodHRwczovL2ltc29pZGMuYmVudGxleS5jb20vcmVzb3VyY2VzIiwiYmVudGxleS1hcGktbWFuYWdlbWVudCJdLCJzdWIiOiJjMWM1MzRhNy0zZDk2LTQ2MzMtYjY5ZC1jMGEzNzE5OWQwZGUiLCJyb2xlIjoiQkVOVExFWV9FTVBMT1lFRSIsIm9yZyI6ImZhYjk3NzRiLWIzMzgtNGNjMi1hNmM5LTQ1OGJkZjdmOTY2YSIsInN1YmplY3QiOiJjMWM1MzRhNy0zZDk2LTQ2MzMtYjY5ZC1jMGEzNzE5OWQwZGUiLCJpc3MiOiJodHRwczovL2ltcy5iZW50bGV5LmNvbSIsImVudGl0bGVtZW50IjpbIkJFTlRMRVlfTEVBUk4iLCJJTlRFUk5BTCIsIlNFTEVDVF8yMDA2IiwiQkVOIiwiQkROIl0sInByZWZlcnJlZF91c2VybmFtZSI6Ikpvc2guUm91emVyQGJlbnRsZXkuY29tIiwiZ2l2ZW5fbmFtZSI6Ikpvc2giLCJzaWQiOiJHMGZTamlOXzBMRjE1Vy1IYnRTaWtoeDNuSXMuU1UxVExVSmxiblJzWlhrdFZWTS5LbWlWLlF2UExVcGJyc2R3engwWGxPbkF3eTFTT0IiLCJuYmYiOjE3MzA4MzgyNDIsInVsdGltYXRlX3NpdGUiOiIxMDAxMzg5MTE3IiwidXNhZ2VfY291bnRyeV9pc28iOiJVUyIsImF1dGhfdGltZSI6MTczMDgzODU0MiwibmFtZSI6Ikpvc2guUm91emVyQGJlbnRsZXkuY29tIiwib3JnX25hbWUiOiJCZW50bGV5IFN5c3RlbXMgSW5jIiwiZmFtaWx5X25hbWUiOiJSb3V6ZXIiLCJlbWFpbCI6Ikpvc2guUm91emVyQGJlbnRsZXkuY29tIiwiZXhwIjoxNzMwODQyMTQyfQ.N_WrgjL2bqxdNLEM5nHh4Fg-FzeA-qxxpryaoaMKz8onnpgzmp-X8dyQ1TyMRqKQY99iLypE9bU45DwRJs7vBgNQ73d53SkC9TKGYn8AAOTJz8c_oEHgkoTEDaaLsMPCw88tqZ34pY0e0oIHIofVDTCvwlzwaJPADfkIxz-8GzhIt2WrXR7f6LWBFSlWNrtNhIr9Tz7UNaLOh97_3fS9KVU1I084CpBga9cj_mjGBeki7mIEQvpqMj8x2bJPae_c6WrPEjKLayrOzq4q0X0Kvle0ZFAm-Se9MFCusTFS51bYrI3IsjagGeIP2U06zzcMJ22NylomE60hz6GK_4uB3g"; |
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.
Would recommend not to commit an access token in text like this, but these are short lived (1 hr) so its probably expired now
Appreciate the fast turn, thanks @aruniverse. |
@aruniverse thank you for the thorough comments so quick! This is still a Draft PR and very much a work in progress and nothing is final. I agree with most of your comments but it was not ready for such scrutiny 😅 I opened the PR to work through the general process and API structure that we'd need. A lot of the code is still very much thrown together to prove out the concept |
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 looks promising.
We're now modifying the API, so we will keep you updated of any change in the API.
packages/engine/Source/Core/ITwin.js
Outdated
const ExportType = Object.freeze({ | ||
"3DFT": "3DFT", | ||
GLFT: "GLTF", | ||
IMODEL: "IMODEL", | ||
CESIUM: "CESIUM", | ||
}); |
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.
We're currently working on the mesh-export
API, and we changed this quite a bit. I updated the documentation on Monday, but it will take some time to be deployed.
3DFT and GLTF are now deprecated, IMODEL is for internal use only (until we deprecate it) and now you can use "3DTILES", which is what I would recommend to use.
packages/engine/Source/Core/ITwin.js
Outdated
/** | ||
* Default settings for accessing the iTwin platform. | ||
* | ||
* Keys can be created using the iModels share routes {@link https://developer.bentley.com/apis/imodels-v2/operations/create-imodel-share/} |
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.
For demo purposes, we're working right now on adding shared keys to the mesh export service. I expect to have it deployed in the next 4 weeks, if all goes as planned.
// obtain export for specified export id | ||
const url = `${ITwin.apiEndpoint}mesh-export/${exportId}`; | ||
|
||
// TODO: this request is _really_ slow, like 7 whole second alone for me |
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 because get-export
is not very used and so we haven't optimized it yet. All apps use get-exports
, and we have kept get-export
to avoid breaking changes.
This quarter we will clean-up the API.
packages/engine/Source/Core/ITwin.js
Outdated
}; | ||
|
||
// obtain export for specified export id | ||
let url = `${ITwin.apiEndpoint}mesh-export/?iModelId=${iModelId}`; |
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'd recommend to use &exportType=3DTILES
to filter out other export types, and change the exportType enum above to 3DTILES too.
Also, you can do &$top=5
to get the latest 5 exports, for performance reasons.
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.
@danieliborra I was hoping for a way to filter based on type but none are documented so I didn't think it was possible. Are there other properties filterable like this?
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 are right. Since the API is in beta/tech. preview but products/users are still using it, we have added some experimental parameters that are not documented because we planed to remove them in the future (this quarter). We didn't want users to implement them and then introduce breaking changes.
We're currently updating the API documentation and cleaning some of those parameter, but it will still take a few days to be deployed in PROD.
* @param {string} iModelId | ||
* @param {string} changesetId | ||
*/ | ||
ITwin.createExportForModelId = async function (iModelId, changesetId) { |
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.
Today, we're auto-generating tiles for all iModels but for the exportType=IMODEL.
We plan to also auto-generate "3DTILES". Development is on-going, we expect to have it end of this quarter.
We also plan to deprecate this endpoint when the auto-generation is in place.
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.
@danieliborra We plan on marking all of these new APIs as "experimental" so we have some more flexibility with breaking changes. So we can remove this function when recommended.
/** | ||
* Delete the specified export | ||
* | ||
* TODO: I'm not sure if we want this or not. Might belong better as an APP level function |
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 won't need it, but it is a good exercise to test the API
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.
We'll likely want to pair the API here down to the minimum necessary for visualization– Managing exports and other function is something that would not be unique to CesiumJS, so to reduce the maintenance burden we can omit it.
* if (!Cesium.defined(tileset)) { | ||
* const exportId = await Cesium.ITwin.createExportForModelId(imodelId, changesetId, accessToken); | ||
* tileset = await Cesium.createIModel3DTileset(exportId); | ||
* } |
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.
We're doing it right now for demo purposes, but that will go away in the future.
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.
@danieliborra Is this referring to the CESIUM
export type? Or something else?
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.
Generating exports manually will go away, as we're implementing right now the auto-exporting for all formats, including CESIUM.
Given that the iTwin platform currently has no way to create long running API keys I spent some time looking into the oauth workflows. The main goal right now is to be able to create one or more sandcastles that can demonstrate using iModels in CesiumJS without requiring a login from the users. @ggetz had the idea to use the ion servers for oauth and return a token that can be used in Sandcastle. My latest commit includes a demo server in The only way I found to add the app user was through the Access Control API which needs you to create a role first for the role id. I found the sandcastle was able to load data using a service app token with these permissions on it's role: The demo sandcastle has been updated to use the new local auth server. If you want to try the Web App flow that requires a login just uncomment/comment the code near the top and reload the sandcastle then log in. You will need access to the iModel you're trying to load. |
Description
Beginning steps for the iTwin/iModel integration utilizing the
mesh-export
APIOpening as a draft for discussion, more details later. The api is a little messy and function names/encapsulation is planned to change. I wasn't sure if we wanted to expose more functions to interact with the iTwin API or just have one function to load export data. It's sort of a separation of concerns thing about what should belong in CesiumJS vs be implemented on the "app side" around CesiumJS.
Assorted tasklist
defaultAccessToken
even make sense if different routes need different scopes?Issue number and link
no issue number
Testing plan
start export
route and click "Try it now"iTwin Demo.html
. These keys only last 1 hour so you may have to refresh itAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change