Page MenuHomeWildfire Games

Fixes black water glitches for certain wind angles
AcceptedPublic

Authored by vladislavbelov on Mar 29 2019, 10:50 PM.

Details

Reviewers
wraitii
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#3061
Summary

The problem was in the expression:

foam1.x = foaminterp.x * WindCosSin.x - foaminterp.z * WindCosSin.y;

The issue was because of negative foam1.x, some videocards handle it differently. It was negative because of WindCosSin - a wind vector, its components can be negative. So I fixed it. But the code is needed to be refactored sometime.

Test Plan

Follow steps in the ticket.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7058
Build 11529: Vulcan BuildJenkins

Event Timeline

Vulcan added a subscriber: Vulcan.Mar 29 2019, 11:00 PM

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

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

Stan accepted this revision.Mar 30 2019, 3:50 PM

This patch seems to fix the issue for me. Tried on a bunch of different maps and could not reproduce the bug anymore.

This revision is now accepted and ready to land.Mar 30 2019, 3:50 PM
elexis added a subscriber: elexis.Mar 30 2019, 9:28 PM
In D1804#73654, @Stan wrote:

This patch seems to fix the issue for me. Tried on a bunch of different maps and could not reproduce the bug anymore.

Testing is good, but not a review.
I'm sure we can find a wrong formula that can give the right results too in a blackbox test.
Vladislav can add Tested By: to the commit message however, at least I trust him to be knowledgeable about this shader.

The problem was in the expression:
foam1.x = foaminterp.x * WindCosSin.x - foaminterp.z * WindCosSin.y;
The issue was because of negative foam1.x, some videocards handle it differently. It was negative because of WindCosSin - a wind vector, its components can be negative. So I fixed it. But the code is needed to be refactored sometime.

Sounds like a good analysis.
If you find a defect, it can be useful to lookup the commit that broke it, so that the author and future readers may learn something for the future.

It seems to be rP15576, which you already have flagged a different concern for. So perhaps you want to fix both in one go, and perhaps one can find more.

Also the reader might want to be interested to learn what the intention behind the 5 to 3 is.

In D1804#73657, @elexis wrote:

I'm sure we can find a wrong formula that can give the right results too in a blackbox test.

It's true, our water shader has a lot of things to improve.

Also the reader might want to be interested to learn what the intention behind the 5 to 3 is.

It was and is just a constant that increases a brightness. It doesn't have a physical meaning. I decreased it because the foam1.x was increased.

Stan added a comment.Mar 31 2019, 11:22 PM
In D1804#73657, @elexis wrote:

Testing is good, but not a review.
I'm sure we can find a wrong formula that can give the right results too in a blackbox test.
Vladislav can add Tested By: to the commit message however, at least I trust him to be knowledgeable about this shader.

That's true, but I don't have review powers for programming anyways :) I told Vladislav that a few times. As I reported this bug, I at least tried to confirm it would be fixed.

In D1804#73657, @elexis wrote:

It seems to be rP15576, which you already have flagged a different concern for. So perhaps you want to fix both in one go, and perhaps one can find more.

Didn't you just tell nani that it was bad to combine patches ?

Wouldn't it be better to max(0, ...) the whole calculation? This is magic-graphical-code but you're changing behaviour a lot here.

vladislavbelov added a comment.EditedApr 23 2019, 10:23 PM

Wouldn't it be better to max(0, ...) the whole calculation? This is magic-graphical-code but you're changing behaviour a lot here.

It makes less sense, because you'll get no flares for some angles.

WindCosSin is a unit vector of wind direction (basis).
foam1 and foam2 detailed foams for current and next frames (to interpolate between them).
foam3 and foam4 not so detailed (zoomed out) foams for current and next frames (to interpolate between them).
foaminterp is interpolation and mix between them. Actually it can be a single float (not a vector), because all foam textures are gray.

foam1.x = foaminterp.x * WindCosSin.x - foaminterp.z * WindCosSin.y;

Mind you, foaminterp.x == foaminterp.z. So it equals to:

foam1.x = foaminterp.x * (WindCosSin.x - WindCosSin.y);

That means we could have negative values for some angles. So made a simple change that doesn't make more sense as well, but works for all angles.

Your related commit, where you added the foam usage: rP15576.

wraitii accepted this revision.Apr 24 2019, 8:37 AM

Mind you, foaminterp.x == foaminterp.z. So it equals to:

foam1.x = foaminterp.x * (WindCosSin.x - WindCosSin.y);

Right, I missed that (didn't actually delve back into this file much). I'd much prefer changing it to foaminterp.x * abs(WindCosSin.x - WindCosSin.y); then to be honest.
I'll accept also, I can see how that would fail on some cards.

Right, I missed that (didn't actually delve back into this file much). I'd much prefer changing it to foaminterp.x * abs(WindCosSin.x - WindCosSin.y); then to be honest.

abs(WindCosSin.x - WindCosSin.y) equals to |cos(angle) - sin(angle)| an its plot:

My formula |cos(angle)| + |sin(angle)|:

As you can see, your formula has zeros for some angles, and it means that we won't see any foam for particular angles.

I think I'll check out how that changes because that might have been WAD by me tbh. Up to personal preference I guess.

I think I'll check out how that changes because that might have been WAD by me tbh. Up to personal preference I guess.

The current patch is just a workaround to remove glitches. The shader code should be refactored in future.

Foam really isn't that useful anymore either, so I guess commit anyways.

Stan resigned from this revision.Thu, May 9, 7:41 PM

@vladislavbelov rebase if necessary and commit?