attaching non-compiled shader objects

[regnirpsj]

the following sequence will attach non-compiled shader objects to a program:

Program prg;

prg << VertexShader().Source(/* ... */)
    << FragmentShader().Source(/* ... */);
prg.Link().Use();

however, looking at a run-time trace (w/ bugle) the current nvidia driver (340.24) is fine w/ this. but according to [https://www.opengl.org/wiki/GLSL_Object#The_Single_Shader_Variant] shader objects need to be compiled before attached to a program. the "correct" (?) sequence should be therefore:

Program prg;

prg << VertexShader().Source(/* ... */).Compile()
    << FragmentShader().Source(/* ... */).Compile();
prg.Link().Use();

i don't mind the current behavior. i just encountered this while trying to add support for GL_ARB_shading_language_include, which required "hi-jacking" ObjectOps<tag::DirectState, tag::Shader>::Compile() and explicitly using glCompileShaderIncludeARB.

shouldn't the program's shader-attachment and/or the link step ensure shader objects are compiled (and if necessary call 'Compile()' if they're not)?

[matus-chochlik]

Hi,

According to the GL specification (https://www.opengl.org/sdk/docs/man/html/glAttachShader.xhtml) shaders can be attached even if their source code was not supplied yet, or if they are not compiled yet, so IMHO the current behaviour is correct.

BTW I've just committed and pushed a wrapper for ARB_shading_language_include to develop. See the ShaderInclude class (in oglplus/named_string.hpp)and the Shader::CompileInclude(...) functions (in oglplus/shader.hpp). I'll modify some of the existing or add some new examples of usage later.

[regnirpsj]

hi matus,

agreed about what the spec says. it's just inconvenient when using shader includes and forgetting to call CompileIncludeWhatever because the link step just fails with no hint what went wrong.

thanks for including the ARB_shading_language_include extension so fast. works like a charm, except that i had to change the signature for Shader::CompileInclude from

GLsizei, const GLchar* const*, const GLint*

to

GLsizei, const GLchar**, const GLint*

because gcc 4.8.3. complains that the path in the arguments cannot be converted to const char**, which i don't really get because the prototype in GL/glext.h (from mesa-libGL-devel-10.1.5-1.20140607.fc20.x86_64) correctly specifies the path argument as const GLchar *const*. maybe, you have an idea what's going on here.

just for reference, this is how i'm using ARB_shading_language_include:

using namespace oglplus; 
namespace bfs = boost::filesystem;

bfs::path const   p(argv[0]);
std::string const d(bfs::canonical(p.parent_path()).string());
std::string const f(p.filename().string());
std::string const b(d + "/../share/shader/glsl");

std::array<std::string const, 6> const file_names = {
  {
    std::string(b + "/" + f + ".vs.glsl"),
    std::string(b + "/" + f + ".fs.glsl"),
    std::string(b + "/" + f + ".functions.glsl"),
    std::string(b + "/" + f + ".light.glsl"),
    std::string(b + "/" + f + ".material.glsl"),
    std::string(b + "/" + f + ".uniforms.glsl"),
  }
};

for (auto fn : file_names) {
  std::stringstream src;

  src << std::ifstream(fn).rdbuf();

  NamedString::Set(NamedStringType::ShaderInclude, fn, src.str());
}

prg_ << VertexShader()
        .Source(NamedString::Get(file_names[0]))
        .CompileInclude(b)
     << FragmentShader()
        .Source(NamedString::Get(file_names[1]))
        .CompileInclude(b);

prg_.Link().Use();

[matus-chochlik]

Hi,

I've added the Program::Build and Program::BuildInclude functions which search the list of shaders attached to a program, if any of them are not compiled then Compile or CompileInclude is explicitly called on them and the program is linked. This way you can get the diagnostic messages from Shader::Compile instead the one from Program::Link.

I've also updated the 022_xyz_planes.cpp example to show the usage.

HTH

[regnirpsj]

cool, both Program::Build[Include] work perfect.

that only leaves one thing i'd like to do/have: to supply a list of prefix-paths to Program::BuildInclude. i experimented with

/* 1 */ GLSLStrings prefix1({  path1        .c_str(),  path2        .c_str(), });
/* 2 */ GLSLStrings prefix2({ (path1 + '\0').c_str(), (path2 + '\0').c_str(), });

but that didn't go down so well. 2 was an attempt to actually provide null-terminated strings to the driver (but that didn't help). would it be desirable to add a Program::BuildInclude variant that takes a sequence of strings and does the right-thing(tm) to get the path/count arrays to the driver? or did i just overlook some obvious way to convert std::sequence<std:string> into something that's compatibe with Program::BuildInclude?

apologies, if i seem a bit greedy ;)

[matus-chochlik]

The GLSLString and GLSLStrings classes should not be instantiated directly, they are just (sort of) type-erasures for other types storing strings and are used only as function parameters, so, that the number of overloads of functions like Shader::Source does not explode. These two classes are efficient, but they do not store the underlying c-strings, whatever stores them (a std::string, std::vector, etc.) must not be destroyed during the whole lifetime of the GLSLString(s) instances (usually the duration of the OGLplus call to Shader::Source etc.).

The GLSLSource on the other hand stores the c-strings internally (if necessary) at the cost of some overhead. So If you want to store the paths in strings you should use the GLSLSource class. In the latest commit I've added constructors for std::vector<String> and std::initializer_list<String>.

For example the following should now work:

String path1(...);
String path2(...);
...
Program prog;
...
prog.BuildInclude({path1, path2, ...});

If you use the same set of includes many times the following is more efficient:

String path1(...);
String path2(...);
...
GLSLSource includes({path1, path2, ...});

Program prog1, prog2, prog3;
...
prog1.BuildInclude(includes);
prog2.BuildInclude(includes);
prog3.BuildInclude(includes);

I'm still not completely happy about how this whole thing is implemented and there will be probably some more changes but (hopefully) nothing interface-breaking.

[regnirpsj]

thanks for these changes, they work very well.