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

Add astcenc compression and decompression. #70363

Merged
merged 1 commit into from Jan 19, 2023

Conversation

fire
Copy link
Member

@fire fire commented Dec 20, 2022

Keep mingw compatibility by modifying astcenc_platform_isa_detection.cpp to use all lowercase for Windows includes.

@reduz

Done as part of the https://github.com/v-sekai/v-sekai-game work for opensource Social VR.

@fire fire requested review from a team and reduz December 20, 2022 19:27
@fire fire marked this pull request as ready for review December 20, 2022 19:27
@fire fire requested a review from a team as a code owner December 20, 2022 19:27
@fire fire mentioned this pull request Dec 20, 2022
5 tasks
@Calinou Calinou added this to the 4.0 milestone Dec 20, 2022
@fire
Copy link
Member Author

fire commented Dec 20, 2022

  • Needs licensing details
  • Pause investigation for interpolation for 4x4 and 8x8

@reduz
Copy link
Member

reduz commented Dec 21, 2022

This looks good to me, but @akien-mga has to check

@fire
Copy link
Member Author

fire commented Jan 9, 2023

ARM-software/astc-encoder#389 was backported to older versions

@akien-mga akien-mga self-requested a review January 19, 2023 11:23
@akien-mga
Copy link
Member

akien-mga commented Jan 19, 2023

I reviewed and cleaned up the code directly (currently as separate commits for review, I'll squash when approved).
I can't push to V-Sekai/astcenc-standalone to update this PR so I had to push to my own fork: https://github.com/akien-mga/godot/commits/astcenc-standalone

Once reviewed and rebased, either you'll need to push to your PR branch or give me access.

Edit: Squashed version if you're fine with it and want to force push it directly: https://github.com/akien-mga/godot/commits/astcenc-standalone-squashed

Co-authored-by: Gordon A Macpherson <gordon.a.macpherson@gmail.com>
Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@akien-mga
Copy link
Member

akien-mga commented Jan 19, 2023

For the record, the email address used to credit @RevoluPowered as co-author doesn't match his GitHub account.

Do you want me/fire to change it to the name and email from https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/67915.patch @RevoluPowered?

Edit: Actually it does match the GitHub account, it's just that in fire's commit "Co-authored-by:" was not recognized due to wrong casing. It works in my squashed commit: akien-mga@696346f
(But let me know anyway if you want this tweaked as it's not the same name/email as you use usually for Godot contributions.)

@akien-mga
Copy link
Member

@fire gave me access to the repo so I could update this PR.

I force push directly my squashed versions with all the changes I made, but to review properly you can still check the individual commits in https://github.com/akien-mga/godot/commits/astcenc-standalone

I haven't tested much, so it would be good to make sure it's still working as intended.

I noticed that we're not handling p_lossy_quality even though astcenc provides quality options. We currently default to "medium" quality. For now I put a TODO, is this something we want to handle in the future?

@fire
Copy link
Member Author

fire commented Jan 19, 2023

I noticed that we're not handling p_lossy_quality even though astcenc provides quality options. We currently default to "medium" quality. For now I put a TODO, is this something we want to handle in the future?

I think for now defaulting to the less quality astc is probably prudent until we can make the speed usable.

This is trying to unblock reduz's astc contributions.

@akien-mga
Copy link
Member

I noticed that we're not handling p_lossy_quality even though astcenc provides quality options. We currently default to "medium" quality. For now I put a TODO, is this something we want to handle in the future?

I think for now defaulting to the less quality astc is probably prudent until we can make the speed usable.

@reduz always says he plans to remove per-texture lossy quality in the future because most encoders don't really care. So using a good default value (medium seems fine to me) is good. We can add more quality options in the future (maybe a project setting instead of per-texture).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I'm happy with this PR with regard to the general code quality and integration of the thirdparty library. Great work, notably on stripping down the unnecessary CLI bits of astcenc!

I just need a confirmation that it still works properly in your tests as I did some cleanup in the compress and decompress methods and, while it should behave like before, I may have inadvertently broken something.

I also updated astcenc from 4.2.0 to 4.3.0 (released yesterday) btw.

@fire
Copy link
Member Author

fire commented Jan 19, 2023

The test I did [was] write an import decode function, it's quite hard to test, waiting for reduz's changes.

@akien-mga
Copy link
Member

So we merge as is and let reduz check if it's actually working when implementing the remaining bits, and fix it if it doesn't?

@fire
Copy link
Member Author

fire commented Jan 19, 2023

Yes, that sounds like a plan.

@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga merged commit 415d120 into godotengine:master Jan 19, 2023
@akien-mga akien-mga deleted the astcenc-standalone branch January 19, 2023 21:49
@realkotob
Copy link
Contributor

realkotob commented Jan 20, 2023

Just read through the ASTC format overview, exciting stuff! Thank you for adding this ❤️

Are there plans to use this as the default compressor for some platforms? I saw some changes to the etcpack module but it's mostly housekeeping. Didn't dig into all the astc files so not sure what the defaults for that are.

Was looking at the hardware support matrix for the extensions and it seems ubiquitous.

@Calinou
Copy link
Member

Calinou commented Jan 21, 2023

Was looking at the hardware support matrix for the extensions and it seems ubiquitous.

According to that page, ASTC support was removed on Intel Arc GPUs, which makes it a no-go as a default for desktop platforms. Support on AMD GPUs on Windows is also not mentioned, and it's probably not great either.

The same goes on macOS, as ASTC is only supported on M1/M2 GPUs, not anything x86.

Defaulting to ASTC on mobile needs testing for two reasons:

  • It's not available in the Compatibility backend. Switching backends will require reimporting all textures (like when you switch from GLES3 to GLES2 in 3.x). I don't know if this is a problem.
  • Some mobile devices likely have bugs with ASTC decoding, given it's recent (by mobile adoption standards).

@realkotob
Copy link
Contributor

@Calinou Thank you for answering my question and pointing out nuances I hadn't considered.

Looking forward to get to test this and see how well it's supported. (I've used ASTC within other engines but platform selection had good defaults so I didn't have to worry about it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants