Skip to content
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

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

Closed
javagl opened this issue May 30, 2021 · 4 comments
Labels
quality Improve code quality or encourage developer success

Comments

@javagl
Copy link
Contributor

javagl commented May 30, 2021

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
Copy link
Member

kring commented Jun 11, 2021

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:

  • Use the appropriate primitive type from the JSON Schema as the property type. e.g. image.mimeType should be declared as std::string.
  • Provide a class with static constant properties for the known enumeration values when they are defined in the JSON Schema.
  • In the generated Doxygen for the property, add a link to the generated enumeration value class.

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.

@kring kring changed the title Unknown image MIME type in glTF causes unspecified parse error Jun 11, 2021
@kring kring added the quality Improve code quality or encourage developer success label Jun 11, 2021
@baothientran
Copy link
Contributor

Test copied from #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
Copy link
Contributor Author

javagl commented Sep 23, 2021

I think that this might be related: KhronosGroup/glTF#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
Copy link
Member

kring commented Jan 14, 2022

This was implemented in #350.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality Improve code quality or encourage developer success
3 participants