-
Notifications
You must be signed in to change notification settings - Fork 203
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
Comments
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 |
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());
} |
I think that this might be related: KhronosGroup/glTF#2031 - There are considerations to replace |
This was implemented in #350. |
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, ananyOf
withstring
should still be modeled as anenum
, 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.
The text was updated successfully, but these errors were encountered: