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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

TinyGltfImporter: backport test changes and minor fixes #106

Closed
wants to merge 24 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Sep 26, 2021

馃憢 As announced, here are the changes to TinyGltfImporter tests for the upcoming CgltfImporter plugin. Mostly these are about moving out-of-bounds indices into their own files because cgltf checks indices at parsing time. This makes things a bit messier, but it lets us share test files between the two importers as much as possible. I was debating not checking this for cgltf at all, but it would probably be nice to have some tests for the core library, if at least to catch regressions.

The other part are some additional tests I needed for CgltfImporter but thought would be nice to test here, too (even if it just means a FAIL because it's not handled). This includes:

  • JSON string decoding (handled by tinygltf)
  • URI decoding (not handled)
  • duplicate and unordered attributes (handled by tinygltf)
  • required but unsupported extensions (not handled)
  • cyclic hierarchies in scenes (not handled)
  • missing target node in animations (handled by tinygltf, animation is skipped)
  • invalid camera type (handled by tinygltf)

There's also a small bug fix and some tiny doc changes in there, check out the respective commits for more info.

This explodes when optimizeQuaternionShortestPath is enabled. Fixed in the next commit.
JSON strings (including keys) are decoded, URIs are not
Needed by importers loading buffers on demand, and not at import time. The idea here is to avoid creating new tests in other importers supporting glTF (the upcoming CgltfImporter, possibly AssimpImporter).
鈥tandard types

These attributes are not supported by the core glTF spec. Separate them out, so other importers can test the supported ones only. The idea here is to avoid creating new tests in other importers supporting glTF (the upcoming CgltfImporter, possibly AssimpImporter).
Unlike TinyGltfImporter, some importers may perform index-checking in openData() and would fail to import the *-invalid.gltf fails altogether. Put each OOB check into their own file so this can be tested for each case. The idea here is to avoid creating new tests in other importers supporting glTF (the upcoming CgltfImporter, possibly AssimpImporter).
Moved out of lightInvalid() to avoid errors in light-invalid.gltf in importers performing array size checks in doOpenData(). The idea here is to avoid creating new tests in other importers supporting glTF (the upcoming CgltfImporter, possibly AssimpImporter).
@codecov
Copy link

codecov bot commented Sep 26, 2021

Codecov Report

Merging #106 (916d5a0) into master (0e0f8e9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   94.85%   94.86%           
=======================================
  Files         112      112           
  Lines        8857     8872   +15     
=======================================
+ Hits         8401     8416   +15     
  Misses        456      456           
Impacted Files Coverage 螖
.../MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h 100.00% <酶> (酶)
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 97.52% <100.00%> (+0.03%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 0e0f8e9...916d5a0. Read the comment docs.

鈥 views

The former is not supported and the latter spits out a confusing error message about buffer -1 being out of bounds. Detect both and print helpful error messages.
Tinygltf doesn't check this for us and it triggers CORRADE_INTERNAL_ASSERT_UNREACHABLE in doTexture(). Fixed in the next commit.
@pezcode
Copy link
Contributor Author

pezcode commented Sep 27, 2021

Just finished the last bit of testing for CgltfImporter and added a few more changes, but this should be it 馃憖

I found a bug and a half that I thought should be fixed. If you want them in another PR to keep this clean, let me know.

@mosra mosra added this to the 2021.0a milestone Sep 28, 2021
@mosra mosra added this to TODO in Asset management via automation Sep 28, 2021
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First-class work again, thank you. I went commit-by-commit (excellent job splitting those up, btw) and everything makes sense. Just some minor questions, but it's mostly ready to get merged :)

thought would be nice to test here, too

Definitely, being aware of the plugin shortcomings is a good thing. Right now I'd put the main effort into clgtf, but I can imagine the new checks could eventually get "backported" to the tinygltf importer as well.

If you want them in another PR to keep this clean

No need, the PR is just a transport medium, what matters in the long run is the commits.

CORRADE_COMPARE(importer->object3DName(1), "UTF-8: 袥芯褉械屑 懈锌褋褍屑 写芯谢芯褉 褋懈褌 邪屑械褌");
CORRADE_COMPARE(importer->object3DName(2), "UTF-8 escaped: 袥芯褉械屑 懈锌褋褍屑 写芯谢芯褉 褋懈褌 邪屑械褌");
CORRADE_COMPARE(importer->object3DName(3), "Special: \"/\\\b\f\r\n\t");
CORRADE_COMPARE(importer->object3DName(4), "Everything: 艡铆膷n铆 膷lun \t\t\n 丨賱賷亘 丕賱賱賵夭");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha :D Funny hidden gems here.

@@ -510,6 +512,8 @@ TinyGltfImporterTest::TinyGltfImporterTest() {
&TinyGltfImporterTest::meshColors,
&TinyGltfImporterTest::meshCustomAttributes,
&TinyGltfImporterTest::meshCustomAttributesNoFileOpened,
&TinyGltfImporterTest::meshDuplicateAttributes,
&TinyGltfImporterTest::meshUnorderedAttributes,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, didn't realize this needs to be handled explicitly for cgltf.

If I remember correctly, I briefly considered checking such cases here as well, but given that json.hpp exposes JSON dicts as STL maps (I think?), the non-duplicity and order was implied by that already so I didn't bother. But with cgltf needing an explicit handling it's good to have a check to ensure consistency between the two importers 馃憤

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cgltf doesn't really do... much, besides parsing. Makes it much easier to reason about its inner workings and doesn't bog everyone down in STL madness for things they don't need. It's refreshing to work with even if it means some extra work 馃憣

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I thought jsmn would sort JSON dicts, but on a second thought it's a non-essential feature, it can be easily done on application side.

@@ -446,6 +449,9 @@ TinyGltfImporterTest::TinyGltfImporterTest() {
&TinyGltfImporterTest::openExternalDataWrongSize},
Containers::arraySize(SingleFileData));

addTests({&TinyGltfImporterTest::requiredExtensions,
&TinyGltfImporterTest::requiredExtensionsUnsupported});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrator: it is not handled

Weak alibi -- historically the plugin was very basic, didn't even have a support for interleaved buffers (!), so checking the extensionsUsed/extensionsRequired fields was a very distant wish. But now that it has a mostly complete support of the core glTF spec and several extensions, implementing those here as well would be a good thing, yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I just have a list of supported extensions to check against in CgltfImporter. There might be some extra considerations for handling fallback paths (e.g. the texture extensions), but that's probably a discussion for another PR.

"extensionsRequired": [
"KHR_materials_pbrSpecularGlossiness",
"EXT_lights_image_based"
]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glTF Validator tells me that extensionsRequired without the corresponding extensions also in extensionsUsed is invalid. Should we bother checking that in any way? In my opinion what you have here is fine, the meaning is clear, but I wonder if this could lead to some behavioral differences compared to other importers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say the (non-invalid) test files should definitely pass the validator 馃憖 I'll fix that.

As for checking that in the importer... I wouldn't bother. We could error out, but once the file is corrected, the import behaviour is the same. It kind of depends on how spec-conformant you want the importer to be and how much validation you want to perform. There are a few cases already where the importer allows 'invalid' (per the spec) files because it really makes no sense to enforce those.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Yep, it already allows scene-less files and other things not allowed by spec, so why not this as well.

{
"attributes": {
"_NEGATIVE_PADDING": 4,
"NOT_AN_IDENTITY": 5,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the lack of a leading underscore have any meaning or is it just random? Like, cgltf only ever parsing custom attributes that do begin with an underscore? Or, in other words, if the "standard type" section above would have one of them with a leading underscore, would cgltf die on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically custom attributes without underscore are not allowed, but neither tinygltf nor cgltf care. In CgltfImporter I added a warning "unknown attribute NOT_AN_IDENTITY, importing as custom attribute". But yeah, it could as well go into the "standard type" mesh if it wasn't for the double component type (5130). The warning I mentioned above happens in doOpenData so this works for testing in CgltfImporter, but I'll change it to have one attribute without underscore in "standard types" too. Good catch 馃槉

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unknown attribute NOT_AN_IDENTITY, importing as custom attribute

Okay, you have thought one step ahead again. This is great, thanks!

default: CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */
default:
Error{} << "Trade::TinyGltfImporter::texture(): invalid minFilter" << s.minFilter;
return Containers::NullOpt;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, for some reason I thought tinygltf did check this, which is why the assert. Or maybe I just looked at the enum values and because there was no "invalid" I assumed it handles this gracefully. Ah well.

Thanks for catching this!

@mosra
Copy link
Owner

mosra commented Sep 28, 2021

Squashed some of the fixup commits with the originals and merged the series as 0e0f8e9...749b970. Thanks again!

@mosra mosra closed this Sep 28, 2021
Asset management automation moved this from TODO to Done Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants