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

(Assimp|OpenGex)Importer: support file callbacks. #47

Closed
wants to merge 3 commits into from

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Aug 13, 2018

Hi @mosra !

As promised, here is support for file callbacks in OpenGexImporter. Assimp will follow as a separate PR also.

Cheers, Jonathan

@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #47 into master will decrease coverage by 1.32%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage    88.9%   87.57%   -1.33%     
==========================================
  Files          43       43              
  Lines        4226     3968     -258     
==========================================
- Hits         3757     3475     -282     
- Misses        469      493      +24
Impacted Files Coverage Δ
.../MagnumPlugins/OpenGexImporter/OpenGexImporter.cpp 93.54% <33.33%> (-0.11%) ⬇️
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 69.55% <50%> (-13.79%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 91.75% <0%> (-0.2%) ⬇️
src/Magnum/OpenDdl/OpenDdl.cpp 90.68% <0%> (-0.12%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
src/MagnumPlugins/DdsImporter/DdsImporter.cpp 78.15% <0%> (+0.16%) ⬆️
src/MagnumPlugins/PngImporter/PngImporter.cpp 74.69% <0%> (+0.28%) ⬆️
... 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 51eb2d4...71ea50a. Read the comment docs.

@Squareys Squareys force-pushed the ogex-callbacks branch 2 times, most recently from 5379461 to 2f6a80d Compare August 13, 2018 21:03
@Squareys Squareys changed the title OpenGexImporter: support file callbacks. (Assimp|OpenGex)Importer: support file callbacks. Aug 13, 2018
@Squareys Squareys force-pushed the ogex-callbacks branch 3 times, most recently from 1030801 to fac2bfb Compare August 13, 2018 21:22
@mosra
Copy link
Owner

mosra commented Aug 14, 2018

@Squareys for some reason all the tests fail on Travis, could you look into that, please? :)

Squareys and others added 3 commits August 14, 2018 16:09
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>

std::unordered_map<std::string, Containers::Array<char>> files;
files["not/a/path/something.ogex"] = Utility::Directory::read(Utility::Directory::join(OPENGEXIMPORTER_TEST_DIR, "texture.ogex"));
files["image.tga"] = Utility::Directory::read(Utility::Directory::join(OPENGEXIMPORTER_TEST_DIR, "image.tga"));
Copy link
Owner

Choose a reason for hiding this comment

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

Wait... this should have been not/a/path/image.tga, because image.tga is next to something.ogex and referenced as such. The same in the Assimp importer.

Basically, it's needed to save the path even when calling openFile() with a callback, so the image callbacks get a proper path. This is not a proper path.

@mosra
Copy link
Owner

mosra commented Aug 18, 2018

The OpenGexImporter support was merged with a few changes in 57f619b. I had to basically rewrite the AssimpImporter support in ea82966 in order to make it properly use callbacks also for nested data files. Besides that, I came across a few more or less critical issues in the AssimpImporter and fixed these as well.

@mosra mosra closed this Aug 18, 2018
@mosra mosra added this to the 2018.10 milestone Oct 23, 2018
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