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

BasisImporter #62

Closed
wants to merge 4 commits into from
Closed

BasisImporter #62

wants to merge 4 commits into from

Conversation

@Squareys
Copy link
Contributor

Squareys commented Jul 31, 2019

> BasisImporter< BasisImageConverter AnyImageImporter/Converter

Hi @mosra !

This is the pullrequest for the BasisImporter plugin. The BasisConverter will be in a separate pullrequest.

I will ping you once you can take the first look at it. Big warning: I currently added all of the https://github.com/binomialLLC/basis_universal code with my patch. I am still hoping BinomialLLC/basis_universal#46 or BinomialLLC/basis_universal#11 or maybe BinomialLLC/basis_universal#13 will be fixed/merged/dealt with by the time this pullrequest is ready or the next release that Rich is working on solves linking to it as a library.

If not, we can rediscuss options for how to integrate it best.

Sincerely,
Jonathan

TODOs

  • Transcode a basis file
  • Target format configuration
  • Magnum plugins find module
  • Basis find module
    • Implement
    • Await tested on all platforms
  • Add to all the packages
  • Remove basis source code
  • Basis dependency on CI
    • AppVeyor
    • travis
  • ... and packages?
  • Add to changelog and other parts of docs
  • Compiled snippets
  • Enable on CI
  • Test
    • Basic, open twice, import twice
    • All aliases (instanced)
    • Some more basis input file configurations
      • Multiple images in a file
  • [ ] AnyImageImporter delegation => mosra/magnum#370
@mosra mosra added this to the 2019.0b milestone Aug 1, 2019
@Squareys Squareys force-pushed the Squareys:basis-importer branch from 0a5d43f to 1f76e6b Aug 20, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 20, 2019

Codecov Report

Merging #62 into master will decrease coverage by 0.15%.
The diff coverage is 78.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   90.94%   90.79%   -0.16%     
==========================================
  Files          49       51       +2     
  Lines        4240     3898     -342     
==========================================
- Hits         3856     3539     -317     
+ Misses        384      359      -25
Impacted Files Coverage Δ
src/MagnumPlugins/BasisImporter/BasisImporter.h 100% <100%> (ø)
src/MagnumPlugins/BasisImporter/BasisImporter.cpp 78.65% <78.65%> (ø)
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 82.42% <0%> (-3.21%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
src/MagnumPlugins/FreeTypeFont/FreeTypeFont.cpp 98.73% <0%> (-0.07%) ⬇️
src/Magnum/OpenDdl/Structure.h 96.49% <0%> (-0.07%) ⬇️
src/MagnumPlugins/DdsImporter/DdsImporter.cpp 77.95% <0%> (-0.04%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
... and 19 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 553154e...b3341af. Read the comment docs.

@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Aug 22, 2019

@mosra So, there has been zero visible progress on build system / library support in basis_universal since June. With this PR, I just copied the files of my forked repository with the debug iterator level fix into external, but that was just a temporary solution.

How are we going to include basis? I currently don't see a way that doesn't involve our own patched version, but maybe a submodule is a better idea than just copying the files?
Otoh, we can stick to only the essential files this way and provide our own CMake setup.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Aug 22, 2019

Oh well.

I would say we do it similarly as we did with ImGui (finding sources and building through and external CMake script), plus relying on your fork for Windows builds until BinomialLLC/basis_universal#46 gets finally merged. Or is there anything else needed beyond that?

Submodules are problematic since git submodule update --init implicitly pulls in everything (what if the 3rd party repo suddenly starts including hundreds of megabytes of data, like assimp did? then everyone downloading plugins or having plugins as a submodule would have to transitively suffer through that too) and this would add a dangerous precedent when other dependencies get considered in the future. (Plus iterating on that 3rd party lib or trying out various forks of it is harder when you have a submodule pinned -- a problem I'm running into very often recently.)

@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Aug 22, 2019

That submodule concern makes a lot of sense, yes.

Finding the sources via CMake is a good idea, I remember you suggested that before 👍 But good to have it documented here on GitHub now.

Or is there anything else needed beyond that?

For windows support, no further code changes. But to build as a library, we will have to add some cmake project. (Theirs just builds the basisu tool.)
But that can be done through the find module.

@Squareys Squareys force-pushed the Squareys:basis-importer branch from 1f76e6b to 08138f9 Aug 22, 2019
@mosra mosra added this to TODO in Asset management via automation Aug 22, 2019
@mosra mosra mentioned this pull request Aug 22, 2019
6 of 45 tasks complete
@Squareys Squareys force-pushed the Squareys:basis-importer branch 3 times, most recently from 5d5f0a0 to 6cbe2cc Aug 23, 2019
@Squareys Squareys force-pushed the Squareys:basis-importer branch 7 times, most recently from f6bbf13 to 9a637c3 Aug 24, 2019
@mosra mosra mentioned this pull request Aug 27, 2019
0 of 8 tasks complete
@Squareys Squareys force-pushed the Squareys:basis-importer branch 3 times, most recently from 032b19b to 2cd6e42 Aug 27, 2019
@Squareys Squareys mentioned this pull request Aug 27, 2019
2 of 2 tasks complete
@Squareys Squareys force-pushed the Squareys:basis-importer branch from 2cd6e42 to 13cfcb9 Aug 27, 2019
Copy link
Owner

mosra left a comment

This looks quite finished already! 🎉

src/MagnumPlugins/BasisImporter/Test/README.md Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/Test/CMakeLists.txt Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
doc/building-plugins.dox Outdated Show resolved Hide resolved
@Squareys Squareys force-pushed the Squareys:basis-importer branch 2 times, most recently from aab0800 to 0c9df86 Aug 28, 2019
@Squareys Squareys force-pushed the Squareys:basis-importer branch from 5b22504 to 57b645b Aug 29, 2019
@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Aug 29, 2019

Alright, done! 🎉

@Squareys Squareys changed the title [WIP] BasisImporter BasisImporter Aug 29, 2019
@Squareys Squareys force-pushed the Squareys:basis-importer branch from c4adfb6 to bd6791e Aug 29, 2019
Copy link
Owner

mosra left a comment

I'm a bit sad that codecov is broken and doesn't show the actual covered lines. Would need to check the coverage locally, then.

src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/BasisImporter.cpp Outdated Show resolved Hide resolved
@Squareys Squareys mentioned this pull request Aug 29, 2019
15 of 15 tasks complete
package/ci/appveyor-desktop.bat Outdated Show resolved Hide resolved
@Squareys Squareys force-pushed the Squareys:basis-importer branch 3 times, most recently from 6d7b29e to 23cead4 Aug 29, 2019
@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Aug 30, 2019

@mosra Ready! 👍

Copy link
Owner

mosra left a comment

Last few bits and then I'll leave you in peace :)

For the real-world test before I merge to master (and a nice & useful feature as a side-effect) I'll add compressed image support to magnum-player and try to open a few Basis images with it.

doc/snippets/MagnumPluginsBasisImporter.cpp Outdated Show resolved Hide resolved
package/ci/appveyor-desktop-mingw.bat Outdated Show resolved Hide resolved
src/MagnumPlugins/BasisImporter/BasisImporter.h Outdated Show resolved Hide resolved
@Squareys Squareys force-pushed the Squareys:basis-importer branch 2 times, most recently from d50b492 to 124d4c8 Aug 31, 2019
@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Aug 31, 2019

Done once more :)
(Waiting for the CI still, though)

@Squareys Squareys force-pushed the Squareys:basis-importer branch from 124d4c8 to ac6fece Aug 31, 2019
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the Squareys:basis-importer branch from ac6fece to d4bb29a Sep 1, 2019
Squareys added 3 commits Jul 22, 2019
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the Squareys:basis-importer branch from d4bb29a to b3341af Sep 1, 2019
@mosra mosra mentioned this pull request Sep 28, 2019
@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 2, 2019

Merged in 627beff, 0c238b8, cd9d838 and 3c00412; updated to ASTC and uncompressed formats in fcb1439. Docs are up now as well.

There's still stuff to be done on Basis side, see #70.

Thanks for working on this!

@mosra mosra closed this Oct 2, 2019
Asset management automation moved this from TODO to Done Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.