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 infuriating grey line #287

Merged
merged 1 commit into from Feb 10, 2020

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Feb 9, 2020

Maybe I'm a wizard, then.

I've noticed that if the bug is not in SSAO neither FXAA code, using both makes worst artifacts than only one of those. I also noticed that ticking FXAA on and off (the later not requiring an engine restart), I was seeing the screen being more streched with on than off, in real time.

Here is with and without fxaa (while ssao is enabled):

with ssao, without fxaa

with ssao, with fxaa

Those screenshots can also be seen there with a nice slider.

So, I've noticed that the only common code in both shaders was something like that:

gl_FragCoord.st * r_FBufScale;

Which is the computation for the pixel size.

I then thought: “maybe those floats in r_FBufScale (a vec2) have rounding errors”, I then modified them in shader the arbitrary way and I was able to produce garbage based on vertical and horizontal lines the same way:

garbage with lines

I then thought: “wait, those floats are passed from c++ to glsl, those floats are just the quotient of 1 by ints, we can pass them as int instead and translate multiplication by division”, I tried, it seems to work.

So, this is expected to fix Unvanquished/Unvanquished#858

On my side it does:

no more boggy lines

@illwieckz
Copy link
Member Author

illwieckz commented Feb 9, 2020

@DolceTriade the bug was almost 15 years old:

https://sourceforge.net/p/xreal/svn/462

added various post process effects
Sat Oct 1 15:31:01 2005 +0000

@DolceTriade
Copy link
Contributor

Ha! Nice find!

@slipher
Copy link
Member

slipher commented Feb 10, 2020

How can this be so important? The max error of a float is 30 parts per billion. It's hard to see how this could make a difference.

@illwieckz
Copy link
Member Author

illwieckz commented Feb 10, 2020

@slipher, the big difference I see is that when this float is set as a define, it is not copied from CPU memory to GPU memory, but stringified by some Str::Format then parsed by the glsl renderer. There is high chance to see a truncation there.

In this case the bug was probably hidden until 2008, when those floats were converted from uniforms to defines:

https://sourceforge.net/p/xreal/svn/1963

u_FBufScale -> r_FBufScale
u_NPOTScale -> r_NPOTScale
Tue Jan 8 17:26:35 2008 +0000

Also, 1/1024 is rational (.0009765625 × 1024 = 1) so vertical line may not occur on 1024×768 screen (very common resolution in 2005, still very present in 2008) while 1/1920 seems to not be (.00052083333333333333 × 1920 ≈ .99999999999999999360), the 1920×1080 (or multiple of it) resolution usage growing since like never.

And in any way, the shortest Str::Format truncates, the faster the bug may occur.

@illwieckz illwieckz changed the title renderer: attempt to fix infuriating grey line renderer: fix infuriating grey line Feb 10, 2020
@illwieckz illwieckz force-pushed the greyline branch 2 times, most recently from 0b237d2 to 65768df Compare February 10, 2020 07:25
@illwieckz illwieckz merged commit 0e1f11c into DaemonEngine:master Feb 10, 2020
@illwieckz
Copy link
Member Author

illwieckz commented Feb 10, 2020

This also fixed the heatHaze edge bug:

heatHaze edge bug

This was a bug that I don't remember having noticed while playing at 1920×1080, but this bug started to be noticeable at 2560×1440 and became very obvious and annoying at 3840×2160.

I'm not able to reproduce it anymore.

@illwieckz
Copy link
Member Author

This also fixed the fog offset issue I talked about in Unvanquished/Unvanquished#799 (comment) .

etmain on Daemon

etmain on Daemon

@DolceTriade
Copy link
Contributor

Dang, big impact fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infuriating Grey Line
3 participants