don't re-check uniform name when initializing textures

[claudeha]

If !(l < 0) then the uniform is found, no need to check strings. Somehow this check is broken, I don't know why, but removing it makes textures display again.

Fixes https://github.com/3Dickulus/FragM/issues/104

[3Dickulus]

This tests the uniform name is in QT object...

            int l = shaderProgram->uniformLocation ( textureUniformName );

            if ( !(l < 0) ) { // found named texture in shader program

this checks the GL object referenced name and the cache name

                glGetActiveUniform(shaderProgram->programId(), l, bufSize, &length, &size, &type, name);
                // check cache first
                if ( !TextureCache.contains ( texturePath ) && textureUniformName == QString(name).trimmed() ) {

... makes sure the texture is in the Qt object, the GL object and that the name is not already in the cache ... I found that these tests prevent textures from being loaded from file every time, only keeps textures in the cache that are used.

just ran the frag render tests...

... they all seem to render fine when called directly or via fQScript, hdr,exr,png... ... tested with USE_OPENGL_4 = true and USE_OPENGL_4 = false (false requires wrapping the GLAPIENTRY MessageCallback stuff with #ifdef USE_OPENGL_4 ... #endif in DisplayWidget::initializeGL() and around MessageCallback declaration)

note: the diffuse object surface in No.3, this frag loads 3 textures, specular, diffuse and background, this frag did not render properly before my latest commit re:textures

if you're getting black screen then I suspect something else is the cause, this is just a symptom ?

[claudeha]

This value of l:

            int l = shaderProgram->uniformLocation ( textureUniformName );

Is not a semantically valid input argument of:

                glGetActiveUniform(shaderProgram->programId(), l, bufSize, &length, &size, &type, name);

That it works on your machine is just luck afaict w.r.t. the specification/documentation. More info in the second commit I pushed in the PR...

[3Dickulus]

image No.4 is GL_SAMPLER_CUBE

the sampler type is handled elsewhere in DisplayWidget::initFragmentTextures() and in load*texture functions, this is mostly about getting the cache and initial file loading where textureID is cached with uniform name for later use.

                if ( loaded ) {
                    glBindTexture((type == GL_SAMPLER_CUBE) ? GL_TEXTURE_CUBE_MAP : GL_TEXTURE_2D, textureID);

[3Dickulus]

l should be GLuint for glGetActiveUniform( GLuint program, GLuint index, GLsizei bufSize, GLsizei length, GLint size, GLenum type, GLchar name )

...trying to follow but not getting there...

[3Dickulus]

tested original code vs this commit...

original...

"DisplayWidget.cpp" 974 initFragmentTextures 3 Background "Background"
"DisplayWidget.cpp" 974 initFragmentTextures 13 Diffuse "Diffuse"
"DisplayWidget.cpp" 974 initFragmentTextures 34 Specular "Specular"

this commit...

"DisplayWidget.cpp" 994 initFragmentTextures 3 Background "Background"
"DisplayWidget.cpp" 994 initFragmentTextures 13 Diffuse "Diffuse"
"DisplayWidget.cpp" 994 initFragmentTextures 34 Specular "Specular"

identical behavior, same indices and uniform name.

[claudeha]

On 22/12/2019 21:55, 3Dickulus wrote:||

identical behavior, same indices and uniform name.

not guaranteed by the spec, you are just (un)lucky I think

what about forcing explicit locations to get discontiguous range?

layout (location = 666) uniform sampler2D Santa;

may need an extension...

see also the comment on bug report #104

[3Dickulus]

layout (location = 666) uniform sampler2D Santa;

anything beyond legacy like that needs to be written into the parser, probably not until v3.0?

the way glGetActiveUniform() is used is fine for this purpose, the buffer size is adequate, show me a case where the uniform name is larger than 256 chars length, size, type and name are returned values, I suppose that they could be initialized to 0s rather than leaving them uninitialized :-/ this is a small optimization over calling gl functions numerous times to fill in the buffer size with an exact value. Not "illegal" in GL terms I think.

the problem you describe in #104 seems related to backbuffer uniform being in the fragmentSource.textures map, it should not be there.

edit:not trying to be difficult, just don't want to apply a patch that treats a symptom but leaves the possible cause unaddressed.

[claudeha]

On 23/12/2019 00:41, 3Dickulus wrote:

anything beyond legacy like that needs to be written into the parser, probably not until v3.0?

Perhaps it would not confuse the FragM parser if you put it on 2 lines?

layout (location = 666)
uniform sampler2D Santa; file[redsuit.jpg]

the way glGetActiveUniform() is used is fine for this purpose, the buffer size is adequate, show me a case where the uniform name is larger than 256 chars

Why not do it correctly instead of having a potential bug (that would be hard to diagnose)?

calling gl functions numerous times to fill in the buffer size with an exact value.

It's one extra call to find the maximum buffer size needed for the whole shader, not a big deal for correctness?

the problem you describe in #104 https://github.com/3Dickulus/FragM/issues/104 seems related to backbuffer uniform being in the fragmentSource.textures map, it should not be there.

No it's a type mismatch: index and location are different things

[3Dickulus]

yes, for correctness...

    glGetProgramiv(shaderProgram->programId(), GL_ACTIVE_UNIFORM_MAX_LENGTH, &bufSize);
    GLchar name[bufSize];

and, for robustness...

    if(textureUniformName == QString(name).trimmed() && (type == GL_SAMPLER_2D || type == GL_SAMPLER_CUBE) ) {
        // check cache first
        if ( !TextureCache.contains ( texturePath ) ) {
    ...
        } else { // use cache
    ...
        }
    }

why this test: target uniform name is used to get an index, the index is then used to acquire length, size, type and returns name as well, so testing the name returned by index query against the target uniform name used to get the index ensures that the index is correct, then testing the type (type == GL_SAMPLER_2D || type == GL_SAMPLER_CUBE) ensures that no attempt is made to initialize, cache or otherwise use a uniform that isn't properly referenced as a supported texture.

not sure what to do with the conflict, I want to commit the above, but should resolve conflict first? or?

[claudeha]

I'd say implement the better fixes on the Development branch and close this PR unmerged? As long as the code is fixed/working then it's all good.