Skip to content

Tinygltf importer fixes #41

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

Merged
merged 4 commits into from
May 10, 2018
Merged

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Apr 29, 2018

Hi @mosra !

As discussed, here are some fixes to the TinyGltfImporter.

Cheers, Jonathan.

TODO

  • UVs import
  • Colors import
  • Cleanup test

@mosra mosra added this to the 2018.0b milestone Apr 29, 2018
@Squareys Squareys force-pushed the tinygltf-importer-fixes branch 2 times, most recently from 81f12bd to 9470d29 Compare April 30, 2018 05:01
@mosra
Copy link
Owner

mosra commented May 1, 2018

Hey, Gitter is giving me 504s at the moment so I can't use it -- I still didn't tag 2018.04 (saw no point in rushing it yesterday) so if you have time today, it'd be great to have it in. 👍 (I'm merging mosra/magnum#233 soon, though.)

@mosra mosra modified the milestones: 2018.04, 2018.0c May 1, 2018
Squareys added 2 commits May 5, 2018 10:00
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the tinygltf-importer-fixes branch 2 times, most recently from 65037f9 to 6d6707c Compare May 7, 2018 21:58
@codecov-io
Copy link

codecov-io commented May 7, 2018

Codecov Report

Merging #41 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   87.19%   87.15%   -0.04%     
==========================================
  Files          41       39       -2     
  Lines        3866     3193     -673     
==========================================
- Hits         3371     2783     -588     
+ Misses        495      410      -85
Impacted Files Coverage Δ
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 69.79% <0%> (-13.55%) ⬇️
...numPlugins/StbImageConverter/StbImageConverter.cpp 73.84% <0%> (-0.44%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
src/Magnum/OpenDdl/Structure.h 96.55% <0%> (-0.06%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
.../MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h
src/MagnumPlugins/PngImporter/PngImporter.cpp 69.87% <0%> (+0.11%) ⬆️
...numPlugins/PngImageConverter/PngImageConverter.cpp 69.11% <0%> (+0.54%) ⬆️
...agnumPlugins/StanfordImporter/StanfordImporter.cpp 94.3% <0%> (+0.55%) ⬆️
src/Magnum/OpenDdl/Implementation/Parsers.cpp 88.03% <0%> (+0.56%) ⬆️
... and 10 more

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 e0d9030...53bbce2. Read the comment docs.

@Squareys Squareys changed the title [WIP] Tinygltf importer fixes Tinygltf importer fixes May 7, 2018
@Squareys Squareys force-pushed the tinygltf-importer-fixes branch from 6d6707c to dcd5cce Compare May 7, 2018 22:27
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.

Some generally minor things. Other than that, 👌

CORRADE_COMPARE(texture->minificationFilter(), Sampler::Filter::Nearest);
CORRADE_COMPARE(texture->mipmapFilter(), Sampler::Mipmap::Linear);

//CORRADE_COMPARE(texture->wrapping(), Array3D<Sampler::Wrapping>(Sampler::Wrapping::Repeat, Sampler::Wrapping::Repeat, Sampler::Wrapping::Repeat));
Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice to have this tested, ideally with different values for X and Y :)

if(normalAccessor.type != TINYGLTF_TYPE_VEC3) {
Error() << "Trade::TinyGltfImporter::mesh3D(): expected type of normal is VEC3";
return Containers::NullOpt;
if (attribute.first.compare("POSITION") == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

why not just == "POSITION"? Similarly for NORMAL below.

std::copy_n(reinterpret_cast<const Vector3*>(buffer.data.data() + bufferView.byteOffset), numNormals, std::back_inserter(normals));

/* Texture coordinate attribute ends with _0, _1 ... */
} else if (attribute.first.compare(0, 8, "TEXCOORD") == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

You could use Utility::String::beginsWith() here ;) Similarly for colors below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's longer :P (But still more readible)

@mosra
Copy link
Owner

mosra commented May 8, 2018

Re CI failures:

  • it seems that the mesh-colors.bin file (containing the actual mesh data?) is missing.
  • the new files are also not embedded for Emscripten/Android, so it fails there completely
  • but it doesn't fail on openFile (as it should), rather quite later at unexpected data count. It looks like opening a nonexistent file is not handled correctly and openFile returns true, can you add a new test case for that (and fix it, if that's really the problem?)?
  • the failures with <?>: it seems like tinygltf is reading past the end of the file, encountering non-ascii data. That's kinda bad :/ Maybe a patch to tinygltf is needed?

@Squareys
Copy link
Contributor Author

Squareys commented May 8, 2018

(and fix it, if that's really the problem?)?

That is probably a problem in TinyGltf... I will check it, though.

That's kinda bad :/ Maybe a patch to tinygltf is needed?

I think that's because I hand edited the file... And that are the actual contents of the file btw. (binary data is appended in .glb)

Thanks for investigating, will have time to finish this up later today.

@mosra
Copy link
Owner

mosra commented May 8, 2018

That is probably a problem in TinyGltf

I don't think so, isOpened() checks for !!_d and in the current code that's true even if the actual file opening inside AbstractImporter::doOpenFile() fails. Not sure how to fix that, tho :)

I think that's because I hand edited the file

Ah I see. Probably some size information mismatch. Use some gltf-to-glb converter then, maybe?

Squareys added 2 commits May 8, 2018 19:25
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the tinygltf-importer-fixes branch from dcd5cce to 53bbce2 Compare May 8, 2018 17:26
@Squareys
Copy link
Contributor Author

Squareys commented May 8, 2018

I don't think so

You may be mixing up .bin with .glb here. The latter is the binary representation of GLTF, the former are files included in non-binary .gltf files which contain binary data and are opened by TinyGTLF.
TinyGltfImporter doesn't know the .bin extension -- and that wouldn't make sense as it is unstructured raw binary data (the metadata is contained in the .gltf file).

Apart from that, everything should be done, let's see what the CIs have to say.

@mosra mosra merged commit 53bbce2 into mosra:master May 10, 2018
@mosra
Copy link
Owner

mosra commented May 10, 2018

I tried very hard to find more things to complain about, but nope. I got nothing. 😆

Merged. Thanks a lot! 👍

@Squareys
Copy link
Contributor Author

Awesome, thanks! 🎉 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants