Page MenuHomeWildfire Games

Fix and clean up water vertex shader
ClosedPublic

Authored by vladislavbelov on Mar 18 2018, 9:46 PM.

Details

Reviewers
Stan
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22004: Clean up and fix height of water vertex shader.
Summary

There was an issue with the water shader. It didn't cost the water height and always had set it to 15.0.

Test Plan
  1. Run the game and open maps with water on different heights.
  2. Check that the water looks ok and not really worse.

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.Mar 18 2018, 9:46 PM
Stan added inline comments.Mar 18 2018, 9:48 PM
binaries/data/mods/public/shaders/glsl/water_high.vs
58 ↗(On Diff #6214)

Why is it a_vertex.x and not a_vertex.y ?

62 ↗(On Diff #6214)

Comment on top, Start with caps

Vulcan added a subscriber: Vulcan.Mar 18 2018, 9:50 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/273/display/redirect

vladislavbelov added inline comments.Mar 18 2018, 9:54 PM
binaries/data/mods/public/shaders/glsl/water_high.vs
58 ↗(On Diff #6214)

Because we have a horizontal plane, defined by xOz. But the Y-axis looks upward.

The normal has a different space (2D with additional params), so a_vertex.y isn't equal to normalCoords.y.

vladislavbelov marked an inline comment as done.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/274/display/redirect

Stan added inline comments.Mar 19 2018, 8:02 PM
binaries/data/mods/public/shaders/glsl/water_high.vs
10 ↗(On Diff #6215)

On top too :)

79 ↗(On Diff #6215)

Are you sure they didn't just mess up the parenthesis

a * ( b + c / d)

instead of

a * ((b+c) /d)
vladislavbelov added inline comments.Mar 19 2018, 8:06 PM
binaries/data/mods/public/shaders/glsl/water_high.vs
79 ↗(On Diff #6215)

I didn't change the expression, you can compare it with the old one:

((0.15+a_waterInfo.r/1.15))

Why the original is so? Because a_waterInfo.r is in [0, 1] as I understand the code.

Moves the comment.

Stan added inline comments.Mar 19 2018, 8:29 PM
binaries/data/mods/public/shaders/glsl/water_high.vs
79 ↗(On Diff #6215)

Yeah, I know I was just wondering if they didn't mess up somehow :)

Sounds like magic numbers tho

vladislavbelov added inline comments.Mar 19 2018, 8:30 PM
binaries/data/mods/public/shaders/glsl/water_high.vs
79 ↗(On Diff #6215)

There are many magic number in shaders sadly. We need to fix it somewhen.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/284/display/redirect

Stan added a comment.Mar 20 2018, 1:03 AM

Would be nice to have a screenshot with D1401

aeonios added a subscriber: aeonios.EditedApr 15 2018, 1:41 PM

--scratch that--

I tested and this seems to work well. The wobblyness seems to be more consistent with this fix.

@aeonios My friend, will you continue to make other modifications? Or directly launch the patch, what is your choice? Well, you choose, my friend.

wraitii accepted this revision.May 3 2018, 8:32 AM
wraitii added a subscriber: wraitii.

This does seem like a bug. Odd that I never noticed it.

Thanks for the patch :)

binaries/data/mods/public/shaders/glsl/water_high.vs
79 ↗(On Diff #6215)

These don't really have semantic value, I think. calling them WAVINESS_FIX_CONSTANT won't help the issue.

3 ↗(On Diff #6228)

Useless

24 ↗(On Diff #6228)

Useless

45 ↗(On Diff #6228)

Useless

This revision is now accepted and ready to land.May 3 2018, 8:32 AM
vladislavbelov added inline comments.May 3 2018, 11:33 AM
binaries/data/mods/public/shaders/glsl/water_high.vs
3 ↗(On Diff #6228)

Do you think that it doesn't increase readability?

wraitii added inline comments.May 3 2018, 11:53 AM
binaries/data/mods/public/shaders/glsl/water_high.vs
3 ↗(On Diff #6228)

My opinion is that the name of the variables is explicit enough here. I don't care that much about it though so feel free to commit it with those if you prefer.

My friends, can this patch be officially released?

This revision was automatically updated to reflect the committed changes.