-
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
Add support for Building Scene Layer (BSL) OGC I3S standard #11678
Conversation
…r color space profile Purpose: Helper function to fix color saturation
…ibute driven filter. Added new initialization option: enableFeatureFiltering, adjustMaterialAlphaMode, applySymbology and calculateNormals
…color space profile
…tIds property - Added validation for actual and expected binary buffer size
- Fixed material color saturation by converting sRGB colors in I3S data to linear color space profile
- Added support for the generic feature attribute driven filter - Added support for I3S symbology
- Added support for I3S symbology
…extensions with the feature index data Added support for 3D objects transparency Added support for CESIUM_primitive_outline glTF extensions with the generated outlines indices Added support for flat normals calculation if vertex normals are stripped out from I3S geometry Fixed vertex color saturation by converting sRGB colors in I3S geometry to linear color space profile
PR Description This PR adds a new functionality to support an OGC Indexed 3D Scene Layer (I3S) 1.3 : Building Scene Layer (BSL) type. The I3S Building Scene Layer – Cesium Sandcastle application showcases BSL consumption of BIM data for Turanga Library in Christchurch (NZ) in CesiumJS. The PR is largely based on extending the original contribution to #9634. Find here a running version of I3S Building Scene Layer - Cesium SandCastle show case. Below are overviews detailing changes brought about by this PR on both UI and Engine level. UI Changes Overview
List of files added/modified:Apps/Sandcastle/gallery/ packages/widgets/Source/ Engine Changes OverviewHigh-level structure of changes:Scene layer initialization became dependent on the layer type ("Building", "3DObject" and "IntegratedMesh" layer types are supported). Common 3D Scene Layer initialization is not changed.I3S Layer class still represents a single user-visible layer: 3D Object or Integrated Mesh. When a Building Scene Layer is initialized, it accomplishes the following:
Transcoding changes:
About color saturation:
About feature attribute driven filter:
About transparency:
About symbology:
List of files added/modified :packages/engine/Source/ I3SField.js I3SGeometry.js I3SNode.js I3SStatistics.js I3SSymbology.js Workers/ |
Thanks @Tamrat-B! Exciting! We'll take a review pass of this before the end of the week. |
Thanks @ggetz. Looking forward to it. |
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.
@Tamrat-B I've taken an initial pass on the code and have a few questions, as well as some code suggestions based on our Coding Guide.
I noticed the demo is a bit slow. Is there anything we can do to address this? @lilleyse for advice.
Before this is merged, there a few things that should be addressed:
- Update
CHANGES.md
with a summary of the fix - Any new contributors should be added to
CONTRIBUTORS.md
@ggetz please have a look for the requested changes based on your reviews. |
@ggetz As to the question regarding performance, we believe there are two main things that affect performance: applySymbology option affecting I3S layer initialization and using a detailed terrain provider (such as ArcGIS elevation provider) seems to affect both initialization and navigation. With disabled terrain and outlines the performance looks quite good. Unfortunately, if we were to use the default CesiumTerrainProvider provider there is a severe mismatch with the Turanga model and half of it will be submerged underground and is not a good option (though performance with quantized mesh looks good). As per the open issue: #8481, it looks currently in CesiumJS the one or two globe picks per frame seems to slow down Cesium for height map based detailed terrain providers like ArcGIS. There is also some community discussions around same topic here. Note: as suggested in the work around of that issue setting Cesium.ScreenSpaceCameraController.enableCollisionDetection to false seems to help, which we can set in the sample. |
that._parseBody(dataView, offset); | ||
} | ||
this._loadPromise = load(this).catch(function (error) { | ||
console.error(error); |
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, the error should be thrown so that the developer can handle it in their app accordingly.
Can we remove the catch and console.log?
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 this one, we recommend keeping the catch. This logic handles any errors with availability or binaries for attributes. When such a condition is present (the issue is related to source I3S data not having complete information and/or unexpected buffer sizes) app developers can do nothing to fix it. Catching these errors allows working with valid attributes, considering invalid attributes as missing.
See below screenshot; the door asset would still be able to show the attributes associated with it with the error catching and logs the issue; without error catching, this asset wouldn’t be able to display the attribute pop up at all.
@ggetz updated based on latest review. Thanks. |
Thanks @Tamrat-B! I was able to sync up with @lilleyse offline. He agrees that for the scope of this PR, the performance is acceptable. I have a few comments after testing.
|
@ggetz we've resolved all the issues you brought up in the latest reviews (1-3 above). Thanks! |
Thanks @Tamrat-B! I did tweak the naming of "I3SBSL*" to "I3SBsl*" to be consistant with out naming conventions, and added an additional bullet in Assuming CI passes, this should be good to merge. |
🎉 Thanks again @Tamrat-B and team! |
Hello, I found information for a console while running the sandcastle example I3S Building Scene Layer. After further analysis, it was found that the problem was I3SDataProvider.fromUrl, and there was indeed a definition of LocationPoints in the Turanga Library. At the same time, I also found that the example I3S Building Scene Layer is an upgrade to I3S 1.3, but in the introduction of I3SDataProvider.fromUrl it says "Currently supported I3S versions are 1.6 and 1.7/1.8 (OGC I3S 1.2)". Is this problem normal, or is there another way to fix it? However, there is no negative impact on the operation of the sample at this time. |
Esri Contribution: Adds support for OGC I3S Building Scene Layer (BSL) in CesiumJS.
Co-authored-by: Anton Smirnov anton.smirnov@actionengine.com
Co-authored-by: Maxim Kuznetsov maxim.kuznetsov@actionengine.com
Co-authored-by: Tamrat Belayneh tbelayneh@esri.com
This PR adds a new functionality to support an OGC Indexed 3D Scene Layer (I3S) 1.3 : Building Scene Layer (BSL) type. The PR also adds a new I3S Building Scene Layer – Cesium Sandcastle application showcasing BSL consumption of a BIM data in CesiumJS. The PR is largely based on extending the original contribution to PR #9634 supporting I3S 3D Object and IntegratedMesh Scene Layers.