Added support for bc6h format #14

theKlaxon · 2024-08-20T11:23:08Z

Added bc6h compression function.
Added bc6h to all enums + lists.
Updated Compressonator to 4.5.52 for proper bc6h support.

- Added bc6h compression function. - Added bc6h to all enums + lists. - Updated Compressonator to 4.5.52 for proper bc6h support.

craftablescience · 2024-08-20T13:23:41Z

The prebuilt static library for Linux will need to be updated as well, I have builds of the latest version at https://github.com/craftablescience/compressonator-compiler/releases/tag/v4.5-f4b53d7-2 if you need it

theKlaxon · 2024-08-20T13:46:18Z

You got it, I'll make the change!

SCell555 · 2024-08-20T14:03:12Z

Just to note, IMAGE_FORMAT_BC6H is signed BC6H.

JJL772

Thanks for this work! I have a couple small comments/questions.

JJL772 · 2024-08-20T15:46:02Z

VTFLib/VTFFormat.h

@@ -95,6 +95,7 @@ typedef enum tagVTFImageFormat
 	IMAGE_FORMAT_ATI2N,						//!<  = Red, Green BC5 compressed format - 8 bpp
 	IMAGE_FORMAT_ATI1N,						//!<  = Red BC4 compressed format - 4 bpp

+	IMAGE_FORMAT_BC6H = 69,					//!<  = Red, Green, Blue, BC6H compressed format - 8 bpp


IMAGE_FORMAT_BC6H should be 71

Cool, I'll change the rest of the enums / lists to reflect that also.

JJL772 · 2024-08-20T15:46:52Z

VTFLib/VTFFile.cpp

@@ -3556,7 +3629,7 @@ static SVTFImageConvertInfo VTFImageConvertInfo[IMAGE_FORMAT_COUNT] =
 	{},
 	{},
 	{},
-	{},
+	{	  8,  0,  0,  0,  0,  0,	-1, -1, -1, -1,	 vlTrue, vlTrue,	NULL, NULL,			IMAGE_FORMAT_BC6H},


This will need to get moved to after IMAGE_FORMAT_BC7 when you update the enum (see the later comment)

JJL772 · 2024-08-20T15:48:03Z

VTFLib/VTFFile.cpp

@@ -3031,7 +3032,7 @@ static SVTFImageFormatInfo VTFImageFormatInfo[] =
 	{},
 	{},
 	{},
-	{},
+	{ "BC6H",					8,  0,  0,  0,  0,  0, vlTrue,  vlTrue  },			// IMAGE_FORMAT_BC6H


This will need to get moved to after IMAGE_FORMAT_BC7 when you update the enum (see the later comment)

JJL772 · 2024-08-20T15:51:23Z

VTFLib/VTFFile.cpp

+	srcTexture.dwWidth = uiWidth;
+	srcTexture.dwHeight = uiHeight;
+	srcTexture.dwPitch = 4 * uiWidth;
+	srcTexture.format = CMP_FORMAT_RGBA_8888;


Is the input to this function always assumed to be RGBA8888?

ah, it was while i was testing things out. ill go ahead and add the SourceFormat param back to make sure an edge case isn't hit.

My issue with this code is less about assuming lpSource is always RGBA8888 and more about the potential data loss. If textures are getting squished down to RGBA8888 before being turned into BC6H, what's the point of using BC6H at all

This is still blocking merge, basically what JJ said

theKlaxon · 2024-08-20T16:41:19Z

Just to note, IMAGE_FORMAT_BC6H is signed BC6H.

Would you like me to note it at the end of the format as IMAGE_FORMAT_BC6H_SF ?

SCell555 · 2024-08-21T04:04:52Z

No, since there won't be an unsigned version. And we don't need to be weird with format naming like compressonator.

theKlaxon · 2024-08-21T13:36:03Z

Alright, I'll at least add a note that it's signed in the format note in the code.

- Moved BC6H format to 71 - Fixed incorrect dwDataSize given to midTexture in VTFFile.cpp

- Updated CMakeLists.txt to reflect new dependencies.

CMakeLists.txt

Co-authored-by: craftablescience <lauralewisdev@gmail.com>

craftablescience

I mostly just have a few nitpicks, once JJ's comment and pthread stuff gets resolved this is good for merge

craftablescience · 2024-09-24T20:49:48Z

CMakeLists.txt

+				CMP_Core_AVX
+				CMP_Core_AVX512
+				CMP_Core_SSE
+				pthread


use cmake's Threads::Threads target instead of pthread

from stackoverflow:

set(CMAKE_THREAD_PREFER_PTHREAD ON) set(THREADS_PREFER_PTHREAD_FLAG ON) find_package(Threads REQUIRED) add_executable(test test.cpp) target_link_libraries(test Threads::Threads)

also, is this actually required? i cant remember any vtflib code that uses pthreads or any threading stuff in std

craftablescience · 2024-09-24T20:49:53Z

CMakeLists.txt

+				CMP_Core_AVX
+				CMP_Core_AVX512
+				CMP_Core_SSE
+				pthread


craftablescience · 2024-09-24T20:52:44Z

VTFLib/VTFFile.cpp

@@ -440,6 +440,7 @@ static CMP_FORMAT GetCMPFormat( VTFImageFormat imageFormat, bool bDXT5GA )
 	// Swizzle is technically wrong for below but we reverse it in the shader!
 	case IMAGE_FORMAT_ATI2N:			return CMP_FORMAT_ATI2N;

+	case IMAGE_FORMAT_BC6H:				return CMP_FORMAT_BC6H;


nit: should come after BC7

craftablescience · 2024-09-24T20:53:02Z

VTFLib/VTFFile.cpp

@@ -3066,6 +3068,7 @@ vlUInt CVTFFile::ComputeImageSize(vlUInt uiWidth, vlUInt uiHeight, vlUInt uiDept
 	case IMAGE_FORMAT_DXT3:
 	case IMAGE_FORMAT_DXT5:
 	case IMAGE_FORMAT_ATI2N:
+	case IMAGE_FORMAT_BC6H:


nit: should be after BC7

craftablescience · 2024-09-24T20:53:35Z

VTFLib/VTFFile.cpp

+	srcTexture.dwWidth = uiWidth;
+	srcTexture.dwHeight = uiHeight;
+	srcTexture.dwPitch = 4 * uiWidth;
+	srcTexture.format = CMP_FORMAT_RGBA_8888;


This is still blocking merge, basically what JJ said

craftablescience · 2024-09-24T20:53:53Z

VTFLib/VTFFile.cpp

@@ -3857,6 +3932,7 @@ vlBool CVTFFile::Convert(const vlByte *lpSource, vlByte *lpDest, vlUInt uiWidth,
 		case IMAGE_FORMAT_DXT5:
 		case IMAGE_FORMAT_ATI2N:
 		case IMAGE_FORMAT_ATI1N:
+		case IMAGE_FORMAT_BC6H:


nit: should be below.. yeah you get the idea

craftablescience · 2024-09-24T20:54:23Z

VTFLib/VTFFormat.h

@@ -94,8 +94,9 @@ typedef enum tagVTFImageFormat

 	IMAGE_FORMAT_ATI2N,						//!<  = Red, Green BC5 compressed format - 8 bpp
 	IMAGE_FORMAT_ATI1N,						//!<  = Red BC4 compressed format - 4 bpp
-
+	


nit: extra space

Added support for bc6h format

bdb9b34

- Added bc6h compression function. - Added bc6h to all enums + lists. - Updated Compressonator to 4.5.52 for proper bc6h support.

JJL772 requested changes Aug 20, 2024

View reviewed changes

theKlaxon added 3 commits August 21, 2024 14:26

Updated win and linux to latest compressonator

43e552a

- Moved BC6H format to 71 - Fixed incorrect dwDataSize given to midTexture in VTFFile.cpp

Added necessary linux libs and tested compilation

3ddae1e

- Updated CMakeLists.txt to reflect new dependencies.

Fixed small linux bug with RGBA_16 dwPitch

1c0563a

craftablescience reviewed Aug 22, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

theKlaxon and others added 4 commits August 22, 2024 20:17

Removed redundant 'public' tag in CMakeLists.txt

1a91251

Co-authored-by: craftablescience <lauralewisdev@gmail.com>

Removed redundant 'public' tag in CMakeLists.txt

96c5591

Co-authored-by: craftablescience <lauralewisdev@gmail.com>

Updated to Craftablescience's linux libs

3c5e0b4

Removed CMP_Framework

b1e59a5

craftablescience requested changes Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for bc6h format #14

Added support for bc6h format #14

theKlaxon commented Aug 20, 2024

craftablescience commented Aug 20, 2024

theKlaxon commented Aug 20, 2024

SCell555 commented Aug 20, 2024

JJL772 left a comment

JJL772 Aug 20, 2024

theKlaxon Aug 20, 2024

JJL772 Aug 20, 2024

theKlaxon Aug 20, 2024

JJL772 Aug 20, 2024

theKlaxon Aug 20, 2024

JJL772 Aug 20, 2024

theKlaxon Aug 20, 2024

JJL772 Aug 23, 2024

craftablescience Sep 24, 2024

theKlaxon commented Aug 20, 2024

SCell555 commented Aug 21, 2024 •

edited

Loading

theKlaxon commented Aug 21, 2024

craftablescience left a comment

craftablescience Sep 24, 2024 •

edited

Loading

craftablescience Sep 24, 2024

craftablescience Sep 24, 2024

craftablescience Sep 24, 2024

craftablescience Sep 24, 2024

craftablescience Sep 24, 2024

craftablescience Sep 24, 2024

		@@ -94,8 +94,9 @@ typedef enum tagVTFImageFormat

		IMAGE_FORMAT_ATI2N, //!< = Red, Green BC5 compressed format - 8 bpp
		IMAGE_FORMAT_ATI1N, //!< = Red BC4 compressed format - 4 bpp

Added support for bc6h format #14

Are you sure you want to change the base?

Added support for bc6h format #14

Conversation

theKlaxon commented Aug 20, 2024

craftablescience commented Aug 20, 2024

theKlaxon commented Aug 20, 2024

SCell555 commented Aug 20, 2024

JJL772 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theKlaxon commented Aug 20, 2024

SCell555 commented Aug 21, 2024 • edited Loading

theKlaxon commented Aug 21, 2024

craftablescience left a comment

Choose a reason for hiding this comment

craftablescience Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCell555 commented Aug 21, 2024 •

edited

Loading

craftablescience Sep 24, 2024 •

edited

Loading