tr_sky: clear color buffer when fastsky is enabled, do not clear screen by default

[illwieckz]

Clear color buffer but nothing else, even if r_clear is disabled, there can be issues when enabling complete clearing by default
with Nvidia proprietary drivers. So clearing the color buffer there prevents void effect in skipped sky without needing to enable r_clear.

Anyway, we may want to do it (and even force a color) on purpose, this would mean clearing is the explicit way to draw the fast sky.

This makes possible to workaround #472 by keeping r_clear disabled while clearing the color buffer when r_fastsky is enabled but not clearing other buffers (because disabled r_clear).

This DOES NOT fix #472. We still have to figure out why we get what looks like precision loss on Nvidia proprietary driver.

[slipher]

How about combining this with the if ( r_clear->integer ) test, so that we know it's not done in an inappropriate place and not done twice?

Also it seems the default value for r_clear should be changed back to 0.

[illwieckz]

How about combining this with the if ( r_clear->integer ) test, so that we know it's not done in an inappropriate place and not done twice?

Done.

An alternative way would be to do if ( r_fastsky->integer ) in the actual clearing code. What do you think about it?

Also it seems the default value for r_clear should be changed back to 0.

Done.

We would also have to edit Unvanquished presets as well.

This code was successfully tested with Nvidia Quadro K1100M GPU using Nvidia 390.143 driver on Linux 5.8.0-53-generic on Ubuntu 20.04.2.

[illwieckz]

An alternative way would be to do if ( r_fastsky->integer ) in the actual clearing code. What do you think about it?

Hmm, doing this would probably just bring back the bug.

[slipher]

An alternative way would be to do if ( r_fastsky->integer ) in the actual clearing code. What do you think about it?

Yes, that's what I was trying to say. Do all the clearing in one place, the place where it is now.

Hmm, doing this would probably just bring back the bug.

Well if you have both the affected hardware+drivers, and use r_fastsky, then you will probably get the bug, regardless of where you do the clearing. The hardware seems reasonably recent though, so it shouldn't be necessary to use fastsky with it?

[illwieckz]

Well if you have both the affected hardware+drivers, and use r_fastsky, then you will probably get the bug, regardless of where you do the clearing. The hardware seems reasonably recent though, so it shouldn't be necessary to use fastsky with it?

The thing is that, when I clear color buffer in tr_sky and then the actual clearing code in tr_backend only clears depth and stencil buffer because r_clear is disabled but r_fastsky is enabled, the bug is gone. But if the code in tr_backend clears color, depth and stencil buffer because r_clear is enabled, the bug is there (that's actual 0.52 behaviour).

Basically the clearing code in tr_backend does:

	int clearBits = GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT;
	
	// clear relevant buffers
	if ( r_clear->integer ) {
		clearBits |= GL_COLOR_BUFFER_BIT;
	}

	glClear( clearBits );

If we do:

	int clearBits = GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT;
	
	// clear relevant buffers
	if ( r_clear->integer || r_fastsky->integer ) {
		clearBits |= GL_COLOR_BUFFER_BIT;
	}

	glClear( clearBits );

I expect the bug to be back, because it would run with r_fastsky enabled the exact same code we run when r_clear is enabled… I have not seen other if ( r_clear->integer ) in the code, so everything seems to be done there.

The hardware seems reasonably recent though, so it shouldn't be necessary to use fastsky with it?

For my K1100M GPU, yes r_fastsky is unneeded, but the bug has been reproduced on Nvidia 465, 390 and 340. The Geforce 8400 GS rev.2 from 2007 (freem's GPU) is running the Nvidia 340 driver and was reported to be playable with low preset at 1024×768 resolution, but this was before I enabled r_fastsky in lowest preset, and it's now known enabling r_fastsky brings sensible performance boost on such hardware. So, we know it's both possible to want to use r_fastsky (requiring clearing) on Nvidia hardware that is expected to be affected by the clearing bug.

[illwieckz]

I confirm, doing this in tr_backend.cpp just brings back the bug:

	int clearBits = GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT;
	
	// clear relevant buffers
	if ( r_clear->integer || r_fastsky->integer ) {
		clearBits |= GL_COLOR_BUFFER_BIT;
	}

	glClear( clearBits );

While doing both this in tr_backend.cpp (actual 0.52 code):

	int clearBits = GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT;
	
	// clear relevant buffers
	if ( r_clear->integer ) {
		clearBits |= GL_COLOR_BUFFER_BIT;
	}

	glClear( clearBits );

and that in tr_sky.cpp (actual PR), workarounds the bug:

	if ( r_fastsky->integer )
	{
		if ( ! r_clear->integer )
		{
			glClear( GL_COLOR_BUFFER_BIT );
		}

		// …
	}

[illwieckz]

I confirm the Geforce 8400 GS rev.2 (from 2007) is affected as well, I own a PCI version of it (which is then even slower than freem's one)

Using fast sky on that particularly slow GPU brings ~7 fps more when there is a sky to draw.

Fast sky, complete clearing (color, stencil, depth clearing):

No fast sky, no clearing:

I noticed not clearing saved 2 fps on that particularly slow GPU, it's the first time I see a difference between with or without clearing:

No clearing:

Complete clearing:

Because this GPU is super slow (while being featureful: OpenGL 3, 512M of VRAM), I used the lowest preset as a basis and 1024×768 game resolution.

I was not able to test the performance of “only clear color buffer on fast sky” because of #474, the tests were done with 0.52 release build.

On my laptop running Ubuntu 20.04, Nvidia 390 driver and 5.8 kernel, I can test properly the PR, but the hardware is too much performant to get significant numbers on clearing performance. At least we know it fixes the bug.

[illwieckz]

I just thought about something, maybe that patch workarounds the bug on that plat23 scene only because there is no sky on that alien base, so the color buffer clearing called by fastsky code does not run.

I still believe this patch is good to have because fastsky code explicitely requires color buffer clearing to work, and with that code, color buffer clearing would be only done when fastsky code runs (if r_clear is disabled).

[slipher]

Some comments from here:

Daemon#473 seems doubtful to me. Tess_StageIteratorSky is part of the surface drawing pipeline. Clearing of course needs to be done before anything is drawn. So clearing the screen after surfaces are already being drawn risks erasing things.

But isn't sky meant to be drawn before anything else? As far as I know that's part of why we can't draw sky over other things and then, sometime objects from other rooms can be seen through the sky… because sky is meant to be drawn before.

In the normal case I suppose the sky should always have the largest depth and be sorted first. But I'm not sure it couldn't happen in some case like with cameras/portals etc.

[illwieckz]

Or maybe @VReaperV being a sky masterwizard knows how to make an alternative sky code that just paints to black the sky area without clearing (and as fast as clearing) when r_fastsky is enabled, so we can always disable clearing.

[VReaperV]

Or maybe @VReaperV being a sky masterwizard knows how to make an alternative sky code that just paints to black the sky area without clearing (and as fast as clearing) when r_fastsky is enabled, so we can always disable clearing.

We can probably put it as a uniform or define in the skybox shader and just set output color to black. I doubt it would be faster than doing a glClear() though.

[illwieckz]

Yes, the idea is to skip entirely the skybox shader.

[VReaperV]

Yes, the idea is to skip entirely the skybox shader.

I'm not sure there's really any way to do that other than by just clearing the framebuffer (well, any sane way that is).

[illwieckz]

Another implementation can be:

diff --git a/src/engine/renderer/tr_backend.cpp b/src/engine/renderer/tr_backend.cpp
index 39665b4ef..775e5b7ec 100644
--- a/src/engine/renderer/tr_backend.cpp
+++ b/src/engine/renderer/tr_backend.cpp
@@ -5623,7 +5623,7 @@ const RenderCommand *ClearBufferCommand::ExecuteSelf( ) const
 	GL_State( GLS_DEFAULT );
 
 	// clear relevant buffers
-	if ( r_clear->integer ) {
+	if ( r_clear->integer || r_fastsky->integer ) {
 		clearBits |= GL_COLOR_BUFFER_BIT;
 	}

There would be no question about the clearing being done at the right place or not.

[illwieckz]

I removed the archive flag on r_fastsky when porting it to new cvar format, because I see no reason to make it archive by default, in fact I doubt there is a lot of cvars that require to be set as archive from the start (the /seta command exists for a good reason)…

[VReaperV]

This works fine on my end with all 4 combinations of r_clear and r_fastsky.

[VReaperV]

LGTM.