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

renderer: fix normalmap dark blotches #260

Merged
merged 1 commit into from Jan 10, 2020

Conversation

@illwieckz
Copy link
Member

illwieckz commented Dec 31, 2019

I discovered that bug while toying with r_showNormalMaps (see #257).

I was like:

Wait,

weird normal z reconstruction

Wait…

weird normal z reconstruction

Wait………

weird normal z reconstruction

As you can see the bug is also visible when diffuse and everything is rendered properly.

So I clamped normal Z component between 0 and 1… and magically the bug disappeared!

Before:

weird normal z reconstruction

weird normal z reconstruction

After:

weird normal z reconstruction

weird normal z reconstruction

I don't know why this bug happens, and why this fix works, but it works. This is a fix I first discovered when I faced similar bug while generating normalmap from heightmap (see #258). This is why I implemented the fix after all the available ways to load normalmaps, and not only at the end of the Z reconstruction: the fix will be used later by normalmap generation too.

Note that an abs(normal.z) is not enough, clamp(normal.z, 0, 1.0) is enough.


Edit: the depicted texture is shared_pk02/rock01, so the file is textures/shared_pk02_src/rock01_n. Note that the normalmap is renormalized at crunch time.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 5, 2020

The bug is reproducible with stock chasm map (outdoor uses that pk02/rock01 texture) on 0.51.1 release:

weird normal z reconstruction

@slipher

This comment has been minimized.

Copy link
Member

slipher commented Jan 6, 2020

I checked the DXT5 normal maps on Chasm for out-of-range values. The sum of the squares of the two components (derived from byte values by x / 255 * 2 - 1) should be no greater than 1 to get a valid 3rd component. Many of the normalmap textures had a few hundred or a few thousand pixels with a sum greater than 1. Presumably, this happens when a vector which originally has an L2 norm less than 1 becomes greater than 1 after being lossily compressed. I confirmed that this is the cause of most of the black areas by replacing any block containing a bad pixel with a solid color (however there is 1 patch that remains mysteriously black which you can see in the screenshot).
unvanquished_2020-01-05_213323_000

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

Wow, that's precious to know! So basically things are going wrong because of compression artifacts…

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

do you think this bug can appear with normalmaps loaded the RGB way?

@slipher

This comment has been minimized.

Copy link
Member

slipher commented Jan 6, 2020

Instead of this we could also consider trying to make the crunch renormalize option always produce vectors with a norm of less than 1. But this patch is certainly easier :)

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

In any way, it's good to have an engine that is able to automatically fix compression artifacts, even if we patch every software on the planet and beyond to not produce problematic compression artifacts.

That said, I'm not against a crunch fix if that's doable.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

note: doing a crunch fix for this would make us eligible for a blog post about it, and we would send it to Binomial/Unity/Khronos so the next Khronos texture standard would take care of this. 😉

@slipher

This comment has been minimized.

Copy link
Member

slipher commented Jan 6, 2020

Yeah, this PR seems a definite good idea in any case considering the large variety of inputs which can have z-component reconstruction. Also a comment about why the sum of the first 2 components can be too large would be good.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

I'm copy pasting your comment

@illwieckz illwieckz force-pushed the illwieckz:darknormalblotch branch 2 times, most recently from 5b46242 to 0055530 Jan 6, 2020
@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

@slipher, your max() code fixes the artifacts as well, so I'm using it.

Do you know if there would be a need for a similar fix with normalmap read the RGB way? And what it would be?

@slipher

This comment has been minimized.

Copy link
Member

slipher commented Jan 6, 2020

Do you know if there would be a need for a similar fix with normalmap read the RGB way? And what it would be?

This GLSL code is downstream from texture decompression, so it doesn't matter how the image was obtained. You just get some RGBA values from 0 to 1.

So in the z-reconstruction case, the code would work regardless of the input texture format. And in the non-z-reconstruction case, there is no taking the square root of a possibly negative number, so the problem could never occur at all.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Jan 6, 2020

Thanks, that's good to know.

@illwieckz illwieckz force-pushed the illwieckz:darknormalblotch branch from 0055530 to 423b92b Jan 9, 2020
@slipher
slipher approved these changes Jan 9, 2020
@illwieckz illwieckz merged commit 7770bfe into DaemonEngine:master Jan 10, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.