-
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
Added I3S data source support in Cesium #9634
Conversation
Thanks for the pull request @Tamrat-B!
Reviewers, don't forget to make sure that:
|
Thanks for doing this, this will be incredibly valuable for us! |
We can preview video intro of this version at https://youtu.be/0C2fvQXqODQ?t=1793 |
Hi, I have one small issue. |
- clamp to default min & max values in ArcGISTiledElevationTerrainProvider if not available - Improved pick accuracy (Find a triangle touching the point px,py,pz, then return the vertex closest to the search point)
- Removed default access tokens
@Tamrat-B thanks to you and team for this pull request! As we discussed offline, the Cesium team has been heads down on 3D Tiles Next (now ready for community feedback!). As part of this effort, much of the glTF and 3D Tiles engine in CesiumJS was rewritten. That work is now merged into I think we are at the point where we can spare a few cycles to do a first pass review on this pull request, but if it needs a lot of iteration on feedback, we may be slow as our main focus is on solidifying the draft extensions for 3D Tiles Next. @lilleyse could you or someone else in the 3D world take an initial look at this? Perhaps a few things to consider:
🙏 🙏 🙏 |
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 again for your contribution @Tamrat-B. I've done a pass through this PR and the Sandcastles look great. I have a few comments: mostly looking at the architecture and I've left some suggestions for how some of the components may be better integrated with CesiumJS' existing functionality for things like math, projections, web workers, etc.
Source/DataSources/I3SDataSource.js
Outdated
* | ||
*/ | ||
|
||
function I3SDataSource(name, scene, options) { |
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 implemented as a Primitive
, similar to how Cesium3DTileset
is implemented.
Source/DataSources/I3SDataSource.js
Outdated
|
||
// Prevent ESLint from issuing warnings about undefined Promise | ||
// eslint-disable-next-line no-undef | ||
var _Promise = Promise; |
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.
Native Promises are not currently used in Cesium.js, so all promises should be handled by using when.js
.
Source/DataSources/I3SDataSource.js
Outdated
|
||
// Code traces | ||
// set to true to turn on code tracing for debugging purposes | ||
var _tracecode = false; |
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.
Logging support is a good idea, however, I think we'd want to implement a system throughout CesiumJS (ideally, on with support for different levels of logging), not just in one module.
Source/DataSources/I3SDataSource.js
Outdated
* | ||
*/ | ||
|
||
function I3SDataSource(name, scene, options) { |
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 top level I3S
class should have some debug parameters like Cesium3DTileset
- things like maximumScreenSpaceError
, optimization options etc.
Source/DataSources/I3SDataSource.js
Outdated
if (that._traceFetches) { | ||
console.log("I3S FETCH:", uri); | ||
} | ||
var request = fetch(uri); |
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.
Network requests should go through the Resource
class whenever possible.
Source/DataSources/I3SDataSource.js
Outdated
.########.########.########.####.##.........######...#######..####.########..##.....##.######## | ||
*/ | ||
|
||
function mercatorAngleToGeodeticLatitude(mercatorAngle) { |
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.
Projection related code should go into a Math or Projection class.
Source/DataSources/I3SDataSource.js
Outdated
.########..########..######...#######..########..########.##.....## | ||
*/ | ||
|
||
function decodeDracoEncodedGeometry(data, bufferInfo) { |
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 functionality of DracoLoader
can be extended with these functions here. Even though it currently may be very glTF-specific, it should be ok to add new functions there that simply transform the data.
Source/DataSources/I3SDataSource.js
Outdated
.##....##..##.....##.##.......##.....##.##...... | ||
..#####.##..#######..########..#######..######## | ||
*/ | ||
function I3SGLTFProcessingQueue() { |
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, all worker related code should use TaskProcessor
.
Source/DataSources/I3SDataSource.js
Outdated
}; | ||
|
||
// Add an entry in the data source content cache | ||
_i3sContentCache[this._completeUriWithoutQuery] = { |
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 Cesium3DTilesetCache
is not fully being utilized here, given that some of the Cesium3DTile
functions are being overwrriten. It may be possible to use another cache system here, but as mentioned below, it would ideally store the transcoded ModelComponents
and not the raw payload, to avoid retranscoding.
Source/DataSources/I3SDataSource.js
Outdated
* @param {I3SNode} [parent] The parent of that feature | ||
* @param {String} [uri] The uri to load the data from | ||
*/ | ||
function I3SFeature(parent, uri) { |
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.
Like Cesium3DTileFeature
, if this is the type that is returned when the user picks a feature, it should be marked as public.
@sanjeetsuhag thanks for the review!! we'll incorporate these review changes and let you know once that's done. |
Thanks again for your contribution @Tamrat-B! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
@sanjeetsuhag @pjcozzi just to give you an update on this (thank you @cesium-concierge for the gentle reminder :)), we are working on addressing the changes this review is asking for. Some of the asks such as changing it to primitive type, switching to use TaskProcessor, extending dracoloader, maximizing cesium3DTilesetCache are fairly involved and would require significant changes. We are allocating resources to make these changes and will share once that's done (fairly complete) for further review and eventually landing the pr. |
…s well as layer urls
@sanjeetsuhag @lilleyse @pjcozzi Please find changes based on the code review. We have addressed all the requested changes including the architectural changes. Here is the summary
|
@Tamrat-B thanks for the update and detailed summary. I'm planning on reviewing in the next week or so. |
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'm still making my way through the code but I wanted to leave some early feedback, mostly on the sandcastles and I3SDataProvider. For minor changes I committed directly.
Some of the bigger issues that stand out are traversal / culling bugs that I noticed while testing the sandcastles and lack of unit tests. Generally the architecture looks good but I still need to finish reviewing to get a complete picture.
@lilleyse we have addressed most of the feedbacks of the review. Here is a summary of the changes: The i3s unit test coverage is generally above 93%. There is still a culling bug where some of the tiles disappear at certain angles but the traversal issues seem generally better now. |
@Tamrat-B I pushed some changes:
I still would like to change the Otherwise I think this is ready. Since I made a lot of changes, if you could re-test other I3S datasets that are not in the sandcastle that would be helpful, to make sure I didn't introduce any regressions. |
This should be good to merge now. I still want to think about the |
@lilleyse @sanjeetsuhag @pjcozzi Thanks a lot folks! This should really make a lot of users happy! Thanks again for the rigorous review and making this contribution much stronger. We'll work on maintaining and improving this together. Special thanks for Anthony Mirabeau for his contribution. |
Esri Contribution: Adds support for I3S 3D Object and IntegratedMesh Layers in Cesium.
Co-authored-by: Alexandre Jean-Claude [email protected]
Co-authored-by: Elizabeth Rudkin [email protected]
Co-authored-by: Anthony Mirabeau [email protected]
Co-authored-by: Tamrat Belayneh [email protected]