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

Trade: support mip levels in image import #369

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Conversation

mosra
Copy link
Owner

@mosra mosra commented Aug 27, 2019

There's a new level option in each imageND() API, plus an imageNDLevelCount() query to get image level count. The level defaults to 0, in which case it behaves exactly like before. For levels above 0, the abstract importer checks against the reported level count and asserts if it's out of bounds.

Things left to do:

  • changelog entry
  • adapt DdsImporter, use the levels instead of reporting N images
  • adapt BasisImporter (currently it opens just the base level)
  • adapt other image importer plugins
  • adapt AssimpImporter, OpenGexImporter and TinyGltfImporter plugins
    • they need to properly propagate this to AnyImageImporter and should cache the importer instance to avoid a single image file being read multiple times for each level (and with assertions enabled, once more for the check in each imageND() call)
    • doing this in a clean way may need to have AnyImageImporter movable
    • reuse mipmap test files from DdsImporter and test level propagation on those

@Squareys since you're working on mosra/magnum-plugins#62, could you have a quick look at this to see if it fits your expectations or needs design changes? Thanks! 馃檹

@mosra mosra added this to the 2019.0b milestone Aug 27, 2019
@mosra mosra self-assigned this Aug 27, 2019
@mosra mosra added this to TODO in Asset management via automation Aug 27, 2019
@Squareys
Copy link
Contributor

At first I was concerned whether image2d(index, level) would enforce anti-patterns in the implementations, but I think quite the opposite is true, this is pretty great, actually :)

Looks good to me! 馃憤

@mosra
Copy link
Owner Author

mosra commented Aug 27, 2019

would enforce anti-patterns in the implementations

I'm interested, what exactly? Just so I can be sure that I'm doing it correctly.

@Squareys
Copy link
Contributor

Squareys commented Aug 27, 2019

what exactly?

I was thinking that this might require doing per-image work per mip level. So if you'd call:

for(int level = 0; level < maxLevel; ++level) {
    importer->image2D(0, level);
    /* this might do work maxLevel-1 times that would only need
       to be done once. E.g. figure out image metadata etc. */
}

But it turns out that in Basis and in DDS that part is already done in doOpenData() usually. Except for some light image info stuff in basis maybe.
Imagine a format that compressess over multiple mips and uncompresses all of them for every image2D call... per-image state would need to be either cached in the importer's state or re-computed for every mip. (I wouldn't think this really needs to be accounted for, since most formats don't seem to work this way?)

@mosra
Copy link
Owner Author

mosra commented Aug 27, 2019

Imagine a format that compressess over multiple mips and uncompresses all of them for every image2D call

Yeah, then the importer would need to be more clever and cache the uncompressed data for subsequent runs (or move some work from doImage() into the doOpen*() function). That's the same AssimpImporter & co. need to do as well -- avoid instantiating, opening and parsing the images for each subsequent loaded level.

@mosra mosra modified the milestones: 2019.0b, 2019.0c Oct 2, 2019
@mosra mosra mentioned this pull request Jan 1, 2020
87 tasks
@mosra mosra mentioned this pull request Feb 18, 2020
70 tasks
@mosra mosra moved this from TODO to In progress in Asset management Feb 23, 2020
@codecov-io
Copy link

codecov-io commented Feb 23, 2020

Codecov Report

Merging #369 into master will decrease coverage by 3.93%.
The diff coverage is 84.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
- Coverage   76.46%   72.53%   -3.94%     
==========================================
  Files         363      368       +5     
  Lines       17500    19330    +1830     
==========================================
+ Hits        13382    14021     +639     
- Misses       4118     5309    +1191
Impacted Files Coverage 螖
src/MagnumPlugins/TgaImporter/TgaImporter.h 100% <酶> (酶) 猬嗭笍
.../MagnumPlugins/AnyImageImporter/AnyImageImporter.h 100% <酶> (酶) 猬嗭笍
src/Magnum/Trade/AbstractImporter.h 100% <酶> (酶) 猬嗭笍
src/MagnumPlugins/ObjImporter/ObjImporter.cpp 93.33% <酶> (-0.65%) 猬囷笍
.../MagnumPlugins/AnySceneImporter/AnySceneImporter.h 100% <酶> (酶) 猬嗭笍
...agnumPlugins/AnySceneImporter/AnySceneImporter.cpp 45.86% <0%> (-0.23%) 猬囷笍
src/Magnum/Trade/AbstractImporter.cpp 100% <100%> (酶) 猬嗭笍
src/MagnumPlugins/TgaImporter/TgaImporter.cpp 100% <100%> (酶) 猬嗭笍
...agnumPlugins/AnyImageImporter/AnyImageImporter.cpp 89.38% <33.33%> (-1.45%) 猬囷笍
src/Magnum/GL/Implementation/ShaderState.cpp 40% <0%> (-60%) 猬囷笍
... and 211 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 010e684...92a9940. Read the comment docs.

@mosra mosra force-pushed the importer-image-level branch 2 times, most recently from 71baeb1 to 8075363 Compare February 25, 2020 11:23
@mosra mosra marked this pull request as ready for review February 25, 2020 11:24
There's a new level option in each imageND() API, plus an
imageNDLevelCount() query to get image level count.
Needed for caching in importers that support mip level import.
@mosra mosra merged commit 92a9940 into master Feb 25, 2020
Asset management automation moved this from In progress to Done Feb 25, 2020
@mosra mosra deleted the importer-image-level branch February 25, 2020 15:46
@mosra
Copy link
Owner Author

mosra commented Feb 25, 2020

For completeness, the corresponding change in plugins is mosra/magnum-plugins@15e890b...99b9e2f. It's a bit more involved in order to be efficient when loading consecutive mip levels -- scene importers have to cache an existing image importer instance to avoid decoding the image again from scratch for each level.

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

3 participants