Revert MR672

[akortunov]

Fixes regression #5922.

MR672 causes new issues and it is unclear how to fix them, so simplest solution is to revert it for now.

[elsid]

Can you make it via git revert -m 1 b31459cc0 saving all information it fills? MR672 is not a link so you can't easily open that mr and there is no commit hash so you can't easily find it. It should look like this:

$ git show
commit 0b3fb2c8fd8e508b5b3887879baf916f26adb784 (HEAD -> master)
Author: elsid <[email protected]>
Date:   Tue Mar 30 13:24:15 2021 +0200

    Revert "Merge branch 'refractiontest' into 'master'"

    This reverts commit b31459cc009105d211cb8b186e6223894727eece, reversing
    changes made to 369adf158381c1e25d64491c04e50b9020d80782.

diff --git a/files/shaders/water_fragment.glsl b/files/shaders/water_fragment.glsl
index db208ea42..d9b9463ad 100644
--- a/files/shaders/water_fragment.glsl
+++ b/files/shaders/water_fragment.glsl
@@ -29,21 +29,20 @@ const float BUMP_RAIN = 2.5;
 const float REFL_BUMP = 0.10;                      // reflection distortion amount
 const float REFR_BUMP = 0.07;                      // refraction distortion amount

+const float SCATTER_AMOUNT = 0.3;                  // amount of sunlight scattering
+const vec3 SCATTER_COLOUR = vec3(0.0,1.0,0.95);    // colour of sunlight scattering
+
 const vec3 SUN_EXT = vec3(0.45, 0.55, 0.68);       //sunlight extinction

 const float SPEC_HARDNESS = 256.0;                 // specular highlights hardness

 const float BUMP_SUPPRESS_DEPTH = 300.0;           // at what water depth bumpmap will be suppressed for reflections and refractions (prevents artifacts at shores)
-const float BUMP_SUPPRESS_DEPTH_SS = 1000.0;      // modifier using screenspace depth (helps prevent same artifacts but at higher distances)

 const vec2 WIND_DIR = vec2(0.5f, -0.8f);
 const float WIND_SPEED = 0.2f;

 const vec3 WATER_COLOR = vec3(0.090195, 0.115685, 0.12745);

-const float SCATTER_AMOUNT = 0.5;                  // amount of sunlight scattering
-const vec3 SCATTER_COLOUR = WATER_COLOR * 8.0;     // colour of sunlight scattering
-
 // ---------------- rain ripples related stuff ---------------------

 const float RAIN_RIPPLE_GAPS = 5.0;
@@ -219,10 +218,7 @@ void main(void)
     float depthSampleDistorted = linearizeDepth(texture2D(refractionDepthMap,screenCoords-screenCoordsOffset).x) * radialise;
     float surfaceDepth = linearizeDepth(gl_FragCoord.z) * radialise;
     float realWaterDepth = depthSample - surfaceDepth;  // undistorted water depth in view direction, independent of frustum
-    screenCoordsOffset *= clamp(
-            realWaterDepth / (BUMP_SUPPRESS_DEPTH
-                * max(1, depthSample / BUMP_SUPPRESS_DEPTH_SS)) // suppress more at distance
-        ,0 ,1);
+    screenCoordsOffset *= clamp(realWaterDepth / BUMP_SUPPRESS_DEPTH,0,1);
 #endif
     // reflection
     vec3 reflection = texture2D(reflectionMap, screenCoords + screenCoordsOffset).rgb;
@@ -240,11 +236,7 @@ void main(void)
     if (cameraPos.z < 0.0)
         refraction = clamp(refraction * 1.5, 0.0, 1.0);
     else
-    {
-        vec3 refractionA = mix(refraction, waterColor, clamp(depthSampleDistorted/VISIBILITY, 0.0, 1.0));
-        vec3 refractionB = mix(refraction, waterColor, clamp(realWaterDepth/VISIBILITY, 0.0, 1.0));
-        refraction = mix(refractionA, refractionB, 0.8);
-    }
+        refraction = mix(refraction, waterColor, clamp(depthSampleDistorted/VISIBILITY, 0.0, 1.0));

     // sunlight scattering
     // normal for sunlight scattering

[akortunov]

Done.

[wareya]

A straight up reversion is bad too, since it just reintroduces the bug I fixed, which is that water becomes completely opaque exclusively when refraction is enabled but not when only reflections are enabled.

"unclear how to fix" is an overstatement. The shoreline artifacts are coming from the refraction texture actually being used instead of fading out at a distance. The hack that suppresses them needs to be adjusted to match. Or the refraction should be rendered on top of something other than bright skybox. And the see-through-ness of the water itself can be adjusted in any number of ways, from the proposed diff I posted on my original MR to making water behave more like it does when refraction is disabled.

Having said that, I'm not against reverting or cordoning off the bugfix I made until other changes are made such that it's not uncovering other problems, as long as the problem where refraction makes the water become completely opaque at high camera positions is not forgotten.

[AnyOldName3]

Has anyone tried just making the refraction RTT clear colour something less artefact-prone yet (by calling the obviously-named method on the RTT camera)? It should be a ten-second job to test that.

[elsid]

Even if it's a ten-second job to fix this, still a new pr/mr is required. No need to keep breaking changes in the master when we are close the make a new release.

[psi29a]

@wareya can you then create an issue to track the original issue? I did not see an accompanying changelog/issue for https://gitlab.com/OpenMW/openmw/-/merge_requests/672

[psi29a]

Even if it's a ten-second job to fix this, still a new pr/mr is required. No need to keep breaking changes in the master when we are close the make a new release.

@elsid I left this PR alone for now because I believed the issue was being worked on, especially when @wareya says:

A straight up reversion is bad too, since it just reintroduces the bug I fixed, which is that water becomes completely opaque exclusively when refraction is enabled but not when only reflections are enabled.

So now the original bug is still there and we just switched back to that instead. 🤷🏼‍♂️

[elsid]

IIUC the issue https://gitlab.com/OpenMW/openmw/-/merge_requests/672 fixed is not a regression. Since that mr by fixing it introduced a regression it make sense to revert the change because known bugs have more priority. Better to keep old bugs rather than replace them with new ones. The code is still there in the git history to take it and complete the work.

[AnyOldName3]

The regression isn't even really a regression, just an existing problem being more obvious now an actual bug is gone. It's also a subjective thing. If we replaced an incomplete set of icons in the CS with a complete one from a different designer, but someone opened an issue report with the regression tag because they thought a few of the new icons were uglier than their predecessors, we wouldn't revert the change and go back to having an incomplete set of icons. This PR is roughly analogous to that situation. Another analogy that's going to bug me if I don't write it now I've thought of it is that we wouldn't relight a house fire if the resident noticed their air conditioning was stuck on when they extinguished a house fire - it's new behaviour that they feel cold, but the cause was always there, just masked by a bigger problem in the opposite direction.

This isn't to say I don't think #5922 should be fixed, just that reverting this is neither a solution nor a step towards a solution.

[AbduSharif]

There's also another regression, the green color of water shader is almost gone when compared to master.

[AnyOldName3]

Things can change without being a regression. The green was believed to have been chosen just because it looked good in one guy's blender scene that he wrote a water shader for that Scrawl ported to OpenMW.

[AbduSharif]

It looked good for other people too tbh (the green color that's almost gone, even real life water has it), me included.

There's other games that do that for their water/water shader, and it looked nice and made sense because of the environment.

[johannesrld]

Personally I think the new water looks better.

[elsid]

If we're at the point when different look for different people means a bug or not a bug then something is wrong. I see that water is too transparent and it's not my subjective perception. We can measure pixel color with and without water both underwater and outside water and compare the values. Even if that mr fixed some bug but didn't do it without objective flaws it should be work in progress and not be merged into the master. From the bug description:

Since formula does not seem to take distance to camera in account, player can see underwater even in kilometers away. Interesting that when camera is even a bit underwater, you will see almost nothing, what is quite inconsistent.

Probably we're trying to measure the work of art where people never agree. But at least it has to be consistent while it models physics. If we're going to the point that someone likes this shader then it's not a problem, just install it for yourself and use or whaterver else shader you want.

[wareya]

yeah I don't have any strong objections to this getting reverted temporarily as long as the narrow problem "refraction water gets opaque as you move away from it" isn't forgotten, even if it's trading one bug for another

[johannesrld]

true, its just the water after all

[akortunov]

I created a ticket for the feature to make sure that we do not forget about it.

[AnyOldName3]

I think it's important that when discussing the water shader we bear in mind that aspects of it are total nonsense physical-plausibility-wise. If we force everyone making aspects of it more physically plausible to remove every perceived negative side effect of doing so, it's never going to get better until someone rocks up with a total replacement. I'm fairly sure the increased transparency is more realisic, not less, but now we've not got artifical opacity standing in for things like:

• wet things generally looking darker than dry things, which isn't modelled, so underwater stuff being too bright in OpenMW
• water tending to accumulate in ditches and dents in the landscape, where there's ambient occlusion from higher up parts of the landscape blocking indirect light from the sky, which isn't modelled, so underwater stuff being too bright in OpenMW
• reflected light from the water's surface not contributing enough in OpenMW

Those three all mean underwater stuff is more visible compared to the water's surface than they're supposed to be. The transparency's fine with wareya's recent changes, just when you additively blend a thing that's darker than it's supposed to be with a thing that's lighter than it's supposed to be, you see more of the lighter thing than you're supposed to.