-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
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.
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.
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. |
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 @jjspace!
I left a few comments for the things that jumped out to me.
However, I understand that some of this is in flux. I believe at this point we have enough info to settle on the public API. Please do a pass on that and the docs, then ping me when ready for another a review pass.
const serviceResponse = await fetch("http://localhost:3000/service"); | ||
const { token } = await serviceResponse.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.
Thinking about user workflow, should this be encapsulated in ITwin.requestAccessToken
or a similar function?
That way, these first few code block is simplified to:
Cesium.ITwin.defaultAccessToken = await Cesium.ITwin.requestAccessToken();
and we could make small changes if needed without needing user code 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.
I could add a function like that but I don't think users should ever be running this themselves. They should do their own oauth and just set the token. This request is just for the sandcastle examples and will only provide access to the sample itwins that we add to the service account.
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 as it seams a bit strange to me that we would include a direct call to fetch
in a Sandcastle example.
It doesn't seem too foreign to me that we would have a function that abstracts away the relevant Cesium ion URL to get the access token under the hood that we plan on using for authentication. For example, this is similar to what IonResource.fromAssetId
is doing. In the future, we could potentially allow this function to reach out to an iTwin endpoint if available, or a user-provided endpoint without having to change the flow.
But let me know what you think.
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.
@jjspace Thoughts?
@ggetz thanks for the comments, this should be updated in response to all of them. I'm still looking into the reality data functionality and hope to have some functions for that added early next week. |
Sounds good! Please include those in a separate PR if this is not merged first. |
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.
My apologies for not keeping the documentation up-to-date, which made you develop all the endpoints, when a few would have been enough.
On the other side, I think it has also helped us to understand how to improve the documentation.
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 @jjspace! Once you take a pass on these comments, could you please update the PR description with the current status? I'm a bit lost on what you're considering ready vs what is temporary.
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 good to me!
(I just added a few nitpicking comments)
@ggetz I think this is getting close and ready for another look. I also updated the main PR description to indicate a summary of our decisions and the remaining tasks |
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.
Overall this is looking good @jjspace! Looking much cleaner overall.
Are we at a point where we can add unit tests and a CHANGES.md
update?
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 looks great, thanks @jjspace, can't wait to get our hands on it!
This is very nearly there now. We're just waiting on the ion route to be updated so auth can work for everyone but the sandcastle has been updated with the new sample data. Remaining:
The sandcastle should also have reality mesh data in it but that will be implemented through a follow up PR on Monday. |
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.
Looks good to 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.
Thanks for the update here @jjspace! The tests look great.
I did a final code pass, mainly on documentation and wording.
* @enum {string} | ||
*/ | ||
ITwinPlatform.ExportStatus = Object.freeze({ | ||
NotStarted: "NotStarted", |
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 these to show up on the documentation page, you'll need to add a jsDoc comment for each property, i.e.
NotStarted: "NotStarted", | |
/** | |
* A status indicating the export has not yet started. | |
* | |
* @type {string} | |
* @constant | |
*/ | |
NotStarted: "NotStarted", |
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 had intended these to be private but then the types generation will complain about the places they're used. One of the many issues with how private
is being used to mean both actually private and just not public outside internal code. I'll add descriptions but no users should really ever have to use these
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 tried adding these comments but it caused issues with the typescript build. I tried a couple different jsdoc arrangements and none seemed to both 1) make our typescript generation happy and accurate and 2) generate the docs in the correct place (under the ITwinPlatform
). I don't think JSDoc or tsd-jsdoc can handle the "nested" nature of these enums correctly...
I've removed for now but can try to look into it more if you think it's worth for this release
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's not ideal but I've added the values in the enum descriptions directly. I think that's an ok compromise for now.
@ggetz this has been updated with the ion oauth route. I believe this PR should be good to go now. Reality data PR open very soon |
🎉 This is looking great @jjspace! Two comments on the Sandcastle:
![]()
|
@ggetz Good to go I think |
Awesome! Thanks @jjspace! And a big thanks to @danieliborra and @aruniverse for your help! |
Description
Beginning steps for the iTwin/iModel integration utilizing the
mesh-export
APIITwinPlatform.defaultAccessToken
.Assorted tasklist
defaultAccessToken
even make sense if different routes need different scopes?Issue number and link
no issue number
Testing plan
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change