CesiumGS / cesium-native

Apache License 2.0
441 stars 215 forks source link

Replace glTF enum properties with primitives (int/string), and provide a class with known enum values #258

Closed javagl closed 2 years ago

javagl commented 3 years ago

The issue that was reported in https://community.cesium.com/t/problems-with-loading-local-3dtiles-models/12869/20 is caused by the JSON data of the GLB of the B3DM containing images that have the "mimeType":"image/webp".

After drilling into the code for a few hours to find the reason for the bug (ouch) I tried to figure out a solution, but I think that the deeper problem might be that the MIME type is modeled as an enum in the first place. It should just be a string.

Depending on how the decision about the output is made in the generate-gltf-classes, this may require a special handling for the image MIME type: The specification declares it as a string, https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/image.schema.json#L24 , and usually, an anyOf with string should still be modeled as an enum, but not in this case.

(Somewhat related: There are considerations to formalize the process of extending the "set of valid MIME types", see https://github.com/KhronosGroup/glTF/issues/1966#issuecomment-822571685)


EDIT: More generally, unknown enum values should not cause "unspecified parse errors", but rather say .... "unspecified enum value X found in Y" or so. Depending on the efforts, this issue might either be generalized, or a new issue may be opened for that.

kring commented 3 years ago

Based on the issue described here, and Marco's demonstration of the problem in #259, I think we should avoid using an enumeration as the type of any glTF property. Instead, we should:

We can't predict which enum properties will need custom values via an extension in the future, so better to just provide the flexibility everywhere from the start. We lose a bit of static typing, but I think it's a necessary cost.

Our glTF classes are meant to be able to represent every valid glTF, but make no attempt to guarantee that every valid instance of a Model is a valid glTF. That's the job of a validator.

baothientran commented 3 years ago

Test copied from https://github.com/CesiumGS/cesium-native/pull/259 to illustrate the issue:

TEST_CASE("Unknown MIME types are handled") {
  const std::string s = R"(
    {
        "asset" : {
            "version" : "2.0"
        },
        "images": [
            {
              "mimeType" : "image/webp"
            }
        ]
    }
  )";

  ReadModelOptions options;
  CesiumGltf::GltfReader reader;
  ModelReaderResult modelResult = reader.readModel(
      gsl::span(reinterpret_cast<const std::byte*>(s.c_str()), s.size()),
      options);

  REQUIRE(modelResult.errors.empty());
  REQUIRE(modelResult.model.has_value());
}
javagl commented 3 years ago

I think that this might be related: https://github.com/KhronosGroup/glTF/pull/2031 - There are considerations to replace enum in the schema with const. Maybe I'm misinterpreting something there, and I left a comment, hoping to better understand the reasoning behind that, but it might be necessary to update the code generator accordingly if this change is applied.

kring commented 2 years ago

This was implemented in #350.