3Dickulus / FragM

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

textures don't load (black screen) #104

Closed claudeha closed 4 years ago

claudeha commented 4 years ago

Describe the bug Textures don't load: blank black screen.

To Reproduce

#version 330 compatibility

#include "MathUtils.frag"
#include "Progressive2D.frag" 

uniform sampler2D Texture; file[/home/claude/DSC04022.jpg]

vec3 color(vec2 pos) {
    return texture(Texture, pos).rgb;
}

(replace texture path with your own file)

Expected behavior Texture image is displayed

Desktop (please complete the following information):

Additional context I tried .jpg, .png, .exr - none of which worked. It says it loads the texture in the log but image remains black.

claudeha commented 4 years ago

git bisect tells me its my fault (003c740f25c3dc784a3a60f02114b5989bb68ec1):

003c740f25c3dc784a3a60f02114b5989bb68ec1 is the first bad commit
commit 003c740f25c3dc784a3a60f02114b5989bb68ec1
Author: Claude Heiland-Allen <claude@mathr.co.uk>
Date:   Thu Aug 29 01:59:26 2019 +0100
3Dickulus commented 4 years ago

I have been trying to reproduce to no avail, textures load as they should, your frag works as expected here ??? black on first build (filename) renders perfectly after using a local file !!!

using latest push on Development branch seems fine.

claudeha commented 4 years ago

http://docs.gl/gl3/glGetActiveUniform says the argument is an index, which is not necessarily the same as a uniform location. So the glGetActiveUniform() call is entirely bogus, the only way to use it is to iterate over indices up to the maximum returned.

Indeed glGetActiveUniform() returns the name of a different uniform variable in my shader (backbuffer instead of Texture, for this example; Seed instead of Gradient for my real shader).

This is likely OpenGL-driver specific behaviour.

3Dickulus commented 4 years ago

http://docs.gl/gl3/glGetActiveUniform

index Specifies the index of the uniform variable to be queried.

no where on that page does it say "which is not necessarily the same as a uniform location."

glGetActiveUniform() expects index of the uniform variable to be queried from a linked program object === shaderProgram->uniformLocation ( textureUniformName );

if textureUniformName = backbuffer then fragmentSource.textures is wrong when QMapIterator<QString, QString> it( fragmentSource.textures ); is called.

is the problem that backbuffer should not be in the map fragmentSource.textures ? how is it getting there?

claudeha commented 4 years ago

On 22/12/2019 22:59, 3Dickulus wrote:

no where on that page does it say "which is not necessarily the same as a uniform location."

uniform locations can be non-contiguous (eg using explicit layout locations in the shader source, you could have uniforms at 1 and 3 but not 0 or 2)

active uniform indices are necessarily contiguous (you can get the count and iterate densely from 0)

so they can't be the same thing in general.

it's a bit of a confusing API but that's how I understand it...

3Dickulus commented 4 years ago

l = index in program which is not equal to the index in the source, some get optimized out or as constants or explicitly located, yes, but the location in program, 0,1,n... after linking is exactly what glGetActiveUniform() expects. edit: contiguous is not expected.

follow it back and figure out why backbuffer is in the map fragmentSource.textures

3Dickulus commented 4 years ago

backbuffer is bound to GL_TEXTURE0 all other textures are GL_TEXTURE0+n... backbuffer uniform should never be in the fragmentSource.textures map fragmentSource.textures map is set by Preprocessor::parseSampler2D, parseSampler2DChannel and parseSamplerCube.

claudeha commented 4 years ago

index is not the same as location!

On 22/12/2019 23:10, 3Dickulus wrote:

l = index in program which is not equal to the index in the source, some get optimized out or as constants or explicitly locatated, yes, but the location in program, 0,1,n... after linking is exactly what glGetActiveUniform() expects.

No!  Suppose there are 10 uniforms, of which 5 are active, with locations after linking and unused uniform removal 1 3 5 7 9 (uniform locations are implementation-defined unless set explicitly with layout(location=...) qualifiers; the uniform locations are not necessarily densely packed).  The glGetProgramiv(program, GL_ACTIVE_UNIFORMS, &count) will return 5, and you use dense indices 0 1 2 3 4 for glGetActiveUniform() corresponding in some implementation-defined way to the 5 active uniforms.  You can tell which uniform you got by the name returned from glGetActiveUniform().

follow it back and figure out why backbuffer is in the map fragmentSource.textures

It isn't.  But it is an active uniform, so will be returned by glGetActiveUniform for some index, that happens to have an index with the same bit representation as the location of a different uniform.

That the original code works on your machine is just (un)lucky.

I looked at the specification for OpenGL 4.6, it consistently uses int location for uniform locations and int index for active uniform indices. glGetActiveUniform() is defined in terms of glGetProgramResourceiv().  If I read the spec right it has a mode for getting the location of a uniform given its index:

GLuint index = ...;
GLenum property = GL_LOCATION;
GLint location;
glGetProgramResourceiv(program, GL_UNIFORM, index, 1, &property, 1, 
NULL, &location);

Which probably wouldn't exist if indices and locations were identical.

3Dickulus commented 4 years ago

the last name test before actually creating a texture is perhaps redundant the reply at this link was helpful too... https://stackoverflow.com/questions/16302276/difference-between-uniform-location-and-uniform-index

use glGetProgramResourceIndex() for glGetActiveUniform() query instead of the value returned by shaderProgram->uniformLocation ( textureUniformName );

GLuint idx = glGetProgramResourceIndex(shaderProgram->programId(), GL_UNIFORM, textureUniformName.toLatin1().data());
glGetActiveUniform(shaderProgram->programId(), idx, bufSize, &length, &size, &type, name);

this functions identical to using location, don't have to iterate over all to get the right index and on nVidia it seems that location and index are the same thing

checking the name returned by glGetActiveUniform against the name sent to glGetProgramResourceIndex is a bit of redundant error checking but couldn't hurt. if glGetActiveUniform returns a different name than we are looking for then something is wrong ;)

I hope I didn't cause you too much brain damage over this and I hope that this one-liner solves the issue for you. Thanks for pointing it out, I would have remained oblivious in nVidia land.

claudeha commented 4 years ago

http://docs.gl/gl4/glGetProgramResourceIndex says it requires OpenGL 4.3+ :( So I guess a fallback method (eg the loop over the active uniforms...) is still needed for OpenGL 3? Could be conditionally compiled/used I suppose.

claudeha commented 4 years ago

Maybe it would be simpler in the long run to propagate the 'type' from the Parser down to this part of the code that needs it, as afaict this is the only thing we need this query for?

3Dickulus commented 4 years ago

No! the type is returned by glGetActiveUniform(); no need to propagate info that's already there! glGetActiveUniform() just needs the right index, must be some GL2|3 compatible command to acquire this... will dig into that... glGetUniformIndices()

No. a fallback method for GL < 4.3 is not required, core since 3.1, unless you want less but then a bunch of other things won't work :-/

                // bugfix textures claude #104
                GLuint idx;
                // get a pointer to the char array in QString
                GLchar *oneName = textureUniformName.toLatin1().data();
                // returns index of "oneName" in idx;
                glGetUniformIndices( shaderProgram->programId(), 1, &oneName, &idx);
                // use idx to acquire more info about this uniform
                glGetActiveUniform(shaderProgram->programId(), idx, bufSize, &length, &size, &type, name);

functionally the same as glGetProgramResourceIndex

claudeha commented 4 years ago

On 25/12/2019 01:04, 3Dickulus wrote:

glGetUniformIndices()

Nice!

3Dickulus commented 4 years ago

just did some testing...

the other place glGetActiveUniform() is used is in the setShaderUniforms() routine but it's not working the same way as initFragmentTextures()

initFragmentTextures() looks up the index from a uniform name

setShaderUniforms() looks up a name from an index ie:glGetProgramiv(programID, GL_ACTIVE_UNIFORMS, &count);

in setShaderUniforms() I queried uniform location and uniform index in the way described and output the info next to i (the loop counter pos) and, on nVidia at least, they are in sync! pos == loc == idx. This is probably the case for anything that doesn't use modern GLSL to specify a location, something to pay attention to when moving forward ;)