3Dickulus / FragM

Derived from https://github.com/Syntopia/Fragmentarium/
GNU General Public License v3.0
349 stars 30 forks source link

fix crash due to wrong index in foundDouble branch #133

Closed claudeha closed 4 years ago

claudeha commented 4 years ago

i is relative to GL_ACTIVE_UNIFORMS, not vw.size(), so we need to search the right n by comparing uniform names.

3Dickulus commented 4 years ago

No crash here in my tests, do you have an example frag that crashes at this point?

it is the correct index, the index in the slider list is not the same as the index in the compiled fragment due to unused vars being optimized out. note a little earlier in that code segment names do get tested after the comment // find a value to go with the name, index in the program, may not be the same as index in our list

inserting DBOUT << i << foundDouble << uniformName; immediately before vw[i]->setIsDouble(foundDouble); generates this output...

"DisplayWidget.cpp" 1503 setShaderUniforms 0 true "AAExp"
"DisplayWidget.cpp" 1503 setShaderUniforms 1 true "AARange"
"DisplayWidget.cpp" 1503 setShaderUniforms 2 true "B"
"DisplayWidget.cpp" 1503 setShaderUniforms 3 true "Bailout"
"DisplayWidget.cpp" 1503 setShaderUniforms 4 true "Center"
"DisplayWidget.cpp" 1503 setShaderUniforms 5 true "ColDiv"
"DisplayWidget.cpp" 1503 setShaderUniforms 8 true "G"
"DisplayWidget.cpp" 1503 setShaderUniforms 9 true "Gamma"
"DisplayWidget.cpp" 1503 setShaderUniforms 12 true "InvertC"
"DisplayWidget.cpp" 1503 setShaderUniforms 15 true "JuliaXY"
"DisplayWidget.cpp" 1503 setShaderUniforms 17 true "Power"
"DisplayWidget.cpp" 1503 setShaderUniforms 19 true "R"
"DisplayWidget.cpp" 1503 setShaderUniforms 20 true "RetryBias"
"DisplayWidget.cpp" 1503 setShaderUniforms 21 true "RetryEntry"
"DisplayWidget.cpp" 1503 setShaderUniforms 30 true "TrigLimit"
"DisplayWidget.cpp" 1503 setShaderUniforms 31 true "Zoom"
"DisplayWidget.cpp" 1503 setShaderUniforms 0 true "Brightness"
"DisplayWidget.cpp" 1503 setShaderUniforms 1 true "Contrast"
"DisplayWidget.cpp" 1503 setShaderUniforms 2 true "Exposure"
"DisplayWidget.cpp" 1503 setShaderUniforms 3 true "Gamma"
"DisplayWidget.cpp" 1503 setShaderUniforms 4 true "Saturation"

with no crash.

3Dickulus commented 4 years ago

the reason the "else" clause got put there was to make sure the double type vars in the buffershader get flagged as doubles, without it the sliders only get 9 decimal places instead of 18

3Dickulus commented 4 years ago

...some testing shows that the vars in buffershader (Post tab) are all set as double after removing that else clause completely... maybe a leftover and not needed at all any more ?

my mistake, you are correct, n indicates the index in the source and i indicates the index in the program... do all double sliders get the right decimal places on debian if you remove that last else clause at line 1502 completely ?

edit: instead of adding code to fix that I would rather just remove the offending code, it was to be a catch-all iirc and as the addition of double types evolved I think it may be obsoleted...

still trying to figure out why the DoubleTest.frag doesn't show the problem here...

3Dickulus commented 4 years ago

when I use this code it says all are set so this else clause can be removed

            for( int n=0; n < vw.count(); n++) {
                if(uniformName == vw[n]->getName()) {
                    if(vw[n]->isDouble()) {
                        DBOUT << "Set:" << n << foundDouble << uniformName;
                    }
                    else {
                        DBOUT << "Setting:" << n << foundDouble << uniformName;
                        vw[n]->setIsDouble(foundDouble);
                    }
                }
            }

Thanks for catching that, still wondering though, how it crashed for you but not here?

3Dickulus commented 4 years ago

aw fudge f'd up

committed filedialog changes and caused conflict with DisplayWidget.cpp

I think this is resolved by removing the else clause at line 1502... committing that now and closing this

3Dickulus commented 4 years ago

ok don't know quite what went on there... that else clause is required! without it buffershader sliders flip to 9 decimal places when switching tabs between a float only frag and a doubles enabled frag...

so your commit is good, I must be loosin' it :(