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.

Sign up for GitHub

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

Jump to bottom

[Experimental] Add Betsy GPU texture compressor #91150

Closed
wants to merge 1 commit into from
Closed

[Experimental] Add Betsy GPU texture compressor #91150

wants to merge 1 commit into from

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Apr 25, 2024 •
edited
Loading

A proof-of-concept implementation of the Betsy GPU texture compressor. Currently, the only supported formats are BC1 and BC6.

In terms of performance it is comparable to etcpak, albeit the results are significantly higher-quality, especially with BC1.

TODO:

  • Add all other formats currently supported by Betsy,
  • Look into BC7 and ASTC,
  • Improve the parallelization of the process. Currently each mip map is compressed one after another, ideally they should all be compressed concurrently and combined at the end,
  • Clean up the code,
  • Get some insight from someone with more knowledge about compute shaders whether I'm doing this correctly,
  • Find a way to speed up RGB to RGBA texture conversion (RGB formats cannot be sampled directly),
  • Use the default static methods in Image instead of rd-specific variants,
  • Some of the shader code was modified, ideally it should be reverted to how it is in upstream,
  • Test on a lot of different GPUs,
  • Move the thirdparty files into the thirdparty directory,
  • Fix memory leaks.

@AThousandShips AThousandShips added this to the 4.x milestone Apr 25, 2024
@fire
Copy link
Member

fire commented Apr 25, 2024

Hooray! I will review as I can.

@fire fire requested a review from a team April 25, 2024 12:49
@fire
Copy link
Member

fire commented Apr 25, 2024

Can you rebase to master and apply the pre-commit run -a to format the code?

@BlueCube3310 BlueCube3310 force-pushed the betsy-gpu-compressor branch 2 times, most recently from 6e5255a to cbd65f4 Compare April 25, 2024 17:29
fire
fire reviewed Apr 25, 2024
View reviewed changes
modules/betsy/bc1.glsl
@@ -0,0 +1,480 @@
#[compute]
Copy link
Member

@fire fire Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@akien-mga What's the policy on things like a glsl compute shader from Betsy. Normally it should go in the thirdparty but maybe you've got other suggestions.

fire
fire reviewed Apr 25, 2024
View reviewed changes
core/io/image.cpp
void (*Image::_image_compress_bptc_rd_func)(Image *, Image::UsedChannels) = nullptr;
void (*Image::_image_compress_etc1_rd_func)(Image *) = nullptr;
void (*Image::_image_compress_etc2_rd_func)(Image *, Image::UsedChannels) = nullptr;
//void (*Image::_image_compress_astc_rd_func)(Image *, Image::ASTCFormat) = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

We tend to remove commented code. It is also unclear if betsy will have ASTC support in the short term.

@clayjohn clayjohn mentioned this pull request Apr 25, 2024
Merged
@BlueCube3310 BlueCube3310 force-pushed the betsy-gpu-compressor branch 2 times, most recently from b98666a to e8ceeb5 Compare April 25, 2024 18:09
fire
fire reviewed Apr 25, 2024
View reviewed changes
@BlueCube3310 BlueCube3310 force-pushed the betsy-gpu-compressor branch from e8ceeb5 to de6ea0c Compare April 28, 2024 12:41
@lyuma
Copy link
Contributor

lyuma commented May 3, 2024

In asset meeting: we should focus on getting this in for BC6H for light maps

@BlueCube3310
Copy link
Contributor Author

In asset meeting: we should focus on getting this in for BC6H for light maps

BC6 support is pretty much finished, it just needs testing. I think I'm going to make a separate PR which only implements that format so it can be merged early on for 4.4, while this one with a full integration can be merged later.

@BlueCube3310 BlueCube3310 mentioned this pull request May 3, 2024
Merged
1 task
@BlueCube3310
Copy link
Contributor Author

Closing as this is superseded by #91535 and #95915

@AThousandShips AThousandShips removed this from the 4.x milestone Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fire fire fire left review comments

Assignees
No one assigned
Labels
archived enhancement feature proposal topic:import
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BlueCube3310 @fire @lyuma @AThousandShips