3d tiles writer #306

krupkad · 2021-08-11T12:24:02Z

Adding a writer for 3D Tiles.

Added ExtensionWriterContext (similar to Extract extension handling #299) and tests for it under CesiumJsonWriter
Added the writers themselves under Cesium3DTilesWriter
- All autogenerated (see Add refactored/revamped JSON schema codegen #307)
- Includes tests

kring · 2021-08-11T12:39:16Z

Thanks @krupkad! What's doing the code generation? The comments say it's generate-gltf-classes, but I don't see any changes to that in this PR.

krupkad · 2021-08-11T12:46:04Z

@kring Just updated the description, there's a branch/PR in #307 with that which I separated because it's extensive.

baothientran

Code generation looks really nice! I just have a couple of comments below.

baothientran · 2021-08-18T15:41:40Z

Cesium3DTiles/include/Cesium3DTiles/Cesium3DTiles.h

+
+  std::optional<std::vector<double>> region;
+
+  std::optional<std::vector<double>> sphere;


Nitpick from me, it would be nice if this one can be a variant type. Maybe save a little bit of memory. Also it would be nice if we have a Box type that defines the center and half-axis, rather than vector<double>. Similar to Region and Sphere. If it's not possible for the generator, I wouldn't worry about it

Is it possible to use a hybrid approach here? For example, we can manually define Box, Sphere, and Region C++ type and all the utility functions. Then, we tell the generator to use those defined type, rather than create it's own. I haven't tried it, so I'm not sure it's worth the effort

Should be possible, but I do think the generated code should be as schema-faithful as possible with minimum "customization", with any custom logic done through other means e.g. inheritance, non-member non-friend utility functions. So maybe in this case we provide e.g. a non-member non-friend BoundingVolume createBox(const glm::dvec3& center, const glm::dvec3& halfSize); in include/Utils.h and src/Utils.cpp.

Related discussion in #362

baothientran · 2021-08-18T15:51:17Z

Cesium3DTiles/include/Cesium3DTiles/Cesium3DTiles.h

+  struct CODEGEN_API Properties : public CesiumUtility::ExtensibleObject {
+    static inline constexpr const char* TypeName = "Properties";
+
+    std::unordered_map<std::string, TilesetProperties> additionalProperties;


using unordered_map maybe slower than map when there are very few item

baothientran · 2021-08-18T16:06:28Z

Cesium3DTiles/include/Cesium3DTiles/Cesium3DTiles.h

+  struct CODEGEN_API GlobalPropertyScalar {
+    static inline constexpr const char* TypeName = "GlobalPropertyScalar";
+
+    struct CODEGEN_API Variant0 : public CesiumUtility::ExtensibleObject {


I kinda know why we use Variant0 naming here since schema doesn't define the name for it. But it still not a good word for API alone

Agreed but unsure how best to address this - looks like it would be fairly not-easy to override this in generator config

baothientran · 2021-08-18T16:10:00Z

Cesium3DTiles/include/Cesium3DTiles/Cesium3DTiles.h

+    std::optional<std::vector<double>> v1;
+    std::optional<double> v2;
+  };
+  struct CODEGEN_API GlobalPropertyCartesian3 {


Nitpick from me, but we should have space between structs

baothientran

That's all I have for the generated code in this round. The only comment I have is that some of the construct in the 3D tiles spec requires one of the several properties to appear at most once (e.g Bounding Volume requires to be a bounding sphere or Box, etc), should we use std::variant in those cases?

baothientran · 2021-08-20T15:38:53Z

Cesium3DTiles/include/Cesium3DTiles/Cesium3DTiles.h

+      DOUBLE
+    };
+
+    double byteOffset;


CesiumGltf has int64 for byteOffset. Can the generator do the same?

Issue is that the 3D Tiles schema has number for this, which does properly correspond to a double. Options are

change the schema to integer like glTF, which is technically a non-standard type (but the generator does handle)

add logic for handling this special case, which feels "incorrect"

We should change the spec. integer is standard in the version of JSON Schema 3D Tiles and glTF use: https://datatracker.ietf.org/doc/html/draft-zyp-json-schema-04

Opened CesiumGS/3d-tiles#476.

baothientran · 2021-08-20T16:19:27Z

Cesium3DTiles/include/Cesium3DTiles/Cesium3DTiles.h

+
+    std::optional<Variant0> v0;
+    std::optional<std::vector<double>> v1;
+    std::optional<double> v2;


This may have the same problem with the bounding volume above. The schema specifies GlobalPropertyScalar can only accept one of the properties above. Should using std::variant be a better choice in this case? cc @kring for more opinions

Did consider this, my concern was that the reader becomes much more complicated since it has to "read every alternative at the same time", so

some template/STL gymnastics is required to get each type instantiated as a read destination

there's no obvious choice when multiple alternatives are valid other than "first in the specified order", which also requires some STL mess to accomplish

Not opposed to doing it that way, just will take a little more work and be a little STL-heavy.

The "multiple optionalsapproach also covers the semantics of bothoneOfandanyOf`, granted that's not relevant for glTF/3DTiles

Without necessarily understanding the full implications, std::variant sounds right to me. It more closely matches the schema semantics and will be more memory efficient as well.

krupkad · 2021-08-25T15:26:52Z

@baothientran @kring the schema has been changed so the byteOffsets are ints, and the writer now uses std::variant.

Upstream changed GlobalPropertyScalar to GlobalPropertyInteger and GlobalPropertyNumber.

cesium-concierge · 2021-10-09T02:00:02Z

Thanks again for your contribution @krupkad!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 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.

lilleyse · 2021-10-12T18:34:37Z

I'll refer back to this PR as I work on GltfWriter and Cesium3DTilesWriter but I think it can be closed.

krupkad added 4 commits August 11, 2021 08:08

Add 3D Tiles writer

85f122c

Make Cesium3DTiles an interface library since it is header-only

1763a24

Refactor CesiumJsonWriter/Cesium3DTilesWriter tests

85e1f20

Update README with 3D Tiles libraries

7bbf270

krupkad requested review from kring and baothientran August 11, 2021 12:24

krupkad mentioned this pull request Aug 11, 2021

Add refactored/revamped JSON schema codegen #307

Closed

krupkad mentioned this pull request Aug 11, 2021

3D Tiles Serializer #278

Closed

Support feature tables in 3D Tiles serialization

f81d623

krupkad force-pushed the 3d-tiles-writer branch from 52cd0d4 to 997bd47 Compare August 17, 2021 13:39

Fix missing Cesium3DTilesWriter install

b63e67b

krupkad force-pushed the 3d-tiles-writer branch from 997bd47 to b63e67b Compare August 17, 2021 13:42

Add 3D Tiles writer context

3c1b085

baothientran reviewed Aug 18, 2021

View reviewed changes

krupkad added 2 commits August 19, 2021 08:09

Fix linkage and header visibility of Json and 3D Tiles writers

f95391b

Fix linkage and header visibility of Json and 3D Tiles writers

cf7962a

baothientran reviewed Aug 20, 2021

View reviewed changes

Use std::variant based 3D Tiles writer

f1023ee

krupkad requested a review from baothientran August 25, 2021 15:15

krupkad and others added 5 commits August 26, 2021 08:50

Remove std::nullopt initialization in 3D Tiles writer

f90b027

Remove std::nullopt initialization in 3D Tiles writer

b35900d

Write 3D Tiles to string instead of stream for exception-safety

98bcf87

Update 3D Tiles schema-generated code.

6367c60

Upstream changed GlobalPropertyScalar to GlobalPropertyInteger and GlobalPropertyNumber.

Merge remote-tracking branch 'origin/main' into 3d-tiles-writer

3f863bd

lilleyse closed this Oct 12, 2021

lilleyse mentioned this pull request Dec 7, 2021

3D Tiles Writer #412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3d tiles writer #306

3d tiles writer #306

krupkad commented Aug 11, 2021 •

edited

Loading

kring commented Aug 11, 2021

krupkad commented Aug 11, 2021

baothientran left a comment •

edited

Loading

baothientran Aug 18, 2021

baothientran Aug 18, 2021

krupkad Aug 24, 2021

lilleyse Oct 12, 2021

baothientran Aug 18, 2021

baothientran Aug 18, 2021

krupkad Aug 24, 2021

baothientran Aug 18, 2021

baothientran left a comment

baothientran Aug 20, 2021

krupkad Aug 24, 2021

kring Aug 25, 2021

krupkad Aug 25, 2021

baothientran Aug 20, 2021

krupkad Aug 24, 2021

kring Aug 25, 2021

krupkad commented Aug 25, 2021

cesium-concierge commented Oct 9, 2021

lilleyse commented Oct 12, 2021


		std::optional<std::vector<double>> region;

		std::optional<std::vector<double>> sphere;

3d tiles writer #306

3d tiles writer #306

Conversation

krupkad commented Aug 11, 2021 • edited Loading

kring commented Aug 11, 2021

krupkad commented Aug 11, 2021

baothientran left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baothientran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krupkad commented Aug 25, 2021

cesium-concierge commented Oct 9, 2021

lilleyse commented Oct 12, 2021

krupkad commented Aug 11, 2021 •

edited

Loading

baothientran left a comment •

edited

Loading