Skip to content

AnyImageImporter: Delegate .basis files - #370

Closed
Squareys wants to merge 2 commits into
mosra:masterfrom
Squareys:anyimporter-basis
Closed

AnyImageImporter: Delegate .basis files#370
Squareys wants to merge 2 commits into
mosra:masterfrom
Squareys:anyimporter-basis

Conversation

@Squareys

@Squareys Squareys commented Aug 27, 2019

Copy link
Copy Markdown
Contributor
BasisImporter BasisImageConverter >AnyImageImporter/Converter<

Hello @mosra !

This part of mosra/magnum-plugins#62 is hereby up for discussion.

TODOs

  • Rebase
  • Test ?

@Squareys Squareys mentioned this pull request Aug 27, 2019
20 tasks
@mosra

mosra commented Aug 27, 2019

Copy link
Copy Markdown
Owner

Why BasisImporterEtc2 and not BasisImporter? Concrete format selection could be done either with

manager.setPreferredPlugins("BasisImporter", {"BasisImporterEtc2"});

or by modifying global configuration for the BasisImporter plugin:

manager.metadata("BasisImporter")->configuration().setValue("format", "Etc2");

@Squareys

Copy link
Copy Markdown
Contributor Author

The issue is that BasisImporter has Etc2 as default, but a hypothetical TinyBasisImporter could use Etc1. So the user would get a different ->compressedFormat() depending on which plugins he has installed 🤔

@mosra

mosra commented Aug 28, 2019

Copy link
Copy Markdown
Owner

Hypothetical

Yes. But even in that case, both defaults are equally arbitrary and probably wrong, which leads me to think...

default

What about ... not having a default at all? Like, the plugin failing to load a file when you forget to give it a default? That seems like the best way to avoid errors. Things that deal with AnyImageImporter need to handle loading errors anyway so this wouldn't introduce anything unexpected, and if the code wants to deal with basis files, it needs to set those defaults properly in advance, otherwise image2D() always gives NullOpt.

@mosra mosra added this to the 2019.0b milestone Aug 28, 2019
@Squareys

Copy link
Copy Markdown
Contributor Author

That is actually an awesome idea, as it will nudge people to read the docs for how to configure and make a probably way more suited decision than "use default Etc2". Will make that change to BasisImporter

@mosra mosra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe I'll merge the converter part later, don't want people to complain that "it wants a BasisImageConverter, but I can't find it anywhere! where it is?" 😆

Comment thread src/MagnumPlugins/AnyImageConverter/AnyImageConverter.h Outdated
Comment thread src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp
@Squareys

Copy link
Copy Markdown
Contributor Author

Maybe I'll merge the converter part later, don't want people to complain that "it wants a BasisImageConverter, but I can't find it anywhere! where it is?" 😆

I doubt that anyone will stumble over this in the next couple of days... I'm planning to get mosra/magnum-plugins#65 merge-ready by Sunday at the latest 😅 Depends on your schedule, though :P

@codecov-io

codecov-io commented Aug 30, 2019

Copy link
Copy Markdown

Codecov Report

Merging #370 into master will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   71.99%   71.98%   -0.01%     
==========================================
  Files         348      348              
  Lines       18180    18186       +6     
==========================================
+ Hits        13088    13092       +4     
- Misses       5092     5094       +2
Impacted Files Coverage Δ
.../MagnumPlugins/AnyImageImporter/AnyImageImporter.h 100% <ø> (ø) ⬆️
...agnumPlugins/AnyImageConverter/AnyImageConverter.h 100% <ø> (ø) ⬆️
...numPlugins/AnyImageConverter/AnyImageConverter.cpp 84.61% <50%> (-1.88%) ⬇️
...agnumPlugins/AnyImageImporter/AnyImageImporter.cpp 90.99% <83.33%> (-0.6%) ⬇️

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 7b43ab5...d5b5ac1. Read the comment docs.

@mosra mosra left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Last two bits, otherwise I'm happy and this is ready to land :)

Comment thread src/MagnumPlugins/AnyImageImporter/AnyImageImporter.h Outdated
Comment thread src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp Outdated
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@mosra

mosra commented Sep 28, 2019

Copy link
Copy Markdown
Owner

Merged as a1c2c9c and b4ca71e, thank you!

@mosra

mosra commented Sep 28, 2019

Copy link
Copy Markdown
Owner

Ugh why is it that I always discover bugs after things get pushed to master -- the file signature is just sB, the bytes after are just a version that changes all the time (in particular, files generated with current basisu have some other version than the bundled kodim20.basis etc. in the Basis repo, so those won't load).

Fix pushed as 64e365e.

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.

3 participants