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

TinyGltfImporter: more backporting #109

Closed

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Oct 8, 2021

Here's the second set of changes for TinyGltfImporter.

Mostly consists of a bunch of changes backported from CgltfImporter deemed "bug/security/robustness" fixes. For slightly more background information, see #107 (comment) and #107 (comment).

This also undoes some test file splits done in #106 because CgltfImporter doesn't depend on cgltf_validate anymore so those checks don't fail in openData.

Needed for test files that have smaller/larger sizes
- scene node list must only contain root nodes
- nodes must not have multiple parents
- cycle detection
Used to be an assert and consequently untested
The MSFT_texture_dds in the test files is there so other importers (CgltfImporter, really) can share test files. Also for CgltfImporter, the invalid basisu index test needs to be separate because that's checked by cgltf at import.
and mention their existence in the docs
The accessor checks for these were previously untested
Originally moved out for CgltfImporter, but it doesn't perform these tests in openData() anymore, so they don't need to be in separate files
@pezcode pezcode mentioned this pull request Oct 8, 2021
std::isdigit requires <cctype>, apparently MSVC 2019 transitively included it somewhere already
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #109 (f74b5c7) into master (144716c) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head f74b5c7 differs from pull request most recent head 2bdc698. Consider uploading reports for the commit 2bdc698 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   94.86%   94.92%   +0.05%     
==========================================
  Files         112      112              
  Lines        8874     8977     +103     
==========================================
+ Hits         8418     8521     +103     
  Misses        456      456              
Impacted Files Coverage Δ
.../MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h 100.00% <ø> (ø)
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 97.71% <100.00%> (+0.19%) ⬆️

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 144716c...2bdc698. Read the comment docs.

@mosra mosra added this to the 2021.0a milestone Oct 11, 2021
@mosra mosra added this to TODO in Asset management via automation Oct 11, 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.

Thank you! Extra points for catching the additional cases of invalid scene hierarchies, I wouldn't even think of that.

@pezcode
Copy link
Contributor Author

pezcode commented Oct 11, 2021

cases of invalid scene hierarchies, I wouldn't even think of that.

I was wondering, should those non-cycle checks maybe be moved out of openData()? Would have to keep parentFor around in _d, but it'd avoid import failure for scenes/nodes that aren't actually imported.

It's used in other places already, be consistent and allow easier search for integer parsing
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.

should those non-cycle checks maybe be moved out of openData()?

I think this would be easier to do in the SceneData rework I'll be doing -- there it would only need to be handled in the doScene() call (which also takes care of all child objects). Here, as far as I can tell, you'd have to duplicate the handling also in doObject3D() (all of them? or only those that import the affected object? sounds complicated).

So I'd say keep it in doOpenData() and then I'll move it to doScene() during the port to new SceneData.

src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp Outdated Show resolved Hide resolved
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.

Wonderful, I'll merge as soon as the CIs look like finishing green :)

@mosra
Copy link
Owner

mosra commented Oct 11, 2021

I applied the latest fixup commits to the rest and merged as 144716c...f7b29fd. Thank you!

@mosra mosra closed this Oct 11, 2021
Asset management automation moved this from TODO to Done Oct 11, 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