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

[3.x] Implement loading DDS textures at run-time (reverted) #69101

Conversation

marcinn
Copy link
Contributor

@marcinn marcinn commented Nov 24, 2022

This is a 3.x backport of godotengine/godot-proposals#5748.
PR for master branch (4.x) is here: /pull/69085

Adds:

  • Image::load_dds_from_buffer()
  • ability to load DDS textures at runtime via Image::load_from_file

The DDS loader implementation is moved from texture loader to image loader.

@marcinn marcinn requested review from a team as code owners November 24, 2022 08:47
@marcinn marcinn force-pushed the 3.x-backport-proposal-5748-loading-dds-at-runtime branch from 9cf61d2 to 7670f68 Compare November 24, 2022 08:52
@marcinn marcinn changed the title Backport implement loading DDS textures at run-time [3.x] Backport implement loading DDS textures at run-time Nov 24, 2022
@akien-mga akien-mga added this to the 3.6 milestone Nov 24, 2022
@marcinn marcinn force-pushed the 3.x-backport-proposal-5748-loading-dds-at-runtime branch from 7670f68 to 39b3f15 Compare November 29, 2022 19:47
core/image.cpp Outdated
@@ -3300,6 +3302,14 @@ Error Image::load_bmp_from_buffer(const PoolVector<uint8_t> &p_array) {
return _load_from_buffer(p_array, _bmp_mem_loader_func);
}

Error Image::load_dds_from_buffer(const PoolVector<uint8_t> &p_array) {
ERR_FAIL_NULL_V_MSG(
_bmp_mem_loader_func,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be _dds_mem_loader_func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed

@lawnjelly
Copy link
Member

This looks great to me, would be useful from a reviewing point of view if you can confirm that the block moved between the files is unchanged so we only have to review the "glue" code.

@marcinn marcinn force-pushed the 3.x-backport-proposal-5748-loading-dds-at-runtime branch from 39b3f15 to 749b808 Compare January 22, 2023 16:08
@marcinn
Copy link
Contributor Author

marcinn commented Jan 22, 2023

would be useful from a reviewing point of view if you can confirm that the block moved between the files is unchanged so we only have to review the "glue" code.

I only changed retvals of ERR_FAIL_COND_V macros (now it is similar to what bmp loader returns):

2023-01-22_17-15

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

I think this is fine (barring the copying right notices for static checks). It seems to copy the established techniques from the other image loaders, and the actual DDS loading is unchanged.

I've been testing this out with some DDS files, and noticed a couple of problems with the DDS loader (nothing to do with this PR):

  • It can't handle incomplete mipmap chains, which do occur in the wild (I may be able to PR to fix this later as I have the fix working locally)
  • It can't handle cubemaps. This would be nice to handle, but the image->load() system currently assumes one image for one load and might require some alternative system for loading multiple images in one go (Image doesn't seem to have support for layers as far as I saw).

@akien-mga akien-mga force-pushed the 3.x-backport-proposal-5748-loading-dds-at-runtime branch from 749b808 to 22468ea Compare February 17, 2023 10:28
@akien-mga
Copy link
Member

Amended myself to fix style issues.

@marcinn
Copy link
Contributor Author

marcinn commented Feb 17, 2023

Amended myself to fix style issues.

Thanks. To not to forget, would you do similar changes for #69085, which is shifted to 4.x for now?

@akien-mga
Copy link
Member

Ah right, I missed that this was backporting a change which hasn't been merged in master yet. I can do the same change there.

I'd probably wait for the master PR to be merged (can be shortly after the 4.0 release) before merging the 3.x one, for consistency.

@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga changed the title [3.x] Backport implement loading DDS textures at run-time [3.x] Implement loading DDS textures at run-time Aug 2, 2023
@TokisanGames
Copy link
Contributor

Should this be reverted? See #81126

@akien-mga
Copy link
Member

akien-mga commented Sep 1, 2023

For the time being yes.

Edit: Done with 54738d3.

@akien-mga akien-mga changed the title [3.x] Implement loading DDS textures at run-time [3.x] Implement loading DDS textures at run-time (reverted) Sep 1, 2023
@RockyMadio
Copy link

I am not able to insert any DDS ( converted by Converseen), the file exists in the project, but not able to display itself in any Sprite or TextureRect.
How did you do it?

@TokisanGames
Copy link
Contributor

We have hundreds of DDS files, both in GD3 and GD4. What happens when you double click the DDS file in the file panel? It should load in the inspector and tell you the format, like this. What do you see?

image

I convert with intel's photoshop dds plugin, nvdia's texture tools, and the gimp.
Which exact format of DDS did you convert to?
Can you reopen your DDS file in photoshop+dds plugin or gimp?

@RockyMadio
Copy link

We have hundreds of DDS files, both in GD3 and GD4. What happens when you double click the DDS file in the file panel? It should load in the inspector and tell you the format, like this. What do you see?

image

I convert with intel's photoshop dds plugin, nvdia's texture tools, and the gimp. Which exact format of DDS did you convert to? Can you reopen your DDS file in photoshop+dds plugin or gimp?

yes, i could open them in gimp again. in Godot 3.5 and 3.6, it appears just as a red X

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.

6 participants