Skip to content
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

Merged
merged 66 commits into from
Oct 17, 2022
Merged

Added I3S data source support in Cesium #9634

merged 66 commits into from
Oct 17, 2022

Conversation

Tamrat-B
Copy link
Contributor

Esri Contribution: Adds support for I3S 3D Object and IntegratedMesh Layers in Cesium.
Co-authored-by: Alexandre Jean-Claude ajeanclaude@spiria.com
Co-authored-by: Elizabeth Rudkin elizabeth.rudkin@presagis.com
Co-authored-by: Anthony Mirabeau anthony.mirabeau@presagis.com
Co-authored-by: Tamrat Belayneh tbelayneh@esri.com

@cesium-concierge
Copy link

Thanks for the pull request @Tamrat-B!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.
@dmfenton
Copy link

Thanks for doing this, this will be incredibly valuable for us!

@gooin
Copy link

gooin commented Jun 27, 2021

We can preview video intro of this version at https://youtu.be/0C2fvQXqODQ?t=1793

@pasu
Copy link
Contributor

pasu commented Aug 16, 2021

Hi, I have one small issue.
In I3SDataSource, it hooked Cesium3DTile.prototype.requestContent and rewrote this function by Cesium3DTile.prototype._hookedRequestContent.
It is a smart way, I always want to know how you can find this way. In my situation, if I want to use the same way to load other formats and convert to 3d tiles, there will be a conflict about Cesium3DTile.prototype.requestContent. Only one requestContent function can be working and it depends on the loading sequence.
My suggestion is that, here, we need a register or observer class to make this function virtual.
Thank you for your suggestion. Great Job! @lilleyse

- 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
@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 14, 2021

@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 main and available as an experimental API in 1.87.1. This work may impact how we would suggest to architect i3s support.

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:

  • For symmetry, I believe this should be a Primitive, not a DataSource
  • Is this reusing intrinsics within CesiumJS reasonably well when possible, e.g., any parts of 3D Tiles LOD selection, cache management, linked lists, the right rendering abstraction layer / transcoding, etc.
    • Are there new intrinsics this could contribute?
  • Is this a good foundation for additional i3s support?
  • Is the public API and implementation consistent with CesiumJS's coding conventions, unit tests, reference doc, sufficient Sandcastle examples, etc.?
  • Are there any new third-party libraries? Are they big? Is the license compatible and documented?
  • What is the maintenance implication of this moving forward and who will do it?
    • Is CesiumJS core the right home or is a separate plugin? If i3s support is well done and likely not to be a maintenance issue, CesiumJS core is preferred for the current CesiumJS architecture.

🙏 🙏 🙏

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a 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.

*
*/

function I3SDataSource(name, scene, options) {
Copy link
Contributor

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.


// Prevent ESLint from issuing warnings about undefined Promise
// eslint-disable-next-line no-undef
var _Promise = Promise;
Copy link
Contributor

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.


// Code traces
// set to true to turn on code tracing for debugging purposes
var _tracecode = false;
Copy link
Contributor

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.

*
*/

function I3SDataSource(name, scene, options) {
Copy link
Contributor

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.

if (that._traceFetches) {
console.log("I3S FETCH:", uri);
}
var request = fetch(uri);
Copy link
Contributor

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.

.########.########.########.####.##.........######...#######..####.########..##.....##.########
*/

function mercatorAngleToGeodeticLatitude(mercatorAngle) {
Copy link
Contributor

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.

.########..########..######...#######..########..########.##.....##
*/

function decodeDracoEncodedGeometry(data, bufferInfo) {
Copy link
Contributor

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.

.##....##..##.....##.##.......##.....##.##......
..#####.##..#######..########..#######..########
*/
function I3SGLTFProcessingQueue() {
Copy link
Contributor

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.

};

// Add an entry in the data source content cache
_i3sContentCache[this._completeUriWithoutQuery] = {
Copy link
Contributor

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.

* @param {I3SNode} [parent] The parent of that feature
* @param {String} [uri] The uri to load the data from
*/
function I3SFeature(parent, uri) {
Copy link
Contributor

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.

@Tamrat-B
Copy link
Contributor Author

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.

@sanjeetsuhag thanks for the review!! we'll incorporate these review changes and let you know once that's done.

@cesium-concierge
Copy link

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 @cesium-concierge stop. If you want me to start again, just delete the comment.

@Tamrat-B
Copy link
Contributor Author

@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.

@Tamrat-B
Copy link
Contributor Author

@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

  • I3S geometry payload is now transcoded to glTF (glb) and not b3dm.

  • Re-implemented what was previously a DataSource type (I3SDataSource) now as a Primitive (I3SDataProvider):
    I3SDataSource already stored Cesium3dTileset objects internally so for the class to behave as a primitive, we simply needed to add the required functions (update, prePassesUpdate, postPassesUpdate, etc.) and call these functions on the internal tilesets. With this change, an I3S dataset can be directly inserted into a scene's list of primitives and should behave transparently. The class has also been renamed to I3SDataProvider to reflect this change.

  • Provided a service based architecture avoiding bloated look up files to convert gravity related (orthometric) height systems to ellipsoidal:
    To seamlessly fuse datasets created on disparate vertical coordinate systems (VCS), we perform geoid conversion of geometric primitives and bounding boxes leveraging existing implementation based on ArcGISTiledElevationTerrainProvider (compiled from Earth Gravitational Model 2008 (EGM2008) Data).The Sandbox examples shows how to set the terrain provider service if required.

  • Worker code refactored to make use of TaskProcessor:
    Code to decode i3s geometry has been moved into a separate file (DecodeI3S.js) and will now be called by going through Cesium's TaskProcessor. This allowed us to remove a lot of redundant code by reusing existing functionality from Cesium which was inaccessible with the old worker implementation. DecodeI3S class is now responsible for generating the glTF buffer.

  • Removed i3sContentCache:
    This was a redundant caching system because tiles were already being cached by the internal Cesium3dTileset. As long as the memory budget is not exceeded, the transcoded tiles remain loaded, and no conversion needs to be repeated.

  • Support debug parameters:
    we added the ability to pass a list of Cesium3dTileset parameters to I3SDataProvider which will pass it along to the internal Cesium3dTileset. This allows the I3sDataProvider to benefit from all existing debug options for Cesium3dTileset like maximumScreenSpaceerror, debugShowBoundingVolume, etc.

  • Further refactoring of implementation
    Refactored the implementation to have distinct I3SFeature and I3SNode classes for better accessibility of public classes. I3SNode in cesium translates to a Cesium3DTile whereas I3SFeature class handles I3S Feature data.

  • Sandbox samples updated:
    To reflect API usage, 3 sandbox samples have been added. Each sample shows usage of consumption of the supported I3S layer types (3D Object (both textured and untextured) and IntegratedMesh) as well as feature attribute (metadata) selection and display.

  • Brief description of the architecture:
    When a scene layer is initialized, and I3SDataProvider.loadURL function is invoked, it processes the 3D Scene Layer of an I3S dataset to create a Cesium 3D Tile Set and loads the root node. When the root node is imported, it creates a Cesium 3D tile that is parented to the Cesium 3D tile set and loads all children of the root node.

    • for each children:
      • Create a place holder 3D tile so that the LOD display can know about it and display as needed
      • When the LOD display decides that a Cesium 3D tile is visible, it invokes requestContent on it.
      • At that moment, we intercept the call to requestContent, and we load the geometry in I3S format.
      • That geometry is then transcoded on the fly to glTF format using TaskProcessor and is ingested by Cesium.
      • When the geometry is loaded, we then load all children of this node as placeholders so that the LOD can know about them too and is able to request content on those children tiles as well as needed/triggered by the cesium LOD request mechanism.
  • Brief description about transcoding. We use web workers to transcode I3S geometries into glTF.

    • the steps are:
      • Decode geometry attributes (positions, normals, etc..) either from DRACO or Binary format.
      • If requested ( by defining a geoidTiledTerrainProvider when creating an I3SDataProvider) convert heights for all vertices & bounding boxes of input I3S layer from its gravity related height to Ellipsoidal.
      • We then transform vertex coordinates from LONG/LAT/HEIGHT to Cartesian in local space and scale appropriately if specified in the attribute metadata
      • Crop UVs if UV regions are defined in the attribute metadata
      • Create a glTF document in memory that will be ingested as part of a glb payload
@lilleyse
Copy link
Contributor

@Tamrat-B thanks for the update and detailed summary. I'm planning on reviewing in the next week or so.

Copy link
Contributor

@lilleyse lilleyse left a 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.

Source/Core/ArcGISTiledElevationTerrainProvider.js Outdated Show resolved Hide resolved
Apps/Sandcastle/gallery/I3S 3D Object Layer.html Outdated Show resolved Hide resolved
Source/Scene/I3SDataProvider.js Outdated Show resolved Hide resolved
Source/Scene/I3SDataProvider.js Outdated Show resolved Hide resolved
Source/Scene/I3SDataProvider.js Outdated Show resolved Hide resolved
Source/Scene/I3SDataProvider.js Outdated Show resolved Hide resolved
Source/Scene/I3SDataProvider.js Outdated Show resolved Hide resolved
@Tamrat-B
Copy link
Contributor Author

@lilleyse we have addressed most of the feedbacks of the review.

Here is a summary of the changes:
-Added suite of unit tests
-Split off I3Slayer into its own file to simplify unit testing
-Refactor logic for handling url and query parameters by using the Resource class
-Remove the ability to replace existing data by loading a new url. Url is passed to constructor instead
-Move camera functionality out of I3SDataProvider
-Add show property to I3SDataProvider
-Replace all instances of null with undefined
-Clean up unused code
-Add some missing documentation

The i3s unit test coverage is generally above 93%.
image

There is still a culling bug where some of the tiles disappear at certain angles but the traversal issues seem generally better now.

@lilleyse
Copy link
Contributor

lilleyse commented Oct 7, 2022

@Tamrat-B I pushed some changes:

  • Fixed the remaining culling issues by computing the position accessor min/max in decodeI3s. The min/max is used to create the draw command's bounding sphere. The previous value [0, 0, 0] was causing undefined behavior in Scene.
  • General cleanup to match CesiumJS code style: using defined in more places, simplifying promises, declaring all member variables in constructor, etc.
  • Some updates to the API. I3SDataProvider now takes a single options object similar to Cesium3DTileset. options.url is the only required option. The provider is loaded in the constructor now, so load() has been removed.
  • Exposed more I3S classes in the documentation since they are used in the sandcastles. Most of them are marked as @internalConstructor.

I still would like to change the requestContent hook approach so that I3SNode doesn't override the Cesium3DTile prototype, I just haven't landed on a good solution yet.

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.

@lilleyse
Copy link
Contributor

This should be good to merge now. I still want to think about the requestContent hook it doesn't need to hold up this PR. Thanks @Tamrat-B and everyone else for this contribution!

@lilleyse lilleyse merged commit 92a6063 into CesiumGS:main Oct 17, 2022
@Tamrat-B
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants