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

Compatibility: Improve gl texture format detection #101217

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Jan 7, 2025

Follow-up to #100856

This PR simplifies the flow of the _get_gl_image_and_format method as well as ensures that any decompressed format is passed directly into the output (previously, most decompressed formats were converted into RGBA8, including HDR ones).

This will ensure that the decompressed ASTC HDR textures will look the same as in Mobile and Forward+

Additionally, support for Image::FORMAT_RGB565 is now implemented, as before it was missing likely due to an oversight.

@fire
Copy link
Member

fire commented Jan 7, 2025

This would be best reviewed by @clayjohn.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected. However, I didn't encounter any issues when loading ASTC-compressed textures in Compatibility prior to this PR.

Code looks good to me.

Testing project: test_pr_101217.zip

I did some testing with various compressed formats and noticed an issue with ETC2 in particular. Also, notice how the ASTC memory usage is much higher in Forward+ than in Compatibility. (It actually takes a long time to load, which makes me think it's being decompressed on the CPU, but only in Forward+, not in Compatibility.)

Forward+

S3TC ETC2 BPTC ASTC
image image image image

This error is printed every frame while previewing the ETC2 texture in the inspector:

ERROR: Attempting to use an uninitialized RID
ERROR: ./servers/rendering/renderer_rd/storage_rd/texture_storage.cpp:1411 - Parameter "tex" is null.

Compatibility

S3TC ETC2 BPTC ASTC
image image image image

Unlike Forward+, I can use the ASTC-compressed HDRI as a sky texture, whereas it's just black in Forward+. I'm on a NVIDIA graphics card which lacks native ASTC support though.

@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Jan 12, 2025

Thank you for testing!

Also, notice how the ASTC memory usage is much higher in Forward+ than in Compatibility. (It actually takes a long time to load, which makes me think it's being decompressed on the CPU, but only in Forward+, not in Compatibility.)

Both are actually decompressed on CPU. The memory usage should be the same, it looks like Compatibility creates a copy of the source image before uploading it to GPU, whereas Forward+ operates on the source data

This error is printed every frame while previewing the ETC2 texture in the inspector:

ERROR: Attempting to use an uninitialized RID
ERROR: ./servers/rendering/renderer_rd/storage_rd/texture_storage.cpp:1411 - Parameter "tex" is null.

That is to be expected, it will be resolved by #100365

Unlike Forward+, I can use the ASTC-compressed HDRI as a sky texture, whereas it's just black in Forward+. I'm on a NVIDIA graphics card which lacks native ASTC support though.

That's surprising. it should work the same in Forward+
Edit: This happens because the engine cannot load .ctex files from the filesystem directly, but only via the imported pipeline, so it's unrelated to this.

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.

Looks good to me.

@akien-mga akien-mga merged commit 89f233a into godotengine:master Jan 12, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi modified the milestones: 4.x, 4.4 Jan 18, 2025
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.

5 participants