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

Fix squish RGTC_R decompression corruption #85863

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

BlueCube3310
Copy link
Contributor

The custom Godot code for squish only allows for decompressing BC5 (RGTC_RG) textures. This means attempting to decompress BC4-compressed images (for the minature texture views, for example) would result in corrupted images:

master PR
aa bb

An MRP for testing the changes: RGTCSquishError.zip
Steps to reproduce:

  1. Load the project and open Main.tscn
  2. Select the mesh instance and look at the Albedo texture preview in the material inspector.

@BlueCube3310 BlueCube3310 requested a review from a team as a code owner December 6, 2023 19:11
@BlueCube3310 BlueCube3310 force-pushed the squish-rgtc-r-error branch 2 times, most recently from 3119db5 to 8a95901 Compare December 6, 2023 19:16
@clayjohn clayjohn added this to the 4.3 milestone Dec 6, 2023
@clayjohn
Copy link
Member

clayjohn commented Dec 6, 2023

Oh wow, doing this was brought up 6 years ago... #13025 (comment)

Copy link
Member

@clayjohn clayjohn 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. Tested locally and it works.

Screenshot from 2023-12-06 13-35-19

One thing to note is that the generated preview for the file system doesn't update (even after deleting the .godot folder). So some users may be surprised to see the corruption in the file system preview

Screenshot from 2023-12-06 13-35-24

@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 6, 2023
@akien-mga
Copy link
Member

One thing to note is that the generated preview for the file system doesn't update (even after deleting the .godot folder). So some users may be surprised to see the corruption in the file system preview

File thumbnails are stored outside the project, in the user cache folder. On Linux it's in ~/.cache/godot/.

@akien-mga
Copy link
Member

Could you add your changes to the existing thirdparty/squish/godot-changes.patch so that it fully documents the changes made for Godot?

This should do the trick (also included a minor comment style change for consistency):

diff --git a/thirdparty/squish/colourblock.cpp b/thirdparty/squish/colourblock.cpp
index 49401358bc..f14c9362bd 100644
--- a/thirdparty/squish/colourblock.cpp
+++ b/thirdparty/squish/colourblock.cpp
@@ -24,9 +24,9 @@
    -------------------------------------------------------------------------- */
 
 #include "colourblock.h"
-// -- Godot start --
+// -- GODOT start --
 #include "alpha.h"
-// -- Godot end --
+// -- GODOT end --
 
 namespace squish {
 
diff --git a/thirdparty/squish/godot-changes.patch b/thirdparty/squish/godot-changes.patch
index ef7bafb4b4..655a8cffc2 100644
--- a/thirdparty/squish/godot-changes.patch
+++ b/thirdparty/squish/godot-changes.patch
@@ -1,22 +1,33 @@
 diff --git a/thirdparty/squish/colourblock.cpp b/thirdparty/squish/colourblock.cpp
-index af8b98036..3d87adaa7 100644
+index af8b980365..f14c9362bd 100644
 --- a/thirdparty/squish/colourblock.cpp
 +++ b/thirdparty/squish/colourblock.cpp
 @@ -24,6 +24,9 @@
     -------------------------------------------------------------------------- */
  
  #include "colourblock.h"
-+// -- Godot start --
++// -- GODOT start --
 +#include "alpha.h"
-+// -- Godot end --
++// -- GODOT end --
  
  namespace squish {
  
-@@ -211,4 +214,23 @@ void DecompressColour( u8* rgba, void const* block, bool isDxt1 )
+@@ -211,4 +214,34 @@ void DecompressColour( u8* rgba, void const* block, bool isDxt1 )
      }
  }
  
-+// -- Godot start --
++// -- GODOT start --
++void DecompressColourBc4( u8* rgba, void const* block)
++{
++    DecompressAlphaDxt5(rgba,block);
++    for ( int i = 0; i < 16; ++i ) {
++        rgba[i*4] = rgba[i*4 + 3];
++		rgba[i*4 + 1] = 0;
++		rgba[i*4 + 2] = 0;
++        rgba[i*4 + 3] = 255;
++    }
++}
++
 +void DecompressColourBc5( u8* rgba, void const* block)
 +{
 +    void const* rblock = block;
@@ -37,21 +48,22 @@ index af8b98036..3d87adaa7 100644
 +
  } // namespace squish
 diff --git a/thirdparty/squish/colourblock.h b/thirdparty/squish/colourblock.h
-index fee2cd7c5..3cb9b7e3b 100644
+index fee2cd7c5d..e1eb9e4917 100644
 --- a/thirdparty/squish/colourblock.h
 +++ b/thirdparty/squish/colourblock.h
-@@ -35,6 +35,9 @@ void WriteColourBlock3( Vec3::Arg start, Vec3::Arg end, u8 const* indices, void*
+@@ -35,6 +35,10 @@ void WriteColourBlock3( Vec3::Arg start, Vec3::Arg end, u8 const* indices, void*
  void WriteColourBlock4( Vec3::Arg start, Vec3::Arg end, u8 const* indices, void* block );
  
  void DecompressColour( u8* rgba, void const* block, bool isDxt1 );
 +// -- GODOT start --
++void DecompressColourBc4( u8* rgba, void const* block );
 +void DecompressColourBc5( u8* rgba, void const* block );
 +// -- GODOT end --
  
  } // namespace squish
  
 diff --git a/thirdparty/squish/config.h b/thirdparty/squish/config.h
-index 92edefe96..05f8d7259 100644
+index 92edefe966..05f8d72598 100644
 --- a/thirdparty/squish/config.h
 +++ b/thirdparty/squish/config.h
 @@ -32,6 +32,26 @@
@@ -82,17 +94,19 @@ index 92edefe96..05f8d7259 100644
  #define SQUISH_USE_SSE 0
  #endif
 diff --git a/thirdparty/squish/squish.cpp b/thirdparty/squish/squish.cpp
-index 1d22a64ad..fd11a147d 100644
+index 1d22a64ad6..086ba11cd0 100644
 --- a/thirdparty/squish/squish.cpp
 +++ b/thirdparty/squish/squish.cpp
-@@ -135,7 +135,13 @@ void Decompress( u8* rgba, void const* block, int flags )
+@@ -135,7 +135,15 @@ void Decompress( u8* rgba, void const* block, int flags )
          colourBlock = reinterpret_cast< u8 const* >( block ) + 8;
  
      // decompress colour
 -    DecompressColour( rgba, colourBlock, ( flags & kDxt1 ) != 0 );
 +    // -- GODOT start --
 +    //DecompressColour( rgba, colourBlock, ( flags & kDxt1 ) != 0 );
-+    if(( flags & ( kBc5 ) ) != 0)
++    if(( flags & ( kBc4 ) ) != 0)
++        DecompressColourBc4( rgba, colourBlock);
++	else if(( flags & ( kBc5 ) ) != 0)
 +        DecompressColourBc5( rgba, colourBlock);
 +    else
 +        DecompressColour( rgba, colourBlock, ( flags & kDxt1 ) != 0 );

@akien-mga
Copy link
Member

You missed the first chunk of my diff changing the style of the "Godot changes" comment at the top of thirdparty/squish/colourblock.cpp.

@YuriSizov YuriSizov merged commit 13f6c68 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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.

4 participants