Load all available texture mip levels in ResourceManager - #505
Merged
Conversation
Generating the mipmap only if the loaded image has a single uncompressed level. Also keeping the existing unused/obsolete on-the-fly texture compression, as it's still wired into the API.
facebook-github-bot
added
the
CLA Signed
bigbike
approved these changes
Feb 26, 2020
erikwijmans
approved these changes
Feb 26, 2020
erikwijmans
left a comment
erikwijmans
left a comment
Contributor
There was a problem hiding this comment.
The CI failure is due to an unreliable test, this should be fixed now.
Codecov Report
@@ Coverage Diff @@
## master #505 +/- ##
=======================================
Coverage 59.86% 59.86%
=======================================
Files 158 158
Lines 7114 7114
Branches 84 84
=======================================
Hits 4259 4259
Misses 2855 2855
Continue to review full report at Codecov.
|
Merged
Ram81
pushed a commit
to Ram81/habitat-web-sim
that referenced
this pull request
Dec 10, 2020
…earch#505) * Update Magnum submodules for mip level loading. * Update Magnum submodules for mip level loading. * assets: load all texture mip levels in ResourceManager. Generating the mipmap only if the loaded image has a single uncompressed level. Also keeping the existing unused/obsolete on-the-fly texture compression, as it's still wired into the API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
The "basisified" MP3D files @erikwijmans encoded a while ago contain a mip pyramid for each image, however until earlier today, Magnum APIs were only able to load the first one (see mosra/magnum#369 for the corresponding change on Magnum's side). While this is a relatively minor change, it has non-negligible impact on rendering times. A very rough artificial benchmark is below, showing clear advantage of having the extra mip levels loaded.
Originally, before the files were basisified, the loaded JPEGs got their mip levels generated during import (second bar in the plot above). Above that is comparison to JPEGs loaded in a single level, which is slower due to extra memory fetches needed for minifying the full-size textures.
What is interesting that, on my machine at least, the cost of decoding an ASTC texture makes it similarly slow as uncompressed textures (however the memory used is four times less). That isn't the case for BC7/BC3 formats (even though BC7 isn't exactly simple either), there I suppose the high vertex count dominated the times, and not texture access, thus the difference with mips loaded not being too large. — I have no way to test on NVidia to compare, but I suspect this is due to ASTC still being relatively new and not fully optimized in AMD/Intel hardware.
Would be great to hear about what speed impact this has on the usual workflows.
Besides the above, this drags along:
Magnum/Math/PackingBatch.h(to be used for the upcoming optimized mesh loading)CORRADE_ITERATION()How Has This Been Tested
Loaded a random basisified MP3D file in the viewer, verified textures show up correctly. The change on Habitat's side actually mirrors an update I did for magnum-player, where I also did the above benchmark and verified other corner case.
ℹ️ The CI fails, but jobs from other branches have the same error, so this time it's not my fault :)
Types of changes