DaemonEngine / Daemon

The Dæmon game engine. With some bits of ioq3 and XreaL.
https://unvanquished.net
BSD 3-Clause "New" or "Revised" License
304 stars 60 forks source link

Bad normal map channel detection, seems to be related to missing ARB_texture_compression_rgtc #375

Closed illwieckz closed 4 years ago

illwieckz commented 4 years ago

I noticed multiple bugs, some being regressions, on Nvidia Geforce 6000 and Geforce 7000 series. Those cards do not work with nouveau (ARB_framebuffer_object is missing and that's a requirement to run the game) but those card work with Nvidia 204 driver.

Nvida 304 driver is in really bad shape for Geforce 6000 series (most of them will just crash while the kernel complains they have fallen off the buse) but I haven't experienced such issue with a Geforce 7000 one.

Both Geforce 6000 and Geforce 7000 experience the bugs I've noticed.

While looking for the cause of the bug, I noticed this in daemon.log:

...GL_ARB_texture_compression_rgtc not found

Over the 40+ cards that were tested, no other one reproduces the bug, but also no one misses that extension. In fact the only other GPUs known to miss that extensions are the ones from the R300 series by ATI and they are experiencing another bug that prevent maps to load at all (see #345) so that other bug is likely to hide on ATI side the one I disscuss in this thread.

What I noticed is while the engine tells this extension is missing, the engine also prints wrong things in logs:

Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor12_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor12_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor09_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor06_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor05_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor04_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor03_n'
Debug: found heightmap embedded in normalmap 'textures/shared_pk02_src/floor01_n'

The wrong thing in those logs is that those textures do not ship height map in normal map file. When an height map is shipped in a normal map file, the height map is stored in alpha channel, this is not the case here.

We have to detect if the height map is stored in alpha channel for two reasons:

So if either height mapping or normal mapping is enabled, we have to detect if the normal map ships an height map.

We need to properly read the normal map when height mapping is disabled because we use a well know technique that is able to reconstruct the normal map from weird formats and in this case, some of the values is stored in alpha channel for optimization purpose (see this snippet in glsl code. See also that comment by @cmf028 who explains things well. The code is written in a way that when a normal map does not ship height map in alpha channel, either storing XYZ in RGB or XYZ in RGA will produce the same result thanks to the compute producing the same result if A is default value for no alpha channel (fully opaque, one). But the engine must not attempt to reconstruct Z from alpha channel value when the normal map ships an height map or the compute would be wrong.

While the internal format (here CRN) stores the data in alpha channel for optimization purpose, the engine usually don't consider the file to be an RGBA file but an RGB one (only the GLSL code will know how to pick properly the required channels), then it's possible for the engine to know if the file embeds an height map or not. If there is three components, there is no height map even if there is some data hidden in alpha channel, if there is four components, there is an height map in alpha channel.

It looks like the two same logs occurs on the same cards. From all the cards tested at this point, only the Nvidia Geforce 6150LE, 6600 Silencer, 7500LE (both Curie hardware running on 304 drivers), ATI X300, X550HM and X1050 (both R300 hardware running on Mesa r300 driver) both report a missing texture_compression_rgtc and a wrong detection of an height map in normal map alpha channel while it's another RGA format instead.

illwieckz commented 4 years ago

Note: both height map in normal map detection and both relief mapping feature (hence the need for detection) were implemented/fixed/activated after 0.51 release.

illwieckz commented 4 years ago

When #378 is implemented, when a GPU driver does not support ARB_texture_compression_rgtc, our normal maps compressed with -dxn crunch options are detected as GL_COMPRESSED_RGBA_S3TC_DXT5_EXT but when the extension is supported, same normal maps are detected as GL_COMPRESSED_RG_RGTC2:

Here some excerpt when calling imagelist from the same engine on same data when the driver does not supports RGTC and then when it supports it.

RGTC unsupported:

 209:  128  128  yes   2D   DXT5     s.rept  t.rept   textures/shared_pk02_src/floor01_n

RGTC supported:

 209:  128  128  yes   2D   RGTC2rg  s.rept  t.rept   textures/shared_pk02_src/floor01_n

I guess the “Capcom trick” is to be able to load RGTC textures wrongly loaded as DXT5, right?

In this case, implementing detection for height map in normal map alpha channel to avoid the “Capcom trick” is defeating the “Capcom trick” because the “Capcom trick” would be there to workaround the face we cannot detect the alpha channel is not an alpha channel (hence, it's not an height map).

illwieckz commented 4 years ago

Currently we have two ways to load normal maps in materials:

The normalMap keyword is calling some heuristic to detect height maps for two reasons, if I remember correctly:

I'm looking for an alternative way to detect DXT5/RGTC2rg variants than using those GL_ constants, for example by being told by crnlib about it. If there is no way for that then, we have to make strong decisions:

First option:

Second option:

Second option would be a really shame. Basically for the three biggest GPU makers for PC, the only hardware range known to not support RGTC at this point is pre OpenGL 3.0 (and even some later OpenGL 2.0 hardware like ATI R500 supports it) and is 15 years old. See the Unvanquished GPU compatibility matrix about that. Also we have to keep in mind this matrix currently only tested Linux drivers, not Windows and macOS ones.

The problem is that any modern hardware with work in progress driver can also fall into this category. For example, on this day, the mesa matrix page says both etnaviv (Vivante) and v3d (Broadcom) drivers lack RGTC support, when they probably support enough extensions to run Dæmon if the engine was ported to such hardware. I think we must not make decision about assumption the engine will not be ported to other platforms than the ones running the Unvanquished game today.

So, if we find no way to detect the alpha channel is not an alpha channel, I vote for making the normalMap keyword an explicit one. To be honest I had mixed feeling when I made it implicit.

I would like to get contradictory arguments or even people telling me I'm wrong, I missed something, I didn't understood something or that I'm lacking some useful knowledge that would help us to fix that issue, given the missing knowledge (or even help) is provided.

illwieckz commented 4 years ago

This is the same data rendered by the same GLSL code on a driver with disabled RGTC support, except in second screenshot there is no detection of alpha channel in image loader and this alpha channel is not wrongly marked as an heightmap:

wrong normal format detection disables shader

wrong normal format detection disables shader

For that second screenshot, the GLSL code rendered properly the normal map even if the file was loaded wrongly because of the lack of RGTC suport, thanks to the “Capcom trick”.

illwieckz commented 4 years ago

It is confirmed ATI R300 cards are affected too.

illwieckz commented 4 years ago

SomaZ said the detection done in engine like OpenJK is about detecting file suffix (_nh) in a similar fashion to Darkplaces does, not about detecting the alpha channel to enable a feature. It looks like not doing such detection is the way to go.

illwieckz commented 4 years ago

fixed by #379: