Differential D3040

Refactors water shader and move normal and specular calculations into separate functions
ClosedPublic
Actions

Authored by vladislavbelov on Oct 17 2020, 1:58 AM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP24134: Refactors water shader and move normal and specular calculations into separate…

Summary

Before the patch the water shader uses different normals for specular and reflection/refraction, what produces incorrect lighting. After the patch specular is more correct, also eyeVec normalization fixed.

You might notice that the water looks a bit different, that because of using the correct normal and too flattened texture.

Before	After

Test Plan

Apply the patch
Open maps with different water settings, make sure that water looks not really worse than previous
Open atlas and try different water settings when you see reflected sun

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Oct 17 2020, 1:58 AM

vladislavbelov edited the summary of this revision. (Show Details)

vladislavbelov edited the test plan for this revision. (Show Details)

Successful build - Chance fights ever on the side of the prudent.

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1658/display/redirect

Harbormaster completed remote builds in B13353: Diff 13642.Oct 17 2020, 2:05 AM

vladislavbelov requested review of this revision.Oct 17 2020, 2:05 AM

vladislavbelov edited the test plan for this revision. (Show Details)Oct 17 2020, 11:36 AM

Works fine on both my graphics card. Wonder if the reflection should continue to rotate depending on the wind position. Before it wasn't really an issue, because it was circular. Map Corinthian Ismuth

	Before	After
NVIDIA GeForce 1070
Intel UHD 620

Also works on other settings. Has no influence on the ugly water.

Silier added a subscriber: Silier.Oct 17 2020, 1:24 PM

Silier added inline comments.

binaries/data/mods/public/shaders/glsl/water_high.fs
166 ↗	(On Diff #13642)	this could go to return statement :)

vladislavbelov added inline comments.Oct 17 2020, 7:43 PM

binaries/data/mods/public/shaders/glsl/water_high.fs
166 ↗	(On Diff #13642)	Yep.

I think the different normals were there on purpose for artistic effect, as the regular fresnel looks a little drab imo. It's particularly obvious in your summary screenshot: compare with this reference image for example:
https://ak.picdn.net/shutterstock/videos/25308344/thumb/1.jpg

IMO my original take, though "technically" buggy, looks slightly better in some cases.

I would motion to not change the different normals, though the refactoring can stay imo.

Edit: though on Stan's screenshots it's less obvious...

In D3040#133829, @wraitii wrote:

I think the different normals were there on purpose for artistic effect, as the regular fresnel looks a little drab imo. It's particularly obvious in your summary screenshot: compare with this reference image for example:
https://ak.picdn.net/shutterstock/videos/25308344/thumb/1.jpg

Yeah, and that effect is reachable after my fix. The only remaining problem is reflection, but it's not so hard to solve, especially on distance.

IMO my original take, though "technically" buggy, looks slightly better in some cases.

Yes, it was, because of smooth reflections. But they don't match the reality. You can't have so smooth reflections and so spreaded speculars like on your screenshot above.

So, I move the water to a more physically correct rendering.

asterix mentioned this in D2812: Adds MSAA to anti-aliasing techniques.Nov 4 2020, 2:15 PM

No oddities seen when testing.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 5 2020, 6:47 PM

Closed by commit rP24134: Refactors water shader and move normal and specular calculations into separate… (authored by vladislavbelov). · Explain Why

This revision was automatically updated to reflect the committed changes.

vladislavbelov added a commit: rP24134: Refactors water shader and move normal and specular calculations into separate….