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

BasisImageConverter #65

Merged
merged 4 commits into from Oct 10, 2019
Merged

BasisImageConverter #65

merged 4 commits into from Oct 10, 2019

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Aug 29, 2019

BasisImporter >BasisImageConverter< AnyImageImporter/Converter

Hey @mosra !

Here's step 2 of Basis support, based on the basis-importer branch, so will need to rebase once #62 is merged :)

TODOs:

  • Fix SEGFAULT on fileData return of doExportData
  • Test
    • RGB
    • RGBA
    • Invalid/Failure
  • Additional config options?
  • [ ] API for config options?
  • Enable on CI
    • Travis
    • AppVeyor
    • CI Green
  • Disable on all packages
  • Documentation
    • License
    • Configuration
    • Add everywhere

@codecov-io
Copy link

codecov-io commented Aug 31, 2019

Codecov Report

Merging #65 into master will decrease coverage by 0.37%.
The diff coverage is 92.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   90.99%   90.62%   -0.38%     
==========================================
  Files          51       53       +2     
  Lines        4377     4201     -176     
==========================================
- Hits         3983     3807     -176     
  Misses        394      394
Impacted Files Coverage Δ
...mPlugins/BasisImageConverter/BasisImageConverter.h 100% <100%> (ø)
...lugins/BasisImageConverter/BasisImageConverter.cpp 92.5% <92.5%> (ø)
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 83.33% <0%> (-3.17%) ⬇️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 92.43% <0%> (-0.47%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
...mPlugins/JpegImageConverter/JpegImageConverter.cpp 96.1% <0%> (-0.15%) ⬇️
src/Magnum/OpenDdl/OpenDdl.cpp 90.64% <0%> (-0.12%) ⬇️
src/MagnumPlugins/FreeTypeFont/FreeTypeFont.cpp 98.73% <0%> (-0.07%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
... and 18 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 66f7357...0f181d8. Read the comment docs.

@Squareys Squareys force-pushed the basis-converter branch 14 times, most recently from 6d3ff09 to 7f7938f Compare September 1, 2019 15:06
@Squareys Squareys changed the title [WIP] BasisImageConverter BasisImageConverter Sep 1, 2019
@Squareys
Copy link
Contributor Author

Rebased onto next :)

@mosra mosra changed the base branch from master to next September 24, 2019 11:03
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!

doc/building-plugins.dox Outdated Show resolved Hide resolved
package/archlinux/PKGBUILD-release Outdated Show resolved Hide resolved
@mosra mosra changed the base branch from next to master September 30, 2019 16:09
@mosra mosra added this to the 2019.0b milestone Oct 1, 2019
@mosra mosra added this to TODO in Asset management via automation Oct 1, 2019
CORRADE_VERIFY(compressedData);

if(_importerManager.loadState("BasisImporterRGBA8") == PluginManager::LoadState::NotFound)
CORRADE_SKIP("BasisImporterRGBA8 plugin not found, cannot test");
Copy link
Owner

Choose a reason for hiding this comment

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

Just check for BasisImporter alone, this error message may be confusing for people who don't know the aliases. Same 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.

But I guess I would check for BasisImporterRGBA8, but just change the error message, right?

Copy link
Owner

Choose a reason for hiding this comment

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

If there's BasisImporter and it doesn't provide a BasisImporterRGBA8 alias, then there's something very off and it should blow up (while this would lead to just SKIP) ... so pick the loudest alternative :D

But this is a minor thing, nothing critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, not necessarily, could just be compiled out or an older version 🤷‍♂

Not sure what your comment means in practice, shall I remove the RGBA8 prefix from both the skip message and the loadState call?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, remove both.

src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

Squareys commented Oct 3, 2019

@mosra Rebased, let's see what the CIs say!

@Squareys Squareys force-pushed the basis-converter branch 4 times, most recently from 46db66f to d31cba5 Compare October 3, 2019 19:29
Signed-off-by: Squareys <squareys@googlemail.com>
@mosra mosra merged commit 0f181d8 into mosra:master Oct 10, 2019
Asset management automation moved this from TODO to Done Oct 10, 2019
@mosra
Copy link
Owner

mosra commented Oct 10, 2019

Merged, thank you! 🎉

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