glTF generator enum type fixes

[javagl]

This is supposed to address #258 ,

but ...

... how this relates to #307 has to be sorted out. There are several loose ends to consider, summarized quickly from the tip of my head:

• In the linked PR, the generate-gltf-classes project has been refactored significantly
• This refactored state is only available as a new project (schema-gen)
• How this new project relates to the original project (or which refactoring steps had been undertaken exactly) is not clear
• The new project was supposed to not only generate the structures and the reader classes, but also the writer classes (in the current main branch, the writer classes are written manually)
• The new project (as of the state of the PR) cannot directly re-generate the CesiumGltf classes as they are. It might be possible to solve this quickly, but not without diving deeper into the code (and thus, it would be not "quickly"...).
• Both the old and the new project are not documented. I mean, not at all. *sigh*

---

This PR is, roughly speaking, the attempt to do the "smallest modifications" to the original code generator that allow us to write code where the enums use their underlying type. The results, summarized with some examples here:

Old integer enums:

struct CESIUMGLTF_API Sampler final : public NamedObject {
  static inline constexpr const char* TypeName = "Sampler";

  /**
   * @brief Magnification filter.
   *
   * Valid values correspond to WebGL enums: `9728` (NEAREST) and `9729`
   * (LINEAR).
   */
  enum class MagFilter {
    NEAREST = 9728,

    LINEAR = 9729
  };

...
  /**
   * @brief Magnification filter.
   *
   * Valid values correspond to WebGL enums: `9728` (NEAREST) and `9729`
   * (LINEAR).
   */
  std::optional<MagFilter> magFilter;

  ...

  /**
   * @brief s wrapping mode.
   *
   * S (U) wrapping mode.  All valid values correspond to WebGL enums.
   */
  WrapS wrapS = WrapS(10497);

New integer enums:

  /**
   * @brief Known values for Magnification filter.
   */
  class MagFilter {
  public:
    static const int32_t NEAREST = 9728;

    static const int32_t LINEAR = 9729;
  };

  ...

  /**
   * @brief Magnification filter.
   *
   * Known values are defined in {@link MagFilter}.
   *
   *
   * Valid values correspond to WebGL enums: `9728` (NEAREST) and `9729`
   * (LINEAR).
   */
  std::optional<int32_t> magFilter;

  ...
  
  /**
   * @brief s wrapping mode.
   *
   * Known values are defined in {@link WrapS}.
   *
   *
   * S (U) wrapping mode.  All valid values correspond to WebGL enums.
   */
  int32_t wrapS = WrapS::REPEAT;

Old string enums:

struct CESIUMGLTF_API AccessorSpec : public NamedObject {
  static inline constexpr const char* TypeName = "Accessor";
...
  /**
   * @brief Specifies if the attribute is a scalar, vector, or matrix.
   */
  enum class Type {
    SCALAR,

    VEC2,

    VEC3,

    VEC4,

    MAT2,

    MAT3,

    MAT4
  };

  /**
   * @brief Specifies if the attribute is a scalar, vector, or matrix.
   */
  Type type = Type();
}

New string enums:

struct CESIUMGLTF_API AccessorSpec : public NamedObject {
  static inline constexpr const char* TypeName = "Accessor";
  ...
  
  /**
   * @brief Known values for Specifies if the attribute is a scalar, vector, or
   * matrix.
   */
  class Type {
  public:
    inline static const std::string SCALAR = "SCALAR";

    inline static const std::string VEC2 = "VEC2";

    inline static const std::string VEC3 = "VEC3";

    inline static const std::string VEC4 = "VEC4";

    inline static const std::string MAT2 = "MAT2";

    inline static const std::string MAT3 = "MAT3";

    inline static const std::string MAT4 = "MAT4";
  };

   ...
   /**
   * @brief Specifies if the attribute is a scalar, vector, or matrix.
   *
   * Known values are defined in {@link Type}.
   *
   */
  std::string type = std::string();

---

The code for the generated Readers is updated accordingly. Some places in the not-auto-generated classes will have to be updated manually - mostly switch statements, but a few others. Unfortunately, the code for the current Writers is also not auto-generated, and this may require a bunch of tedious updates that are supposed to be ... overwritten by the auto-generated writers soon. But if this is desired (depending on the priorities), I can tackle that.

---

Finally... I have to link to Lava Flow here.

[kring]

This looks good to me. Just a couple of nitpicky comments.

[kring]

This is pretty nitpicky, but let's use a struct rather than a class with all public members. Consistent with the other glTF types.

[kring]

This would be slightly better as a constexpr I think.

[kring]

I think this can't be constexpr in C++17, but if it can be it should be too.

[javagl]

I have updated the generator to use struct and constexpr, as suggested. Based on the resulting state of the generator, the following commits are

• Update of the resulting auto-generated classes
• Updating the non-auto-generated classes accordingly
• Removal of the magic_enum dependency (it was only used in CesiumGltfWriter for the enums, and is no longer required)
• Fixes for failing tests...

---

The update of the non-auto-generated classes was less of a hassle than I thought. The mimeTypeToMimeString function could be removed, and I hope/think that the update for mimeTypeToExtensionString makes sense.

---

There's one question about error handling:

Files like Accessor.h contained functions like

int8_t computeNumberOfComponents(CesiumGltf::Accessor::Type type);

which are now a

static int8_t computeNumberOfComponents(const std::string& type);

which means that the error handling becomes more important. These should at least print an error message (as marked with a few TODO Print a warning here markers).

Right now, spdlog is a dependency of Cesium3DTilesSelection, with the spdlog-cesium.h convenience wrapper.

In order to use spdlog in CesiumGltf as well, one could

• add spdlog as a dependency to CesiumGltf, and add a similar convenience header there
• move that header to CesiumUtility, and use it from there (most other projects, particularly Cesium3DTilesSelection and CesiumGltf, already depend on CesiumUtility)

I'm slightly leaning towards the latter. One concern is that it makes spdlog an implicit part of the public(!) API of CesiumUtility. But ... what is the "public API of CesiumUtility" anyhow? So this might not matter so much here. If there are any preferences, just let me know...

---

Initially, the tests failed, because there are some assumptions about the initial values for the enum members. For example, the AccessorSpec::type was initialized to be an empty string (because it is optional (because it does not have a default value)). But the tests assumed that it had the value Type::SCALAR if it was not initialized. This was fixed in the subsequent commits: Now each enum member is initialized with its default value (if that was given), or with "the first" enum value otherwise.

---

I was a bit confused about the role of the files in the generated directory. There had been some .cpp files that apparently didn't belong there, and they have been removed in 0f6645f (if this is an issue, I'll have another look what happened there...).

[lilleyse]

I opened a follow up PR that brings us up-to-date with the latest glTF schema: #351

I was a bit confused about the role of the files in the generated directory. There had been some .cpp files that apparently didn't belong there, and they have been removed in 0f6645f (if this is an issue, I'll have another look what happened there...).

I was also confused by this.

[kring]

I don't think CesiumGltf has any business logging, especially not in such a low-level function. Documenting that it returns 0 for an unknown type is sufficient. Then the caller can assert, throw an exception, or log as appropriate.

[kring]

This looks good to me, let's get it merged for our release tomorrow.

[javagl]

Before this becomes part of a release, it should be tested more thoroughly.

I have re-introduced the unit test that I had created in #259 to illustrate the issue. This one passes now.

But... this only means that it no longer bails out with an "Unspecified parse error".

Further tests will be necessary. Particularly: If there is a B3DM with a glTF with a image/webp MIME type, then it has to be decided where and how to handle the fact that we currently cannot decode WebP images. (Some relevant discussion on a low technical level is in KhronosGroup/glTF#1966 (comment) , but how to actually detect and present that error has to be decided for our implementation).

Unfortunately, we don't have a tileset that uses WebP images. The closest that we have was a single B3DM that was provided in the forum thread https://community.cesium.com/t/problems-with-loading-local-3dtiles-models/12869/20 where this issue came up. As a quick test, I just took https://github.com/CesiumGS/cesium/tree/main/Specs/Data/Cesium3DTiles/Batched/BatchedTextured , changed the jpeg to webp with a hex editor, and loaded that in CesiumForUnreal. It does no longer print the parse error, but ... it does not print an error at all: Apparently, the mimeType is ignored in the loader part, underlying stb image loader detects the actual image data (which is still JPEG) and processes it. (Well, it does not display anything ... but it also doesn't display anything for the unmodified test data, so sorting this out is a different story...).

(Edit: The crucial point is: We have to test whether the whole thing might crash if it actually encounters real, binary WebP data!)

In order to have a real-world test, we need a tileset that actually contains a B3DM with a glTF with a WebP image. And preferably, with the same complexity as the BatchedTextured test, and not as some random, non-open, 200MB tileset that some user provided. I can try to create such tileset during the weekend, add it to the https://github.com/CesiumGS/cesium-native/tree/main/CesiumGltfReader/test/data , and create an actual test. (That test would pass iff it does not crash despite the WebP data (but prints a warning)).

And... sure, I'd fix the merge conflicts...

[kring]

I tried the Modern Office (WebP) tileset in the cesiumlab ion account. It prints messages like this to the console:

LogCesium: Warning: Texture source index must be non-negative and less than 1, but is -1

But it otherwise loads and displays fine (no textures of course):

Before this PR, we got messages like this:

LogCesium: Error: [2021-10-01 12:18:35.240] [error] [GltfContent.cpp:44] Failed to load binary glTF from https://assets.cesium.com/17743/0material54_0.b3dm?v=4:
- JSON parsing error at byte offset 1491: Parsing was terminated.

And the model didn't display at all. So we could definitely do better on the warning message, but this still a major improvement.

[kring]

@javagl I'm going to merge this without waiting for a proper test. But if and when you'd like to do that (it's certainly a good idea!) you should be able to get a good test tileset by uploading a simple textured glTF to ion and asking it to produce WebP textures: