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: support for 2D array images #118

Closed
wants to merge 13 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Jan 4, 2022

This adds missing functionality to BasisImageConverter to allow 2D array images.

The rationale for treating ImageView3D as a 2D array image is the same reason all 3D images in BasisImporter are imported as 2D array images. The tl;dr is that basis_universal assumes mip levels are 2D and KTX2 export doesn't support 3D images either.

There are minor fixes for KtxImageConverter in the commit list too, noticed those while cross-referencing code and docs.

1D images are treated as 2D images with height 1. 3D images are always treated as 2D array images because:
- basis_universal saves 3D images as 2D array images when exporting as KTX2
- basis_universal assumes mip levels are always 2D, producing size mismatches for real 3D mip levels
- basis_universal has no concept of indicating z-flip, this avoids having to reason about it (there's no header flag in .basis, and KTX2 is always written as 2D array images)
- BasisImporter always imports true 3D images (only supported by .basis, not KTX2) as 2D array images because of the same mip size issue
@pezcode pezcode force-pushed the basis-image-converter-3d-images branch from def11fc to 1acb9a5 Compare January 5, 2022 14:57
@pezcode pezcode force-pushed the basis-image-converter-3d-images branch from 1acb9a5 to 54de2e6 Compare January 5, 2022 15:10
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #118 (869137c) into master (9e30f51) will increase coverage by 0.00%.
The diff coverage is 97.75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
  Coverage   95.56%   95.57%           
=======================================
  Files         115      115           
  Lines       10830    10847   +17     
=======================================
+ Hits        10350    10367   +17     
  Misses        480      480           
Impacted Files Coverage Δ
...mPlugins/BasisImageConverter/BasisImageConverter.h 100.00% <ø> (ø)
src/MagnumPlugins/BasisImporter/BasisImporter.h 100.00% <ø> (ø)
...agnumPlugins/KtxImageConverter/KtxImageConverter.h 100.00% <ø> (ø)
src/MagnumPlugins/KtxImporter/KtxImporter.h 100.00% <ø> (ø)
src/MagnumPlugins/BasisImporter/BasisImporter.cpp 92.63% <66.66%> (ø)
...lugins/BasisImageConverter/BasisImageConverter.cpp 92.99% <98.73%> (+0.60%) ⬆️
...numPlugins/KtxImageConverter/KtxImageConverter.cpp 99.47% <100.00%> (ø)
src/MagnumPlugins/KtxImporter/KtxImporter.cpp 90.69% <100.00%> (ø)

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 9e30f51...869137c. Read the comment docs.

@mosra mosra added this to TODO in Asset management via automation Jan 9, 2022
@mosra mosra added this to the 2021.0a milestone Jan 9, 2022
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!

This is inconsistent with the behavior of other image converters that only handle 2D images. To convert 1D images, cast the ImageView1D to ImageView2D.
…ne as the signature

It's part of the signature. This makes it consistent with existing code.
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! This looks great now 👍

TODO for me post-merge: make AnyImageConverter aware of Basis in 3D image export.

@pezcode pezcode changed the title BasisImageConverter: support for 1D images and 2D array images BasisImageConverter: support for 2D array images Jan 10, 2022
@mosra
Copy link
Owner

mosra commented Jan 30, 2022

Thank you! Merged as f9e10f4...2828427, I shuffled the commits around a bit to erase the temporary 1D image support from the history.

@mosra mosra closed this Jan 30, 2022
Asset management automation moved this from TODO to Done Jan 30, 2022
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

2 participants