tr_image: do not use R_FindImageLoader with cube maps, test multifile cubemaps first (or preview may be loaded as cubemap instead) #1394

illwieckz · 2024-10-26T15:17:42Z

Deep rewrite of image loading code.

do not walk .shader files in tr_image.cpp
do not use R_FindImageLoader with cube maps
only use home read for KTX with IF_HOMEPATH
load home KTX with IF_HOMEPATH before doing any lookup
do not do any other home loading
test multifile cubemaps before singlefile ones (or preview may be loaded as cubemap instead)
fix a crash when singlefile cubemap is freed because it was loaded but not created
look for cubemap file with explicit extension
add many debug logs and factorize them

Broken wasteland skybox #1393

- do not walk .shader files in tr_image.cpp - do not use R_FindImageLoader with cube maps - only use home read for KTX with IF_HOMEPATH - load home KTX with IF_HOMEPATH before doing any lookup - do not do any other home loading - test multifile cubemaps before singlefile ones (or preview may be loaded as cubemap instead) - fix a crash when singlefile cubemap is freed because it was loaded but not created - look for cubemap file with explicit extension - add many debug logs and factorize them

src/engine/renderer/tr_image.cpp

slipher · 2024-10-26T19:07:07Z

src/engine/renderer/tr_image.cpp

+			foundLoader = true;
+		}
+
+		if ( pak == nullptr ) {


We don't want this homepath thing (yes it should be deleted from the other one too)

@VReaperV Is it used to load cached reflection cubemaps?

The one in R_FindImageLoader() is.

Well, something is wrong, R_FindImageLoader() is not expected to be used to load a cached cubemap.

For now I'll keep the home loader in both, then later we can decide what is to be kept when things are not weird.

🔹️ xreaperx: Well, I thought you were asking about the homepath usage in the loader?
🔹️ xreaperx: I added it so it would look for .ktx under homepath
🔹️ illwieckz: yes
🔹️ illwieckz: but you said only R_FindImageLoader requires to read home path
🔹️ xreaperx: Well yeah otherwise it doesn't find the files
🔹️ illwieckz: but this is a cubemap
🔹️ illwieckz: R_FindImageLoader is for non-cubemap files
🔹️ xreaperx: Yeah there's some weirdness with that

🔹️ xreaperx: @illwieckz R_FindCubeImage() use R_FindImageLoader(), not R_FindCubeImageLoader()
🔹️ xreaperx: So if that's not what it should do, then the error is in that

I say: we merge this that fixes the skybox, then later we fix the reflection cubemap code.

Sounds reasonable to me. I don't know if there's supposed to be some other way to load images from homepath, but other functionality that I looked at (log, glsl cache) used FS::HomePath.

This is not the code that's used for reflection maps. The reflection map search sets an IF_HOMEPATH flag instructing to search the homepath instead of the VFS. It's bad to search the homepath for every random image.

Yes and no. loader.ImageLoader() is the only call that uses IF_HOMEPATH, but R_FindImageLoader(), which preceeds it, doesn't use that, and is used to know if the file exists in the first place. I suppose a proper solution might be to pass imageParams.bits to it and then use IF_HOMEPATH.

The purpose of this PR is to fix skybox loading, other things like “attempting to remove some code and see if that breaks reflections“ later.

slipher · 2024-10-26T19:16:06Z

src/engine/renderer/tr_image.cpp

+	char baseName[ MAX_QPATH ];
+	COM_StripExtension3( buffer, baseName, sizeof( baseName ) );
+
+	const FS::PakInfo* bestPak = nullptr;


Could use at least a TODO comment about respecting explicit extensions.

illwieckz · 2024-10-27T04:01:15Z

I finally reworded the thing in a better way, including the non-cubemap code.

illwieckz · 2024-10-31T09:02:15Z

I added some TODO comments saying the removal of homepath reading have to be investigated later.

VReaperV · 2024-10-31T09:29:12Z

src/engine/renderer/tr_image.cpp


-	if ( *ext && bestLoader == -1 )
+	if ( *ext && loader )


This should be !loader.

illwieckz · 2024-10-31T17:06:17Z

So I did a deeper rewrite.

First, I refactored much code and added more logs. It makes me able to check how and if the cached KTX loading worked.

So I dropped every home loading but cached KTX: now only the cached KTX is loaded from home, nothing else is loaded from home. It even doesn't iterate loaders for them: it does a KTX home dir straight and return.

I actually found a crash that happens when loading a singlefile cubmemap succeeds but image creation fails (I had to simulate this case to verify it would crash).

I also implemented the look-up of singlefile cubemap with explicit extension.

The first post was updated accordingly.

illwieckz · 2024-10-31T17:20:12Z

I noticed some functions were copying with Q_strncpyz strings received as argument before using them. I guess it's some security check in case some user-provided data is intentionally wrong (missing null terminator). I don't know if such copy is needed but I renamed the symbols to make them more obvious.

slipher · 2024-10-31T19:28:19Z

src/engine/renderer/tr_image.cpp

-		// We found a file and its pak is better than the best pak we have
-		// this relies on how the filesystem works internally and should be moved
-		// to a more explicit interface once there is one. (FIXME)
+		// FIXME: Move to a more explicit interface once there is one.


The preceding sentence is important context for this comment. Without it written together, it would be difficult to guess what it's talking about.

slipher · 2024-10-31T21:02:57Z

src/engine/renderer/tr_image.cpp

-	COM_StripExtension3( buffer, cubeMapBaseName, sizeof( cubeMapBaseName ) );
-
-	for ( const cubeMapLoader_t &loader : cubeMapLoaders )
+	// Look for cached 6-face skybox file.


The reflection cubemaps aren't "skyboxes"

slipher · 2024-10-31T21:04:34Z

src/engine/renderer/tr_image.cpp

+	return bestLoader;
+}
+
+static image_t *R_LoadAndCreateCubeImage( const char *imageName, const char* altName, const imageExtLoader_t *loader, imageParams_t& imageParams )


This could use a comment (or function name change) indicating it is only used for single-file variants

slipher · 2024-10-31T21:19:14Z

src/engine/renderer/tr_image.cpp

-	Q_strncpyz( buffer, imageName, sizeof( buffer ) );
-	unsigned hash = GenerateImageHashValue( buffer );
+	char imageName[ 1024 ];
+	Q_strncpyz( imageName, untrustedName, sizeof( imageName ) );


We don't need to make a copy of the function argument. The one concern like that which should be taken care of is that when you use COM_Parse* it returns a pointer to a temporary buffer, which you need to either copy, or finish using immediately.

In fact all the multiple copies (the thing I first thought it was a security check) and this one ware a VERY UGLY workaround for a bug where the .shader file was iterating the multiple images of lines like AnimMap 1.5 models/weapons/lgun/display1 models/weapons/lgun/display2 models/weapons/lgun/display3 models/weapons/lgun/display4 twice: once in tr_shader.cpp, once in tr_image.cpp… So ParseStage from tr_shader.cpp was reading next token, passed it to R_FindImageFile, then R_FindImageFile backuped the token position, passed the token to R_LoadImage that was moving to the next token in the back or ParseStage, etc. And with multi-file cubemaps it meant the image code was iterating next .shader` file token 6 time, requiring to reset the state 6 times… This was all crazy.

slipher · 2024-10-31T23:08:28Z

src/engine/renderer/tr_image.cpp

+				if ( IsImageCompressed( imageParams.bits ) )
+				{
+					Log::Warn("Cube map face '%s' has DXTn compression, cube map unusable", fileName );
+					R_FreeCubePics( pic, i );


Should be i + 1

Done, also done in next free.

slipher · 2024-10-31T23:27:33Z

src/engine/renderer/tr_image.cpp

-				Log::Warn("cubemap face '%s' is a multilayer image with %d layer(s), cube map unusable", filename, numLayers);
-				break;
-			}
+				R_LoadImage( &fileName[ 0 ], &pic[ i ], &width, &height, &numLayers, &numMips, &imageParams.bits );


Just fileName

Done. The various &name[0] were in fact a workaround for the many type discrepancies of the various string copies to workaround the double shader keyword iteration… What a mess!

src/engine/renderer/tr_image.cpp

slipher · 2024-10-31T23:45:47Z

src/engine/renderer/tr_image.cpp

+			}
+		}
+
+		/* If the image path is “some.thing” for a file named “some.thing.crn”,


These comments are written in a confusing way. You could say If we are give the name "some.thing", first we look for some.thing.webp, some.thing.tga etc... Then, some.webp, some.tga, etc.

slipher · 2024-10-31T23:48:38Z

src/engine/renderer/tr_image.cpp

+		try loading with “something” as a basename. */
+		if ( *ext && !loader )
+		{
+			COM_StripExtension3( token, fileName, sizeof(fileName) );


The token temporary pointer shouldn't be used after calling a bunch of other functions.

Removed. In fact the whole token wording was a because of this code parsing the .shader file, which was wrong.

illwieckz

OK, so, lot of that madness code with many copies and pointers were very convoluted workarounds to the fact the image code was walking the .shader file while the shader code did the same and then there was some crazy code done to make this wrongness work.

illwieckz · 2024-11-01T06:42:26Z

src/engine/renderer/tr_image.cpp

-	Q_strncpyz( buffer, imageName, sizeof( buffer ) );
-	unsigned hash = GenerateImageHashValue( buffer );
+	char imageName[ 1024 ];
+	Q_strncpyz( imageName, untrustedName, sizeof( imageName ) );


In fact all the multiple copies (the thing I first thought it was a security check) and this one ware a VERY UGLY workaround for a bug where the .shader file was iterating the multiple images of lines like AnimMap 1.5 models/weapons/lgun/display1 models/weapons/lgun/display2 models/weapons/lgun/display3 models/weapons/lgun/display4 twice: once in tr_shader.cpp, once in tr_image.cpp… So ParseStage from tr_shader.cpp was reading next token, passed it to R_FindImageFile, then R_FindImageFile backuped the token position, passed the token to R_LoadImage that was moving to the next token in the back or ParseStage, etc. And with multi-file cubemaps it meant the image code was iterating next .shader` file token 6 time, requiring to reset the state 6 times… This was all crazy.

illwieckz · 2024-11-01T06:42:38Z

src/engine/renderer/tr_image.cpp

+	return bestLoader;
+}
+
+static image_t *R_LoadAndCreateCubeImage( const char *imageName, const char* altName, const imageExtLoader_t *loader, imageParams_t& imageParams )


illwieckz · 2024-11-01T06:42:53Z

src/engine/renderer/tr_image.cpp

-	COM_StripExtension3( buffer, cubeMapBaseName, sizeof( cubeMapBaseName ) );
-
-	for ( const cubeMapLoader_t &loader : cubeMapLoaders )
+	// Look for cached 6-face skybox file.


illwieckz · 2024-11-01T06:44:41Z

src/engine/renderer/tr_image.cpp

-				Log::Warn("cubemap face '%s' is a multilayer image with %d layer(s), cube map unusable", filename, numLayers);
-				break;
-			}
+				R_LoadImage( &fileName[ 0 ], &pic[ i ], &width, &height, &numLayers, &numMips, &imageParams.bits );


Done. The various &name[0] were in fact a workaround for the many type discrepancies of the various string copies to workaround the double shader keyword iteration… What a mess!

src/engine/renderer/tr_image.cpp

illwieckz · 2024-11-01T06:45:55Z

src/engine/renderer/tr_image.cpp

+				if ( IsImageCompressed( imageParams.bits ) )
+				{
+					Log::Warn("Cube map face '%s' has DXTn compression, cube map unusable", fileName );
+					R_FreeCubePics( pic, i );


Done, also done in next free.

illwieckz · 2024-11-01T06:46:45Z

src/engine/renderer/tr_image.cpp

+		try loading with “something” as a basename. */
+		if ( *ext && !loader )
+		{
+			COM_StripExtension3( token, fileName, sizeof(fileName) );


Removed. In fact the whole token wording was a because of this code parsing the .shader file, which was wrong.

slipher · 2024-11-02T19:50:13Z

LGTM

illwieckz added T-Regression A-Renderer labels Oct 26, 2024

VReaperV reviewed Oct 26, 2024

View reviewed changes

src/engine/renderer/tr_image.cpp Show resolved Hide resolved

slipher reviewed Oct 26, 2024

View reviewed changes

illwieckz force-pushed the illwieckz/skyboxes branch from 85febb1 to b1819eb Compare October 27, 2024 04:00

illwieckz force-pushed the illwieckz/skyboxes branch from b1819eb to 4a9b464 Compare October 27, 2024 22:07

illwieckz mentioned this pull request Oct 29, 2024

0.55.1 pre-release thread (stuff to do to be shipped in the release) Unvanquished/Unvanquished#3165

Closed

illwieckz force-pushed the illwieckz/skyboxes branch from 4a9b464 to 3cc6ef4 Compare October 31, 2024 06:25

VReaperV reviewed Oct 31, 2024

View reviewed changes

src/engine/renderer/tr_image.cpp Outdated

if ( *ext && bestLoader == -1 )

if ( *ext && loader )

Copy link

Contributor

VReaperV Oct 31, 2024

Choose a reason for hiding this comment

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

This should be !loader.

illwieckz force-pushed the illwieckz/skyboxes branch 6 times, most recently from 13cb6ae to e958009 Compare October 31, 2024 16:58

illwieckz force-pushed the illwieckz/skyboxes branch 2 times, most recently from 7ef328f to eab6a6a Compare October 31, 2024 17:18

illwieckz force-pushed the illwieckz/skyboxes branch from eab6a6a to fe3df0b Compare October 31, 2024 17:24

slipher reviewed Oct 31, 2024

View reviewed changes

illwieckz force-pushed the illwieckz/skyboxes branch 2 times, most recently from c40fc19 to 58c3631 Compare November 1, 2024 06:50

illwieckz commented Nov 1, 2024

View reviewed changes

illwieckz merged commit ef89e9b into master Nov 2, 2024
9 checks passed

illwieckz deleted the illwieckz/skyboxes branch November 2, 2024 20:07

tr_image: do not use R_FindImageLoader with cube maps, test multifile cubemaps first (or preview may be loaded as cubemap instead) #1394

tr_image: do not use R_FindImageLoader with cube maps, test multifile cubemaps first (or preview may be loaded as cubemap instead) #1394

Conversation

illwieckz commented Oct 26, 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

illwieckz Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

illwieckz Oct 27, 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

illwieckz commented Oct 27, 2024

illwieckz commented Oct 31, 2024

Choose a reason for hiding this comment

illwieckz commented Oct 31, 2024

illwieckz commented Oct 31, 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

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

illwieckz 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

slipher commented Nov 2, 2024

illwieckz commented Oct 26, 2024 •

edited

Loading

illwieckz Oct 27, 2024 •

edited

Loading

illwieckz Oct 27, 2024 •

edited

Loading

illwieckz commented Oct 31, 2024 •

edited

Loading