Render bug, large black box occluding screen

[stgatilov]

5 minutes ago, nbohr1more said:

Both the bloom_downsample.frag.glsl and bloom_upsample.frag.glsl accumulate a "sum" variable.

Some of the operations done on the sum variable are subtractions.

My fix is to ensure that the minimum value for sum is 0

void main() {
    // query previous mipmap level by a full-pixel offset (corresponds to half-pixel in our output framebuffer)
    vec2 offset = vec2(1, 1) / textureSize(u_sourceTexture, 0);
    vec4 sum = sampleTexture(vec2(0, 0)) * 4;
    sum += sampleTexture(-offset);
    sum += sampleTexture(offset);
    sum += sampleTexture(vec2(offset.x, -offset.y));
    sum += sampleTexture(vec2(-offset.x, offset.y));
    FragColor = max(sum, 0) / 8;

 

I don't see any subtractions in this code, only additions.

[nbohr1more]

Just now, stgatilov said:

I don't see any subtractions in this code, only additions.

-offset.y and -offset.x will act like subtractions in this code block no?

[AluminumHaste]

I reverted the glprogs back to the default ones, problem still gone.
 

[nbohr1more]

Just now, AluminumHaste said:

I reverted the glprogs back to the default ones, problem still gone.
 

You mean with the new material def?

( If so ) That's great, please commit those changes.

That said, we should still try to cure bloom of it's vulnerability to bad values

[stgatilov]

The problem may be caused by bump stage?

In the debugger, I see that this material (soft one) has three ambient stages and one bump stage.
Three ambient stages is the main one plus two from frob-highlight, and the bump stage is probably caused by bumpmap line.

To be honest, I don't know what's the purpose of bumpmap line in this material...

[stgatilov]

11 minutes ago, nbohr1more said:

-offset.y and -offset.x will act like subtractions in this code block no?

Nope, they just change texture coordinates.

The colors are still added.

[nbohr1more]

1 minute ago, stgatilov said:

The problem may be caused by bump stage?

In the debugger, I see that this material (soft one) has three ambient stages and one bump stage.
Three ambient stages is the main one plus two from frob-highlight, and the bump stage is probably caused by bumpmap line.

To be honest, I don't know what's the purpose of bumpmap line in this material...

As I can tell, this was probably copied from an unlit diffuse version and made emissive by using a raw image map rather than diffuse. Adding specular probably gives the engine something to do with the bump data?

[stgatilov]

@AluminumHaste, does the problem go away if you replace "return totalColor;" with "return vec3(0);" in interaction.common.fs.glsl and do reloadGlslPrograms?
Assuming you use "r_useNewBackend 1", of course.

If yes, then the problem comes from this shader.

[nbohr1more]

6 minutes ago, stgatilov said:

Nope, they just change texture coordinates.

The colors are still added.

Ahh, so the only way that "sum" would ever be negative is if the sample color at the coordinates was also negative.

 

[AluminumHaste]

45 minutes ago, stgatilov said:

@AluminumHaste, does the problem go away if you replace "return totalColor;" with "return vec3(0);" in interaction.common.fs.glsl and do reloadGlslPrograms?
Assuming you use "r_useNewBackend 1", of course.

If yes, then the problem comes from this shader.

I don't know, I'm in bed lol, it's after midnight. Will check in the morning.

[AluminumHaste]

10 hours ago, stgatilov said:

@AluminumHaste, does the problem go away if you replace "return totalColor;" with "return vec3(0);" in interaction.common.fs.glsl and do reloadGlslPrograms?
Assuming you use "r_useNewBackend 1", of course.

If yes, then the problem comes from this shader.

Yes, it goes away. I bound F10 to reloadGLSLprograms so I didn't have to open the console or touch the mouse.
Undid all the changes to the material def and gl progs.

 

[stgatilov]

2 hours ago, AluminumHaste said:

Yes, it goes away. I bound F10 to reloadGLSLprograms so I didn't have to open the console or touch the mouse.
Undid all the changes to the material def and gl progs.

Then you should selectively disable parts of the math in advancedInteraction until you find the root of the issue.

Just remember that special values have some quirks. For instance, if you compare NaN with anything, comparison always returns false. Although I'm not sure optimizer won't break that rule...

Since adding specularmap helped you, I think "specularColor" value should be a good start.
If you reset it to zero after it gets computed, does the issue go away?
If yes, then try various subterms it is composed from...

Also, don't believe clamps. It is undefined what clamp returns for NaN, and I won't be surprised if it remains NaN even though it is stupid.

[nbohr1more]

I would start by commenting out these:

float fresnelTerm = pow(1.0 - NdotV, fresnelParms2.w);   

float rimLight = fresnelTerm * clamp(NdotL - 0.3, 0.0, fresnelParms.z) * lightParms.y;

[AluminumHaste]

21 minutes ago, nbohr1more said:

I would start by commenting out these:

float fresnelTerm = pow(1.0 - NdotV, fresnelParms2.w);   

float rimLight = fresnelTerm * clamp(NdotL - 0.3, 0.0, fresnelParms.z) * lightParms.y;

 

If I comment those out, the console shows failure to compile the program. Missing declarations.

But the problem colour boxes does go away, but a lot of the lighting is broken.

 

[AluminumHaste]

41 minutes ago, stgatilov said:

Since adding specularmap helped you, I think "specularColor" value should be a good start.

If you reset it to zero after it gets computed, does the issue go away?

 

I think I did this right:

vec3 specularColor = specularCoeff * fresnelCoeff * specular * (diffuse * 0.25 + vec3(0.75));

So right after I set the value to all 0s.

specularColor = vec3(0.0);

 

Compiles just fine, but still blue flickering boxes around the windows.

[nbohr1more]

Just now, AluminumHaste said:

 

If I comment those out, the console shows failure to compile the program. Missing declarations.

 

Sorry, you've got to remove them from the subsequent parts of the shader or define them as = 1.0. The latter might be easier.

[AluminumHaste]

@stgatilov@nbohr1more

I'm in discord if you wanna hop on and chat.

So I also defined them as 1.0, no change.

[AluminumHaste]

To be clear, I am seeing specular highlight changes, everything specular is really bright with them defined at 1.0.

[AluminumHaste]

This still doesn't appear to be a specular highlight issue, as even with the fixes added to the latest source, I can still get what looks like buffer corruption.

[nbohr1more]

I am wondering why we can't just change

return totalColor

to

return max(totalColor, 0);

?

Is there any reason why we would ever want interaction.common to output negative values?

If this addresses the window bug, it will probably be good enough for 2.10, and then we can revert in SVN and dig further if needed.

@AluminumHaste try this one. It needs to go under glprogs/stages/interaction/ and you need to set r_useNewBackend 1

interaction.common.fs.glsl

[stgatilov]

7 hours ago, AluminumHaste said:

This still doesn't appear to be a specular highlight issue, as even with the fixes added to the latest source, I can still get what looks like buffer corruption.

Maybe something is bad with your driver or GPU?

The menu has nothing to do with interaction shader, in fact its shader is very simple. Maybe texture compression is broken? Maybe try some of the new cvars like image_mipmapMode and image_useTexStorage.

2 hours ago, nbohr1more said:

Is there any reason why we would ever want interaction.common to output negative values?

Why do you think there is some problem with negative values?

[nbohr1more]

1 hour ago, stgatilov said:

Why do you think there is some problem with negative values?

Because I fixed the same artifact in totallytubular's shader by clamping to positive values.

The only reason a bloom blend would darken the screen is if it were sent negative values or sent positive values so large that they become negative. If clamping cured 90% of the issue in the bloom upsample \ downsample shaders then I say it's worth a shot to try here further at the core of the shader pipeline.

[stgatilov]

1 hour ago, nbohr1more said:

The only reason a bloom blend would darken the screen is if it were sent negative values or sent positive values so large that they become negative.

Floating point arithmetic has no wrap-around unlike integer arithmetic.
You cannot add large positive numbers to get a negative one, you can only get positive infinity. If you multiply it by zero, you get a NaN. And if you have a NaN, it will always remain NaN, having no sign at all. It does not matter whether the image becomes brighter or darker, just some pixels become NaN.

[nbohr1more]

2 minutes ago, stgatilov said:

Floating point arithmetic has no wrap-around unlike integer arithmetic.
You cannot add large positive numbers to get a negative one, you can only get positive infinity. If you multiply it by zero, you get a NaN. And if you have a NaN, it will always remain NaN, having no sign at all. It does not matter whether the image becomes brighter or darker, just some pixels become NaN.

Thanks!

So if we use a "max" function between the NaN and 0 the driver appears to choose 0 but it cannot be counted on as the behavior of max between 0 and NaN is undefined? I guess I would hope that driver writers should know that 0 is the best choice there.

[stgatilov]

37 minutes ago, nbohr1more said:

So if we use a "max" function between the NaN and 0 the driver appears to choose 0 but it cannot be counted on as the behavior of max between 0 and NaN is undefined? I guess I would hope that driver writers should know that 0 is the best choice there.

If X = Nan, then IEEE-compliant implementation should produce:

• (x > 0 ? x : 0) => 0
• (x >= 0 ? x : 0) => 0
• (x < 0 ? 0 : x) => NaN
• (x <= 0 ? 0 : x) => NaN

We don't know how exactly max/min is implemented in GLSL, same for clamp.

Even worse, OpenGL does not force any particular behavior of NaN onto implementations: they can are free to have NaN or e.g. immediately reset NaN result to zero. I believe using NaN in any builtin function including clamp and min/max is undefined behavior according to the spec.

The worst thing is that compiler optimizations can change the way comparisons work. On C++ side, this is usually controlled by "fast math" flag. Since we don't enable it, we can be certain that we can work with NaNs properly in C++. But I think GPU code is optimized more aggressively, so who knows what compiler can or can't do.