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

Resize textures using GL_PROXY mechanism (and other things) #394

Closed
wants to merge 14 commits into from

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Oct 27, 2020

  1. Resize textures using GL_PROXY mechanism
  2. Introduce picMin and picMax

The ATI X1950 PRO can display the game with medium preset including full size textures except the skybox that is 1024^6 size:

picmax

With r_picMax set to 512, everything looks good:

picmax

The r_picMax cvar expects the size of an edge, so r_picMax 512 will scale down every image larger than a square image of 512×512 size.

Using r_picMax 512 will scale down textures from tex-vega that are 1024×1024 large but will not scale down textures from tex-pk01 that are 512x512 large. This way a map using both vega and pk01 textures will look highly textured with 512×512 textures everywhere while using r_picmip 1 would have displayed a mix of nice 512x512 and ugly 256×256 images.

Both r_picmip and r_picMax can be used at the same time. For example, by using r_picmip 1 and r_picMax 512, once minimap is set as nopicmip (see Unvanquished/Unvanquished#1230 ), the game will scale down all textures but the minimap to keep it clear, unless it is larger than 512, but minimaps with size up to 512 will not be scaled down

I added two shader keywords: picMin and picMax.

The picMin one allows an artist to tell the renderer to never downscale a texture under this size. This is useful for models for example, since seeing details like the eyes and others is better.

The picMax allows an artist to tell the renderer to downscale the texture to this size, even if the player is playing full size.

I added another cvar, r_picMin, which allows to override the picMin value set in shader, and when it is set to -1, never downscale images that has picMin value (it would be like making them nopicmip).

@slipher
Copy link
Member

slipher commented Oct 28, 2020

It looks like there is some code right next to the code you added which is supposed to do exactly the same thing. What is glConfig2.maxCubeMapTextureSize on your test machine?

@illwieckz
Copy link
Member Author

illwieckz commented Oct 29, 2020

I get 4096 for glConfig2.maxCubeMapTextureSize on the ATI X1950 PRO.

@illwieckz
Copy link
Member Author

illwieckz commented Oct 29, 2020

Interestingly those are the values on the original code to scale down lightmaps:

scaledWidth: 1024, scaledHeight: 1024, glConfig2.maxCubeMapTextureSize: 4096

Is glConfig2.maxCubeMapTextureSize the size of a single face or the size of the whole cube? If it's the size of a single face then the driver is buggy, if it's the size of the whole cube then we have to compare scaledWidth and scaledHeight with glConfig2.maxCubeMapTextureSize / 6 instead.

That said, this hardware is not meant to support 4096 sized standard flat textures, so it's probably the summed size of all the faces of the skybox.

Testing against glConfig2.maxCubeMapTextureSize / 6 fixes the issue, but it may be wrong.

I still think the r_picMax feature is good even if unneeded to fix bugs. For example setting r_picMax to 512 still produces high definition rendering and it's better than doing r_picmip 1 that would reduce 4096 sided large textures to still-heavy 2048 ones and ok-ish 512 sided textures to ugly 256 ones.

Basically r_picmip looks to be a relic from a past where all textures had the same size and were low definition anyway, and then user would somewhat expect the resulting images having the same size. To me this r_picMax feature is more useful than r_picmip, this is what I would naturally look for instead.

@illwieckz
Copy link
Member Author

I added a commit to fix skybox scaling according to the hardware limits.

Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

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

I doubt you are supposed to divide the cubemap max size by 6, but who knows. According to this the max size is supposed to be at least 1024, so reporting the correct value would be illegal... I wonder if 4096 is a default value when the implementation doesn't provide a max size.

I just noticed something really stupid... GL error checking is disabled by default (r_ignoreGLErrors 1). Turning it on, however, terminates the program the first time as an error. Of course the sensible thing to do would be a non-fatal log message. This should help diagnose things, since using a texture larger than the max size is one of the GL errors which can be produced.

I agree r_picmip is a puzzling feature. I guess you could use it to test the downsampling code, or as a brute force way to reduce GPU memory. Both of which could be done with picmax as well. A quick google suggests it has been mostly popular as a cheat to make enemies more visible in various games.

src/engine/renderer/tr_init.cpp Outdated Show resolved Hide resolved
src/engine/renderer/tr_image.cpp Outdated Show resolved Hide resolved
@illwieckz illwieckz changed the title Introduce r_picMax to only do picmic on large images Introduce r_picMax to only do picmip on large images Oct 30, 2020
@illwieckz
Copy link
Member Author

I rewrote entirely the code to compute an image size, or to be more precise, to compute a pixel size in memory (see #378).

I added two shader keywords, picMin and picMax.

The picMin one allows an artist to tell the renderer to never downscale a texture under this size. This is useful for models for example, since seeing details like the eyes and others is better.

The picMax allows an artist to tell the renderer to downscale the texture to this size, even if the player is playing full size. This can be useful for example if a texture is shipped in 4096×4096 size but is only displayed on a tiny surface. This allow to ship full size textures even if they are used downscaled in game. It's better for conservancy.

I added another cvar, r_picMin, which allows to override the picMin value set in shader.

So basically, texture is downscaled using picMax (or r_picMax if positive) unless a shader uses picMin. If a shader uses picMin, the texture is downscaled to the largest value between picMin or picMax unless r_picMin is set. If a shader uses picMin and r_picMin is set to something greater than zero, then texture is downscaled using r_picMin. There is a special value for r_picMin: if a shader uses picMin and r_picMin is set to -1, then texture is not downscaled at all. This makes possible to downscale a lot what is known to be downscalable safely, and to not downscale at all what is known to not downscale properly. This is useful for competitive scenes: it makes possible to make world surfaces using only one color but models would be rendered properly.

Pictures may help to understand, in all those screenshots, models shaders have picMin 128 instruction.

r_picMax 512 and r_picMin 0, every texture is loaded with 512×512 size maximum.

picmax

r_picMax 64 and r_picMin 0, every texture is loaded with 64×64 size maximum, except models that are loaded with 128×128 size.

picmax

r_picMax 1 and r_picMin 0, every texture is loaded with 1×1 size maximum, except models that are loaded with 128×128 size.

picmax

r_picMax 1 and r_picMin -1, every texture is loaded with 1×1 size maximum, except models that are loaded full size.

picmax

@slipher
Copy link
Member

slipher commented Oct 31, 2020

The picMax allows an artist to tell the renderer to downscale the texture to this size, even if the player is playing full size. This can be useful for example if a texture is shipped in 4096×4096 size but is only displayed on a tiny surface. This allow to ship full size textures even if they are used downscaled in game.

This doesn't sound like a great practice to encourage, since you would still pay full price in file size and load times. Wouldn't something like this be better as part of the map build process?

@illwieckz
Copy link
Member Author

This is better as part of a map build process, but a mapper can still write shaders based on third-party texture set and other situations.

Note that I did not added that because of a urge need. I see a real need for picMin so I did it, and right after than I've seen picMax could be done for free. There would be no issue to not have it, but once picMin is implemented, picMax costs nothing, so why not.

src/engine/renderer/tr_image.cpp Outdated Show resolved Hide resolved
src/engine/renderer/tr_image.cpp Outdated Show resolved Hide resolved
src/engine/renderer/tr_local.h Outdated Show resolved Hide resolved
src/engine/renderer/tr_image.cpp Outdated Show resolved Hide resolved
@@ -170,286 +580,270 @@ void R_ImageList_f()
{
image = (image_t*) Com_GrowListElement( &tr.images, i );
char buffer[ MAX_TOKEN_CHARS ];
char imageType[ MAX_TOKEN_CHARS ];
Copy link
Member

Choose a reason for hiding this comment

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

Just use std::string

Copy link
Member Author

@illwieckz illwieckz Nov 7, 2020

Choose a reason for hiding this comment

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

Still TODO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, there is still usage of Com_sprintf, not sure how to handle that with std::string.

Copy link
Member

Choose a reason for hiding this comment

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

.c_str() to get a char*

Copy link
Member Author

Choose a reason for hiding this comment

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

I still don't know how to do this with std::string:

Com_sprintf( imageType, sizeof( imageType ),  "%5i ", image->type );

Copy link
Member

Choose a reason for hiding this comment

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

Com_sprintf -> Str::Format

src/engine/renderer/tr_image.cpp Outdated Show resolved Hide resolved
@illwieckz illwieckz force-pushed the picMax branch 2 times, most recently from 07bd0cf to 69c1d4e Compare November 7, 2020 11:55
@illwieckz
Copy link
Member Author

illwieckz commented Nov 7, 2020

I've added a commit temporarily named wat that may fix an issue… While reading the original code, I thought this part looked weird and that it sounded like a typo. I would appreciate opinions on that.

@illwieckz
Copy link
Member Author

This is basically what does the wat commit, before:

Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 0, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 1, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 2, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 3, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 4, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 5, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 6, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 7, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 8, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 9, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 10, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 11, layer 0

after:

Debug: Loading compressed image models/buildables/barricade/body_d with 2048×2048 original size, mip 0, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 1024×1024 size, mip 1, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 512×512 size, mip 2, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 256×256 size, mip 3, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 128×128 size, mip 4, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 64×64 size, mip 5, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 32×32 size, mip 6, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 16×16 size, mip 7, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 8×8 size, mip 8, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 4×4 size, mip 9, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 2×2 size, mip 10, layer 0
Debug: Loading compressed image models/buildables/barricade/body_d with 1×1 size, mip 11, layer 0

Original behavior sounded wrong, right?

@illwieckz
Copy link
Member Author

illwieckz commented Nov 7, 2020

Unfortunately the new code to detect images being too large for the hardware does not work, or not the way we expect it. I never see them being downscaled.

@illwieckz
Copy link
Member Author

illwieckz commented Nov 7, 2020

It works, but not the way we expect it for cube maps. If I double width and height when passing them to CheckImage2D, the obviously too large size is detected and proper order is given to downscale, and the skybox is rendered properly (because effectively downscaled):

Debug: Uploading image env/shared_space_src/sky07 with 256×256 size, downscaled from 1024×1024 size, layer 0
Debug: Uploading image env/shared_space_src/sky07 with 256×256 size, downscaled from 1024×1024 size, layer 1
Debug: Uploading image env/shared_space_src/sky07 with 256×256 size, downscaled from 1024×1024 size, layer 2
Debug: Uploading image env/shared_space_src/sky07 with 256×256 size, downscaled from 1024×1024 size, layer 3
Debug: Uploading image env/shared_space_src/sky07 with 256×256 size, downscaled from 1024×1024 size, layer 4
Debug: Uploading image env/shared_space_src/sky07 with 256×256 size, downscaled from 1024×1024 size, layer 5

Edit: hu, no, that does not work, I forgot I have previously set r_picMax… There is no downscaling based on testing the upload, at all.

@illwieckz
Copy link
Member Author

Well, if I do this (notice the << 3):

	scalingStep = CheckImage2D( internalFormat, scaledWidth << 3, scaledHeight << 3, 0, format, GL_UNSIGNED_BYTE );

it works:

Warn: Image env/shared_space_src/sky07 too large, downscaling 1 time(s) to fit the hardware limits 
Debug: Uploading image env/shared_space_src/sky07 with 512×512 size, downscaled from 1024×1024 size, layer 0 
Debug: Uploading image env/shared_space_src/sky07 with 512×512 size, downscaled from 1024×1024 size, layer 1 
Debug: Uploading image env/shared_space_src/sky07 with 512×512 size, downscaled from 1024×1024 size, layer 2 
Debug: Uploading image env/shared_space_src/sky07 with 512×512 size, downscaled from 1024×1024 size, layer 3 
Debug: Uploading image env/shared_space_src/sky07 with 512×512 size, downscaled from 1024×1024 size, layer 4 
Debug: Uploading image env/shared_space_src/sky07 with 512×512 size, downscaled from 1024×1024 size, layer 5 

@illwieckz
Copy link
Member Author

illwieckz commented Nov 8, 2020

I noticed that, on this computer reporting 4096 max cubemap texture size, if I multiply a 1024 sized texture by 4, it does not scale down, then display garbage, meaning 4096 is expected to be OK but 1024 is already not. While multiplying by 5 downscaling happens (and then displays the skybox properly because being downscaled to 512).

@illwieckz
Copy link
Member Author

The current branch works, but I have no explanation for the multiplier. I've set 6 like the amount of faces a cube has, and a comment explaining there is no explanation.

@illwieckz illwieckz changed the title Introduce r_picMax to only do picmip on large images resize textures using GL_PROXY mechanism, introduce picMin and picMax Nov 8, 2020
@illwieckz
Copy link
Member Author

I got this on debug build, I don't know how to fix it:

#0  0x00005555558d618e in Str::AssertOnTinyFormatError (reason="\"tinyformat: Not enough conversion specifiers in format string\"") at /home/illwieckz/dev/buildme-unvanquished/Daemon/src/common/String.cpp:37
#1  0x0000555555650b73 in tinyformat::detail::streamStateFromFormat (out=..., spacePadPositive=@0x7fffffff4181: false, ntrunc=@0x7fffffff4188: -1, fmtStart=0x5555559bcf38 "", formatters=0x7fffffff4410, argIndex=@0x7fffffff4184: 5, numFormatters=7) at /home/illwieckz/dev/buildme-unvanquished/Daemon/libs/tinyformat/tinyformat.h:607
#2  0x00005555556518e2 in tinyformat::detail::formatImpl (out=..., fmt=0x5555559bcf38 "", formatters=0x7fffffff4410, numFormatters=7) at /home/illwieckz/dev/buildme-unvanquished/Daemon/libs/tinyformat/tinyformat.h:807
#3  0x0000555555651da9 in tinyformat::vformat (out=..., fmt=0x5555559bcef8 "Uploading compressed image %s with %d×%d size, mip %d, layer %d", list=...) at /home/illwieckz/dev/buildme-unvanquished/Daemon/libs/tinyformat/tinyformat.h:963
#4  0x00005555558223c1 in tinyformat::format<char [64], int, int, unsigned short, unsigned short, int, int> (out=..., fmt=0x5555559bcef8 "Uploading compressed image %s with %d×%d size, mip %d, layer %d") at /home/illwieckz/dev/buildme-unvanquished/Daemon/libs/tinyformat/tinyformat.h:973
#5  0x0000555555821a7f in tinyformat::format<char [64], int, int, unsigned short, unsigned short, int, int> (fmt=0x5555559bcef8 "Uploading compressed image %s with %d×%d size, mip %d, layer %d") at /home/illwieckz/dev/buildme-unvanquished/Daemon/libs/tinyformat/tinyformat.h:982
#6  0x0000555555820e8b in Str::Format<char (&) [64], int&, int&, unsigned short&, unsigned short&, int&, int&> (format=...) at /home/illwieckz/dev/buildme-unvanquished/Daemon/src/common/String.h:315
#7  0x000055555581faa9 in Log::Logger::Debug<char (&) [64], int&, int&, unsigned short&, unsigned short&, int&, int&> (this=0x555556236c80 <Log::defaultLogger>, format=...) at /home/illwieckz/dev/buildme-unvanquished/Daemon/src/common/Log.h:220
#8  0x000055555581e932 in Log::Debug<char (&) [64], int&, int&, unsigned short&, unsigned short&, int&, int&> (format=...) at /home/illwieckz/dev/buildme-unvanquished/Daemon/src/common/Log.h:272
#9  0x00005555558189c2 in R_UploadImage (dataArray=0x7fffffff4b18, numLayers=1, numMips=8, image=0x7fffd5f3b200, imageParams=0x7fffffffcf70) at /home/illwieckz/dev/buildme-unvanquished/Daemon/src/engine/renderer/tr_image.cpp:1127

…ters

makes easy to extend and to pass data through multiple cascading functions
@illwieckz illwieckz changed the title resize textures using GL_PROXY mechanism, introduce picMin and picMax resize textures using GL_PROXY mechanism Feb 7, 2021
@illwieckz illwieckz changed the title resize textures using GL_PROXY mechanism [DRAFT] resize textures using GL_PROXY mechanism May 31, 2021
@illwieckz illwieckz changed the title [DRAFT] resize textures using GL_PROXY mechanism WIP: resize textures using GL_PROXY mechanism May 31, 2021
@illwieckz illwieckz marked this pull request as draft May 31, 2021 22:07
@slipher slipher changed the base branch from 0.52.0/sync to master June 30, 2021 19:56
@illwieckz illwieckz changed the title WIP: resize textures using GL_PROXY mechanism WIP: resize textures using GL_PROXY mechanism (and other things) May 11, 2024
@illwieckz
Copy link
Member Author

illwieckz commented May 11, 2024

I close this as commits will be cherry-picked in dedicated PRs.

@illwieckz illwieckz closed this May 11, 2024
@illwieckz illwieckz changed the title WIP: resize textures using GL_PROXY mechanism (and other things) Resize textures using GL_PROXY mechanism (and other things) May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants