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

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

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.

In D1804, @vladislavbelov wrote:

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.

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.

In D1804#75402, @wraitii wrote:

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.

In D1804#75904, @wraitii wrote:

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.

In D1804#75938, @wraitii wrote:

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.

@vladislavbelov rebase if necessary and commit?