DescriptionStefan "#@" Langer
2007-02-15 20:25:23 EST
INFO:
shaderTextHashTable is the hash-table for not loaded shaders (which helps findig the right "textblock")
this sourcecode was build like it should do this "file by file" (also all shadertext is in one huge char-array ... maybe the "file by file"-suff is to make shader-file "debugging" a bit easier)
THE BUG:
- each "file" will be aborted after the first token
- all hashes will be build with the last "file"
=> so, foreach first "shaderblock" there are two hashtable entries
( and it makes it hard to understand why this code works at all ;) )
SOLUTION:
Index: code/renderer/tr_shader.c
===================================================================
--- code/renderer/tr_shader.c (revision 1045)
+++ code/renderer/tr_shader.c (working copy)
@@ -2923,20 +2923,15 @@
// look for label
while ( 1 ) {
token = COM_ParseExt( &p, qtrue );
- if ( token[0] == 0 ) {
+ // break, if we passed the end of this file
+ // memoryaddresses are reversed to the array indexes !
+ if ( token[0] == 0 || ( i-1 >= 0 && p >= buffers[i-1] ))
break;
- }
hash = generateHashValue(token, MAX_SHADERTEXT_HASH);
shaderTextHashTableSizes[hash]++;
size++;
SkipBracedSection(&p);
- // if we passed the pointer to the next shader file
- if ( i < numShaders - 1 ) {
- if ( p > buffers[i+1] ) {
- break;
- }
- }
}
}
@@ -2958,20 +2953,15 @@
while ( 1 ) {
oldp = p;
token = COM_ParseExt( &p, qtrue );
- if ( token[0] == 0 ) {
+ // break, if we passed the end of this file
+ // memoryaddresses are reversed to the array indexes !
+ if ( token[0] == 0 || ( i-1 >= 0 && p >= buffers[i-1] ) )
break;
- }
hash = generateHashValue(token, MAX_SHADERTEXT_HASH);
shaderTextHashTable[hash][shaderTextHashTableSizes[hash]++] = oldp;
SkipBracedSection(&p);
- // if we passed the pointer to the next shader file
- if ( i < numShaders - 1 ) {
- if ( p > buffers[i+1] ) {
- break;
- }
- }
}
}
Setting a QA contact on all ioquake3 bugs, even resolved ones. Sorry if you get a flood of email from this, it should only happen once. Apologies for the incovenience.
--ryan.
Well done for spotting the bug, I can't imagine how you came across it.
Turns out it's not really as bad as you think. For the first n - 1 shader files (where n is the number of shader files) it is broken as you correctly point out. When it reaches the last shader file however, ( i < numShaders - 1 ) becomes false, so the check for the next file is never actually made. Since buffers[] is backwards relative to s_shaderText, p now effectively points to s_shaderText. So the loading the last file actually hashes ALL the shaders. This is why it doesn't /appear/ broken :)
So in actual fact, rather than a sparse hash table, it's actually overpopulated to the tune of n - 1 entries.
Regardless of the result, this code is totally wacky, so I've fixed it by getting rid of buffers[] after loading from the files and just hashing directly on s_shaderText.
Fixed, pending commit.
INFO: shaderTextHashTable is the hash-table for not loaded shaders (which helps findig the right "textblock") this sourcecode was build like it should do this "file by file" (also all shadertext is in one huge char-array ... maybe the "file by file"-suff is to make shader-file "debugging" a bit easier) THE BUG: - each "file" will be aborted after the first token - all hashes will be build with the last "file" => so, foreach first "shaderblock" there are two hashtable entries ( and it makes it hard to understand why this code works at all ;) ) SOLUTION: Index: code/renderer/tr_shader.c =================================================================== --- code/renderer/tr_shader.c (revision 1045) +++ code/renderer/tr_shader.c (working copy) @@ -2923,20 +2923,15 @@ // look for label while ( 1 ) { token = COM_ParseExt( &p, qtrue ); - if ( token[0] == 0 ) { + // break, if we passed the end of this file + // memoryaddresses are reversed to the array indexes ! + if ( token[0] == 0 || ( i-1 >= 0 && p >= buffers[i-1] )) break; - } hash = generateHashValue(token, MAX_SHADERTEXT_HASH); shaderTextHashTableSizes[hash]++; size++; SkipBracedSection(&p); - // if we passed the pointer to the next shader file - if ( i < numShaders - 1 ) { - if ( p > buffers[i+1] ) { - break; - } - } } } @@ -2958,20 +2953,15 @@ while ( 1 ) { oldp = p; token = COM_ParseExt( &p, qtrue ); - if ( token[0] == 0 ) { + // break, if we passed the end of this file + // memoryaddresses are reversed to the array indexes ! + if ( token[0] == 0 || ( i-1 >= 0 && p >= buffers[i-1] ) ) break; - } hash = generateHashValue(token, MAX_SHADERTEXT_HASH); shaderTextHashTable[hash][shaderTextHashTableSizes[hash]++] = oldp; SkipBracedSection(&p); - // if we passed the pointer to the next shader file - if ( i < numShaders - 1 ) { - if ( p > buffers[i+1] ) { - break; - } - } } }