Add support for point clouds (pnts)

[j9liu]

WIP, but I'm opening it now to track the tasks involved. This builds off of #334 and will finally close #333 when finished.

TODO

• Implement support for all point cloud semantics
  • Positions are done (though values being properly stored in buffers still untested)
  • Colors
    • need to convert the different RGB formats into vertex colors appropriately
    • need to convert CONSTANT_RGBA to a PBR material
  • Normals
    • need to decode oct16_p normals
  • Batch IDs
    • need to convert into feature IDs
• Convert batch table to glTF feature metadata extension
• Support Draco extension
• Unit tests

[j9liu]

Happy to announce this is ready for review whenever you get a chance @kring !

[timoore]

I tried this branch with the Melbourne Point Cloud and vsgCs and triggered an assertion:

worldviewer: /home/moore/graphics/cesium-native/extern/rapidjson/include/rapidjson/document.h:1253: rapidjson::GenericValue<Encoding, Allocator>::ConstMemberIterator rapidjson::GenericValue<Encoding, Allocator>::MemberBegin() const [with Encoding = rapidjson::UTF8<>; Allocator = rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator>; ConstMemberIterator = rapidjson::GenericMemberIterator<true, rapidjson::UTF8<>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >]: Assertion `IsObject()' failed.

The problem is that the batch table in this tile is of length 0.
Stack trace:

#5  0x0000000000914775 in rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >::MemberBegin (this=0x7fffdbedb510) at /home/moore/graphics/cesium-native/extern/rapidjson/include/rapidjson/document.h:1253
#6  0x00000000008ec870 in Cesium3DTilesSelection::(anonymous namespace)::convertBatchTableToGltfFeatureMetadataExtension (
    batchTableJson=..., batchTableBinaryData=..., gltf=..., featureCount=41214, result=...)
    at /home/moore/graphics/cesium-native/Cesium3DTilesSelection/src/BatchTableToGltfFeatureMetadata.cpp:1412
#7  0x00000000008ed4ee in Cesium3DTilesSelection::BatchTableToGltfFeatureMetadata::convertFromPnts (featureTableJson=..., 
    batchTableJson=..., batchTableBinaryData=..., gltf=...)
    at /home/moore/graphics/cesium-native/Cesium3DTilesSelection/src/BatchTableToGltfFeatureMetadata.cpp:1592
#8  0x00000000009876e8 in Cesium3DTilesSelection::(anonymous namespace)::convertPntsContentToGltf (pntsBinary=..., header=..., 
    headerLength=28, result=...) at /home/moore/graphics/cesium-native/Cesium3DTilesSelection/src/PntsToGltfConverter.cpp:1556
#9  0x0000000000987827 in Cesium3DTilesSelection::PntsToGltfConverter::convert (pntsBinary=...)
    at /home/moore/graphics/cesium-native/Cesium3DTilesSelection/src/PntsToGltfConverter.cpp:1576
#10 0x00000000008d9091 in operator() (__closure=0x1e63880, pCompletedRequest=...)
    at /home/moore/graphics/cesium-native/Cesium3DTilesSelection/src/TilesetJsonLoader.cpp:821

The tileUrl is "https://assets.ion.cesium.com/43978/0/0.pnts?v=1".

[j9liu]

Thanks for the catch @timoore. I just pushed a commit to fix it, but let me know if the issue persists.

[timoore]

Thanks for the catch @timoore. I just pushed a commit to fix it, but let me know if the issue persists.

Unfortunately the MemberCount() call fails with the same problem. My stab at fixing the issue is timoore@6fbffd2 .

With that, I can see points in Melbourne. I don't know if vsgCs is doing the right thing with the lighting and point rendering.

[kring]

Wow, nice work here @j9liu! This looks very well implemented and tested. I only have minor comments below. Please take a look at @timoore's latest comment as well, of course.

[kring]

Can these two checkBufferContents functions be merged into one, with a default value for the epsilon parameter, and the epsilon value is ignored for integer comparisons? Seems like it would eliminate a lot of duplication.

[j9liu]

When I do this, Windows + VS2017 and Linux + GCC are throwing compile errors in Travis because the epsilon parameter isn't used in all branches of the function. Do you know how to address that without code duplication?

[kring]

You can add an attribute to the parameter:
https://en.cppreference.com/w/cpp/language/attributes/maybe_unused
You can also use specialization rather than if constexpr, but that's a bigger change.

[j9liu]

@timoore thanks for letting me know your fix. I moved the IsObject check to PntsToGltfConverter so that it doesn't even try to invoke BatchTableToGltfFeatureMetadata if batchTableJson is invalid.

With that, I can see points in Melbourne. I don't know if vsgCs is doing the right thing with the lighting and point rendering.

It seems like the geometry is there but the colors are missing. Or they may just be faded. I have to add a correction that converts the pnts sRGB colors to linear colors for the glTF, as pointed out in CesiumGS/cesium-unity#197.

[j9liu]

Also @timoore, could you sign a CLA here since you found a fix for the rapidjson issue? The CLA forms and relevant info can be found here.

[timoore]

It seems like the geometry is there but the colors are missing. Or they may just be faded. I have to add a correction that converts the pnts sRGB colors to linear colors for the glTF, as pointed out in CesiumGS/cesium-unity#197.

I had a bug where I wasn't doing anything with normals for points, so they were only being lit by ambient lighting. They are looking better now. That's interesting about sRGB colors in points. I followed the comment chain from cesium-unity to cesium. I don't see in the point clouds spec where that's specified, but obviously I would take @lilleyse 's word for it!

[timoore]

Also @timoore, could you sign a CLA here since you found a fix for the rapidjson issue? The CLA forms and relevant info can be found here.

Done.

[kring]

Thanks @j9liu!