Skip to content

Vertex skinning compatible with old GLSL#498

Merged
slipher merged 2 commits intoDaemonEngine:masterfrom
slipher:vertexskin
Jul 6, 2021
Merged

Vertex skinning compatible with old GLSL#498
slipher merged 2 commits intoDaemonEngine:masterfrom
slipher:vertexskin

Conversation

@slipher
Copy link
Copy Markdown
Member

@slipher slipher commented Jul 4, 2021

Fixes #490. TODO: Test on the Nvidia driver to make sure it still works.

@illwieckz
Copy link
Copy Markdown
Member

illwieckz commented Jul 4, 2021

I successfully tested this code, compiling successfully the GLSL code and not seeing any artifacts on those three configurations:

GL_VENDOR: Intel Open Source Technology Center
GL_RENDERER: Mesa DRI Intel(R) 965GM (CL)
GL_VERSION: 2.1 Mesa 20.0.8
GL_SHADING_LANGUAGE_VERSION: 1.20
GL_MAX_VERTEX_UNIFORM_COMPONENTS 16384
Using GPU vertex skinning with max 256 bones in a single pass
GL_VENDOR: NVIDIA Corporation
GL_RENDERER: Quadro K1100M/PCIe/SSE2
GL_VERSION: 3.2.0 NVIDIA 390.143
GL_SHADING_LANGUAGE_VERSION: 1.50 NVIDIA via Cg compiler
GL_MAX_VERTEX_UNIFORM_COMPONENTS 4096
Using GPU vertex skinning with max 233 bones in a single pass
GL_VENDOR: AMD 
GL_RENDERER: AMD Radeon (TM) R9 390 Series (HAWAII, DRM 3.38.0, 5.8.0-59-generic, LLVM 11.0.0) 
GL_VERSION: 4.6 (Core Profile) Mesa 21.0.1 
GL_SHADING_LANGUAGE_VERSION: 4.60 
GL_MAX_VERTEX_UNIFORM_COMPONENTS 16384
Using GPU vertex skinning with max 256 bones in a single pass 

The scene was +devmap plat23 +delay 100f setviewpos 2227 2113 249 40 25

@illwieckz
Copy link
Copy Markdown
Member

illwieckz commented Jul 4, 2021

LGTM, though I liked that very fast “one multiplication, two bitwise operations” code… 😥

I would still greatly appreciate a comment around this code saying to future readers a rewrite of this code may break support for multiple configurations on various ways. 🙂

@illwieckz
Copy link
Copy Markdown
Member

Though, for the GL error thing, I find weird to change the default according to the build

@illwieckz
Copy link
Copy Markdown
Member

illwieckz commented Jul 5, 2021

for the glerror thing, why not doing three values:

  • 0: disable
  • 1: enable
  • -1: enable only on debug build (default)

Edit: or 2.

@slipher
Copy link
Copy Markdown
Member Author

slipher commented Jul 5, 2021

I would still greatly appreciate a comment around this code saying to future readers a rewrite of this code may break support for multiple configurations on various ways. slightly_smiling_face

Done.

for the glerror thing, why not doing three values:

* `0`: disable

* `1`: enable

* `-1`: enable only on debug build (default)

This makes sense but it's a little annoying to do, since the value is checked in 6 different places.

@illwieckz
Copy link
Copy Markdown
Member

Why not something like this?

commit 372dbfe7a3f36f1f1d19af788a5d27da733c6252
Author: Thomas Debesse <dev@illwieckz.net>
Date:   Mon Jul 5 10:58:58 2021 +0200

    renderer: seonly check for GL errors on debug build with r_ignoreGLErrors -1

diff --git a/src/engine/renderer/tr_backend.cpp b/src/engine/renderer/tr_backend.cpp
index b69f69830..418d485ec 100644
--- a/src/engine/renderer/tr_backend.cpp
+++ b/src/engine/renderer/tr_backend.cpp
@@ -1368,7 +1368,7 @@ static void RB_SetupLightForShadowing( trRefLight_t *light, int index,
 							      tr.shadowCubeFBOImage[ light->shadowLOD ]->texnum, 0 );
 				}
 
-				if ( !r_ignoreGLErrors->integer )
+				if ( !ignoreGLErrors() )
 				{
 					R_CheckFBO( tr.shadowMapFBO[ light->shadowLOD ] );
 				}
@@ -1488,7 +1488,7 @@ static void RB_SetupLightForShadowing( trRefLight_t *light, int index,
 					R_AttachFBOTexture2D( GL_TEXTURE_2D, tr.shadowMapFBOImage[ light->shadowLOD ]->texnum, 0 );
 				}
 
-				if ( !r_ignoreGLErrors->integer )
+				if ( !ignoreGLErrors() )
 				{
 					R_CheckFBO( tr.shadowMapFBO[ light->shadowLOD ] );
 				}
@@ -1540,7 +1540,7 @@ static void RB_SetupLightForShadowing( trRefLight_t *light, int index,
 					R_AttachFBOTextureDepth( tr.sunShadowMapFBOImage[ splitFrustumIndex ]->texnum );
 				}
 
-				if ( !r_ignoreGLErrors->integer )
+				if ( !ignoreGLErrors() )
 				{
 					R_CheckFBO( tr.sunShadowMapFBO[ splitFrustumIndex ] );
 				}
@@ -1983,7 +1983,7 @@ static void RB_BlurShadowMap( const trRefLight_t *light, int i )
 	R_BindFBO( fbos[ index ] );
 	R_AttachFBOTexture2D( images[ index + MAX_SHADOWMAPS ]->type, images[ index + MAX_SHADOWMAPS ]->texnum, 0 );
 
-	if ( !r_ignoreGLErrors->integer )
+	if ( !ignoreGLErrors() )
 	{
 		R_CheckFBO( fbos[ index ] );
 	}
diff --git a/src/engine/renderer/tr_cmds.cpp b/src/engine/renderer/tr_cmds.cpp
index be572cce6..4226613b0 100644
--- a/src/engine/renderer/tr_cmds.cpp
+++ b/src/engine/renderer/tr_cmds.cpp
@@ -811,7 +811,7 @@ void RE_BeginFrame()
 	}
 
 	// check for errors
-	if ( !r_ignoreGLErrors->integer )
+	if ( !ignoreGLErrors() )
 	{
 		R_SyncRenderThread();
 		GL_CheckErrors_( __FILE__, __LINE__ );
diff --git a/src/engine/renderer/tr_init.cpp b/src/engine/renderer/tr_init.cpp
index 0b2f93fca..3c76b461f 100644
--- a/src/engine/renderer/tr_init.cpp
+++ b/src/engine/renderer/tr_init.cpp
@@ -379,7 +379,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 		int  err;
 		char s[ 128 ];
 
-		if ( r_ignoreGLErrors->integer )
+		if ( ignoreGLErrors() )
 		{
 			return;
 		}
diff --git a/src/engine/renderer/tr_local.h b/src/engine/renderer/tr_local.h
index 7d560a943..5314fb6cd 100644
--- a/src/engine/renderer/tr_local.h
+++ b/src/engine/renderer/tr_local.h
@@ -3033,6 +3033,20 @@ static inline void halfToFloat( const f16vec4_t in, vec4_t out )
 
 //====================================================================
 
+/* r_ignoreGLErrors values:
+- -1: check for GL errors on debug build only
+- 0: never check for GL errors
+- 1: always check for GL errors */
+
+inline bool ignoreGLErrors ()
+{
+#ifdef DEBUG_BUILD
+	return !!r_ignoreGLErrors->integer;
+#else
+	return r_ignoreGLErrors->integer < 1;
+#endif
+}
+
 #define IMAGE_FILE_HASH_SIZE 4096
 	extern image_t *r_imageHashTable[ IMAGE_FILE_HASH_SIZE ];
 

slipher and others added 2 commits July 6, 2021 02:45
Fixes DaemonEngine#490 (the method using bitwise operations would not compile if
someone had support for only GLSL v1.20).
@slipher
Copy link
Copy Markdown
Member Author

slipher commented Jul 6, 2021

Why not something like this?

OK.

@illwieckz
Copy link
Copy Markdown
Member

If the code is still the same as the one I tested in #498 (comment)

then LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nvidia fix likely broke GLSL 1.20 GPUs

2 participants