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

[Merged by Bors] - KTX2/DDS/.basis compressed texture support #3884

Closed
wants to merge 39 commits into from

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Feb 7, 2022

Objective

  • Support compressed textures including 'universal' formats (ETC1S, UASTC) and transcoding of them to
  • Support .dds, .ktx2, and .basis files

Solution

  • Fixes Compressed texture support #3608 Look there for more details.
  • Note that the functionality is all enabled through non-default features. If it is desirable to enable some by default, I can do that.
  • The basis-universal crate, used for .basis file support and for transcoding, is built on bindings against a C++ library. It's not feasible to rewrite in Rust in a short amount of time. There are no Rust alternatives of which I am aware and it's specialised code. In its current state it doesn't support the wasm target, but I don't know for sure. However, it is possible to build the upstream C++ library with emscripten, so there is perhaps a way to add support for web too with some shenanigans.
  • There's no support for transcoding from BasisLZ/ETC1S in KTX2 files as it was quite non-trivial to implement and didn't feel important given people could use .basis files for ETC1S.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 7, 2022
@superdump superdump mentioned this pull request Feb 7, 2022
2 tasks
@superdump superdump added A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Feb 7, 2022
@superdump superdump force-pushed the compressed-textures branch from 7f01cff to fa574dc Compare February 7, 2022 15:17
@superdump superdump self-assigned this Feb 7, 2022
{
return Err(TextureError::FormatRequiresTranscodingError(
TranscodeFormat::Rgb8,
));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe an error is a strange way to communicate this but...

@superdump superdump marked this pull request as ready for review February 15, 2022 15:39
@superdump superdump requested a review from cart February 15, 2022 15:40
@superdump
Copy link
Contributor Author

superdump commented Feb 15, 2022

I've looked this over some more and gone through to remove/address a bunch of FIXMEs I'd left. I changed my mind and now I'm thinking that it's better to just get this in rather than being overly careful. It's better that people start using it and we iterate rather than letting perfect be the enemy of good.

Cargo.toml Show resolved Hide resolved
@superdump
Copy link
Contributor Author

About the need to add GltfPlugin and ImagePlugin after the renderer initialisation: would the normal case still work if that's not done?

There are plans to make renderer initialisation async, which is needed at least for android and webgpu

Then I guess RenderDevice will not be available and this will panic? Maybe they should be changed back to .get_resource::<RenderDevice>().map_or_else(|| wgpu::Features::empty(), |render_device| render_device.features()) or similar but correct?

@superdump
Copy link
Contributor Author

About the need to add GltfPlugin and ImagePlugin after the renderer initialisation: would the normal case still work if that's not done?
There are plans to make renderer initialisation async, which is needed at least for android and webgpu

Then I guess RenderDevice will not be available and this will panic? Maybe they should be changed back to .get_resource::<RenderDevice>().map_or_else(|| wgpu::Features::empty(), |render_device| render_device.features()) or similar but correct?

Although, this will basically mean that there will be no compressed texture support on Android and WebGPU and that isn't acceptable. So this is just something that needs to be solved. I tried to look into if the loader can be mutated after initialisation, but then I realised that that wouldn't work as it could allow attempts to load assets before enough information is available to do so. Some things have to wait until the renderer is initialised. How exactly we do that, I don't know.

@mockersf
Copy link
Member

mockersf commented Mar 6, 2022

the async plugin story in Bevy is not great for now, it sounds like we will need to work on it to have compressed texture support everywhere

@superdump
Copy link
Contributor Author

the async plugin story in Bevy is not great for now, it sounds like we will need to work on it to have compressed texture support everywhere

Yes, and it feels like it is outside the scope of this PR.

@superdump superdump mentioned this pull request Mar 6, 2022
@superdump superdump added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 7, 2022
@superdump
Copy link
Contributor Author

By the way, I didn't do the blanket matches so it was easy to see what formats there were in the library and which were supported and not, by looking at the code. I'm fine with it either way though.

@cart
Copy link
Member

cart commented Mar 15, 2022

By the way, I didn't do the blanket matches so it was easy to see what formats there were in the library and which were supported and not, by looking at the code. I'm fine with it either way though.

I think I'd prefer to bias toward smaller code + eliminate redundancies. People that want to add new formats will know what they're looking for and can look at the enum definitions / docs to find out if its supported.

@cart
Copy link
Member

cart commented Mar 15, 2022

(but thanks for the heads up. thats definitely a reasonable approach and I probably should have discussed it before making the change)

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This looks good to me. This supports like a million permutations of texture types, so I can't really test that it works. But this feels like a "let people yell at us if we did something wrong" sort of situation.

@cart
Copy link
Member

cart commented Mar 15, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 15, 2022
# Objective

- Support compressed textures including 'universal' formats (ETC1S, UASTC) and transcoding of them to 
- Support `.dds`, `.ktx2`, and `.basis` files

## Solution

- Fixes #3608 Look there for more details.
- Note that the functionality is all enabled through non-default features. If it is desirable to enable some by default, I can do that.
- The `basis-universal` crate, used for `.basis` file support and for transcoding, is built on bindings against a C++ library. It's not feasible to rewrite in Rust in a short amount of time. There are no Rust alternatives of which I am aware and it's specialised code. In its current state it doesn't support the wasm target, but I don't know for sure. However, it is possible to build the upstream C++ library with emscripten, so there is perhaps a way to add support for web too with some shenanigans.
- There's no support for transcoding from BasisLZ/ETC1S in KTX2 files as it was quite non-trivial to implement and didn't feel important given people could use `.basis` files for ETC1S.
@bors bors bot changed the title KTX2/DDS/.basis compressed texture support [Merged by Bors] - KTX2/DDS/.basis compressed texture support Mar 15, 2022
@bors bors bot closed this Mar 15, 2022
@superdump
Copy link
Contributor Author

Woohoo!

aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

- Support compressed textures including 'universal' formats (ETC1S, UASTC) and transcoding of them to 
- Support `.dds`, `.ktx2`, and `.basis` files

## Solution

- Fixes bevyengine#3608 Look there for more details.
- Note that the functionality is all enabled through non-default features. If it is desirable to enable some by default, I can do that.
- The `basis-universal` crate, used for `.basis` file support and for transcoding, is built on bindings against a C++ library. It's not feasible to rewrite in Rust in a short amount of time. There are no Rust alternatives of which I am aware and it's specialised code. In its current state it doesn't support the wasm target, but I don't know for sure. However, it is possible to build the upstream C++ library with emscripten, so there is perhaps a way to add support for web too with some shenanigans.
- There's no support for transcoding from BasisLZ/ETC1S in KTX2 files as it was quite non-trivial to implement and didn't feel important given people could use `.basis` files for ETC1S.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Support compressed textures including 'universal' formats (ETC1S, UASTC) and transcoding of them to 
- Support `.dds`, `.ktx2`, and `.basis` files

## Solution

- Fixes bevyengine#3608 Look there for more details.
- Note that the functionality is all enabled through non-default features. If it is desirable to enable some by default, I can do that.
- The `basis-universal` crate, used for `.basis` file support and for transcoding, is built on bindings against a C++ library. It's not feasible to rewrite in Rust in a short amount of time. There are no Rust alternatives of which I am aware and it's specialised code. In its current state it doesn't support the wasm target, but I don't know for sure. However, it is possible to build the upstream C++ library with emscripten, so there is perhaps a way to add support for web too with some shenanigans.
- There's no support for transcoding from BasisLZ/ETC1S in KTX2 files as it was quite non-trivial to implement and didn't feel important given people could use `.basis` files for ETC1S.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

Compressed texture support
3 participants