-
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
iTwin Reality Data - add GeoJSON/KML support #12344
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
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, this looks good overall. I put 2 minor comments.
I guess we want to wait for the final data before merging?
@@ -38,54 +38,19 @@ ITwinPlatform.ExportType = Object.freeze({ | |||
}); | |||
|
|||
/** | |||
* Types of Reality data | |||
* Types of Reality data. This is a partial list of types we know we can support |
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 are we dropping the unsupported types here?
In the loading functions, we already create a local supportedRealityDataTypes
array, so it seems like this enum could still retain all valid types, including unsupported 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.
This was requested by the iTwin team. The types are potentially changing and/or not a complete list so I trimmed this down to only those types we really care about. I still wanted to keep it in the enum instead of only in the array of supportedRealityTypes
to make it centrally located and easy to change later.
// Create tileset of the reality data mesh | ||
// TODO: swap this out with a different mesh | ||
const realityMeshId = "85897090-3bcc-470b-bec7-20bb639cc1b9"; | ||
const realityMesh = await Cesium.ITwinData.createTilesetForRealityDataId( |
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.
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 a problem in the other sandcastle as well. 4
seemed to be a decent value overall but maybe not that far zoomed out. This is not the final mesh going to be used for this Sandcastle so I can re-evaluate that when I update to the other data.
Thanks for the review @jjhembd!
That is correct. The code shouldn't change but I will change the iTwin and specific reality data assets the sandcastle is pointing at before this is ready to merge |
Description
This is a follow up to #12334 to add support for GeoJSON and KML data types from the Reality Management API.
The implementation should be good to go but the Sandcastle will need updated to the iTwin and data we actually want to use.
Issue number and link
No issue
Testing plan
iTwin Feature Service
sandcastle and make sure all data is loading as expectedAuthor checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change