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

[4.2] Godot broke DDS, converts when it shouldn't #190

Closed
Calinou opened this issue Aug 21, 2023 · 20 comments
Closed

[4.2] Godot broke DDS, converts when it shouldn't #190

Calinou opened this issue Aug 21, 2023 · 20 comments
Labels
not a bug Not a bug waiting for godot Bug or missing feature in the engine
Milestone

Comments

@Calinou
Copy link

Calinou commented Aug 21, 2023

OS: Fedora 38
GPU: GeForce RTX 4090 (NVIDIA 535.98)
Godot version: 4.2.dev c495eb510
Terrain3D version: https://github.com/outobugi/Terrain3D/releases/tag/v0.8.2-alpha_gd4.1.1 (using Linux binary release)

Mipmap rendering doesn't work, resulting in grainy textures at a distance in the demo project:

image

Textures up close look smooth, with bilinear filtering applied.

Anisotropic Filtering Level and Texture Mipmap Bias project settings are at their default value in the demo project.

The DDS textures in demo/textures are detected as not having any mipmaps:

image

It seems enabling Mipmaps > Generate on them in the Import dock then saving and reloading the scene fixes the issue.

PS: This could be a good opportunity to set Anisotropic Filtering Level to 16× in the demo project 🙂

@TokisanGames
Copy link
Owner

My copy of the DDS files in the demo do have mipmaps, on all 4 textures. These are the exact same files I zipped up for the linux release. I have no graininess up close or far away. You've seen this in my latest YT video. I've also not seen any in any pics from users.
image

Also DDS files do not have any import settings in Godot, and never have from GD3 through 4.1.1. The import panel is entirely blank for DDS files. Are you sure you're looking at our included DDS files and not some PNG files, which do have import settings?

I can look at bumping up anisotropic filtering on the demo and see how it impacts visuals or performance.

@Calinou
Copy link
Author

Calinou commented Aug 21, 2023

Also DDS files do not have any import settings in Godot, and never have from GD3 through 4.1.1. The import panel is entirely blank for DDS files. Are you sure you're looking at our included DDS files and not some PNG files, which do have import settings?

I've just tested this on 4.1.1. I can see options in the Import dock after selecting one in the FileSystem dock:

image

@TokisanGames
Copy link
Owner

TokisanGames commented Aug 21, 2023

Hmm, on windows there has never been anything in the import panel for DDS files, and no support for import files. In fact, you and I talked about it wayyyy back here in GD#50900

image

Please open one of these dds files in Gimp. On my windows system it asks if I want to load mipmaps. Then on Layers it displays layers for the main and 10 others.

image

If you don't see this, please send me your albedo_ht dds file and I'll compare it with mine. If you do see all layers, then you sir have a bug in your DDS library on linux not detecting mipmaps that exist in the file.

@Calinou
Copy link
Author

Calinou commented Aug 21, 2023

These are the layers I see (excuse the tiny font in GIMP, it's a system issue):

image

The demo download doesn't include *.import files for the DDS images, but here are the ones I have locally: textures_import_files.zip

@TokisanGames
Copy link
Owner

TokisanGames commented Aug 21, 2023

Gimp is correct then. The DDS file already has mipmaps in it. On Windows Godot there are no options for importing. It has never generated import files because it does not convert them to CTEX. It reads the DDS as is, detecting the mipmaps and using the texture natively. That is why we use DDS because they are already in DXT format. Godot has some bugs with the automatic DXT conversion.

On your system is the first time ever that I've seen Godot have import options for DDS files and it not automatically detect the mipmaps that are already in there. Your import file shows that your godot converted the file:
res://.godot/imported/ground037_alb_ht.dds-465fdc1deba1c0c06d4b6c6d8226eacf.ctex

I do not have any ctex files in .godot/imported for my dds files. Not in the demo, and not in Out of the Ashes (out of 800 dds files). They are used natively. Either you have some project settings telling Godot to convert the file again that should be disabled, or there is a bug in Linux Godot that causes this. There's absolutely no reason for it to convert the file.

Also on windows for an importable file like PNG, when my Import panel is set to Keep File(no import) the import panel goes blank. On your image, your import panel is set to Keep File (no import) yet you have all of the settings of an import. Seems to me that linux importing has a bug or two.

image

Image

@lw64
Copy link
Contributor

lw64 commented Aug 21, 2023

How did you install godot? Fedora package or flatpak, or something else?

@TokisanGames
Copy link
Owner

@lw64 You use Linux don't you? I assume by your comment you didn't have the same issue? If you double click a demo dds in godot, does it show 10 mipmaps? Does the demo have grainy or normal textures?

@lw64
Copy link
Contributor

lw64 commented Aug 22, 2023

No I didnt have any of these issues. godot shows import settings for dds files for me and its all correct. No graininess.

@TokisanGames
Copy link
Owner

I've also heard no complaints and one other positive that it's fine on Linux.
https://twitter.com/GoDevNZ1/status/1693875745402356102?t=CBIlw883Btnn8V6TL6mGaQ&s=19

Sorry @Calinou this is something on your system not detecting the mipmaps that are there in the file.

@TokisanGames TokisanGames added the not a bug Not a bug label Aug 22, 2023
@TokisanGames
Copy link
Owner

@Calinou I just loaded my project in 4.2-dev build of Godot and see grainy textures. I discovered that 4.2-dev decided to import and convert all of my dds files. Yikes. It spent a few minutes doing so and finally hung. I had to force it closed.

Godot's auto conversion to DXT format already had bugs in it. Now it's converting working DXT files to broken DXT files! My terrain textures are all grainy and report that they do not have mipmaps, even though they do.

Here's what I think happened to you. Someone broke DDS in Godot 4.2. You were running Terrain3D with 4.2 which modified the DDS files and created import files. Then you switched back to 4.1 which read the import files and tried to read the ctex files. If you delete all of your dds import files I bet it will work normally in Godot 4.1.

Do you know of which PRs and tickets that modified DDS recently? These changes need to be fixed.

Thanks

@TokisanGames TokisanGames reopened this Aug 29, 2023
@TokisanGames TokisanGames added the waiting for godot Bug or missing feature in the engine label Aug 29, 2023
@TokisanGames TokisanGames changed the title Mipmap rendering doesn't work in the demo project due to textures not being imported with mipmaps enabled [4.2] Godot broke DDS, converts when it shouldn't Aug 29, 2023
@Calinou
Copy link
Author

Calinou commented Aug 29, 2023

Here's what I think happened to you. Someone broke DDS in Godot 4.2. You were running Terrain3D with 4.2 which modified the DDS files and created import files. Then you switched back to 4.1 which read the import files and tried to read the ctex files. If you delete all of your dds import files I bet it will work normally in Godot 4.1.

This could be due to either godotengine/godot#69085 or godotengine/godot#76572.

cc @marcinn

@TokisanGames
Copy link
Owner

TokisanGames commented Aug 29, 2023

@Calinou I will bisect those commits and figure out which caused the issue.

Repository owner deleted a comment from CarpenterBlue Aug 29, 2023
@TokisanGames
Copy link
Owner

It was indeed caused by godotengine/godot#69085.

@marcinn1
Copy link

marcinn1 commented Aug 29, 2023

It was indeed caused by godotengine/godot#69085.

I don't understand why adding ability to load dds at runtime causes reimporting DDS in a broken way. The DDS import code has been moved to a separate file (it is reused in load_dds_from_buffer and ResourceFormatDDS::load), but its logic remains intact.

I assume the issue must be related some way, but the bug is elsewhere. With MRP I could try to find the problem and fix the bug, but it will take a few days since I'm on vacation now.

@TokisanGames
Copy link
Owner

It was confirmed caused by your PR by bisecting from 4.1 to Master. Sure it may be a bug elsewhere in the engine that was only revealed once your PR was introduced. Let's follow up on your PR so we don't have two conversations occurring.

@TokisanGames
Copy link
Owner

Merging godotengine/godot#81126 will revert the PR and close this issue.

@TokisanGames TokisanGames modified the milestones: Stable, Beta Aug 29, 2023
@marcinn1
Copy link

marcinn1 commented Sep 1, 2023

In fact, you and I talked about it wayyyy back here in GD#50900

So it looks like Godot 3.x is still "broken" (but everyone forgot about that), and the problem in Godot 4 was probably "solved" by accident. I'm still thinking about the issue my PR created and how to fix it (I need DDS loading at runtime -> my project is dead again after a year of useless waiting), and I feel like it just showed (unintentionally!) a bit of a mess hidden in the code.

@TokisanGames
Copy link
Owner

TokisanGames commented Sep 1, 2023

@marcinn1 This is Terrain3D, and you'll probably want to bring up the issue in the Godot repo. However since you're here please reread this comment.

The original proposal was for loading DDS files on the CPU as Images. In Terrain3D we operate predominantly with Images and we recommend all of our users use DDS files only, due to Godot's bugs in DXT conversion. We pull images from the textures, then convert them back into TextureArrays. It does waste cycles, but it is functional and only requires a quick, one-time operation on load. Also export works fine. So while I'd be happy to reduce wasted cycles and load DDS files as Images directly, sorry to say that this PR is not required even for an editor plugin that relies completely on DDS files.
godotengine/godot#69085 (comment)

You don't need to wait on your project. You can load your dds files as Images at run time. We do. Just not directly. Use Texture2D.get_image(). Feel free to look at our code.

@marcinn1
Copy link

marcinn1 commented Sep 1, 2023

Thanks for a hint. I'll try to load dds that way.

Ps sorry for double posting - I have trouble with GH

@marcinn1
Copy link

marcinn1 commented Sep 1, 2023

This is Terrain3D, and you'll probably want to bring up the issue in the Godot repo. However since you're here please reread this comment.r p

No need to reread. I understand why it is important.

I will not bring any issues for Godot repo - it is a waste of time. I'm pretty sure that ResourceFormatDDS::load is used for every dds under the hood, also when "keep file / no import" is set. It creates ImageTexture. I'm almost sure that Terrain3D is based on somehow broken DDS handling in Godot, and it will work only until Godot is fixed or all changes to dds handling are reverted. You're expecting that dds will be always imported as "no import". In fact you should have a easy way to request such default behaviour, because defaults can always change.

I have 18k+ external dds to load on demand at runtime. Currently there is no other way to do that than by using ResourceFormatDDS:load, which is not exposed to GDScript and it is called only at import stage. But I have no DDS files inside my project. I'm not importing them. I need a way to create ImageTexture from buffer. That's why I moved dds handling code outside, and the side and unexpected effect was a change of default import mode of DDS.

I understand that my patch broke backward compatibility, but what I am trying to say is that your requirements are based on something that may change in the future. And Instead of fixing the issue, a whole possibility to load dds from external files was completely reverted. Everything is fine now, but I'm off.

Good luck with Terrain3D.
Marcin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug Not a bug waiting for godot Bug or missing feature in the engine
Projects
None yet
Development

No branches or pull requests

4 participants