2

I've started learning how to use OpenGL and I absolutely hate having my shaders declared as global variables before my main().

I thought it would be cool to make a structure or class that would read the shader from a file in my project directory. The file reading works perfectly fine, but for some reason it won't actually output an image like it would if I had the shader declared before main. Here is my example.

Shader Reading Structure: 

#include "allHeader.h" struct shaderReader { shaderReader::shaderReader(std::string); const GLchar* source; }; shaderReader::shaderReader(std::string name) { std::string line, allLines; std::ifstream theFile(name); if (theFile.is_open()) { while (std::getline(theFile, line)) { allLines = allLines + line + "\n"; } source = allLines.c_str(); std::cout << source; theFile.close(); } else { std::cout << "Unable to open file."; } }

Snapshot of Area right before main()

shaderReader vertexShader = shaderReader("vertex.txt"); shaderReader fragmentShader = shaderReader("fragment.txt"); const GLchar* vertexSource = "#version 150 core\n" "in vec2 position;" "void main() {" " gl_Position = vec4(position, 0.0, 1.0);" "}"; const GLchar* fragmentSource = "#version 150 core\n" "out vec4 outColor;" "void main() {" " outColor = vec4(1.0, 1.0, 1.0, 1.0);" "}"; int main() { //stuff }

Works

 GLuint vertexShaderObject = glCreateShader(GL_VERTEX_SHADER); glShaderSource(vertexShaderObject, 1, &vertexSource, NULL); glCompileShader(vertexShaderObject); //Now let's create the Fragment Shader Object GLuint fragmentShaderObject = glCreateShader(GL_FRAGMENT_SHADER); glShaderSource(fragmentShaderObject, 1, &fragmentSource, NULL); glCompileShader(fragmentShaderObject);

Doesn't work for some Reason

 const GLchar* ob1 = vertexShader.source; const GLchar* ob2 = fragmentShader.source; GLuint vertexShaderObject = glCreateShader(GL_VERTEX_SHADER); glShaderSource(vertexShaderObject, 1, &ob1, NULL); glCompileShader(vertexShaderObject); //Now let's create the Fragment Shader Object GLuint fragmentShaderObject = glCreateShader(GL_FRAGMENT_SHADER); glShaderSource(fragmentShaderObject, 1, &ob2, NULL); glCompileShader(fragmentShaderObject);

Also Doesn't Work

 GLuint vertexShaderObject = glCreateShader(GL_VERTEX_SHADER); glShaderSource(vertexShaderObject, 1, &vertexShader.source, NULL); glCompileShader(vertexShaderObject); //Now let's create the Fragment Shader Object GLuint fragmentShaderObject = glCreateShader(GL_FRAGMENT_SHADER); glShaderSource(fragmentShaderObject, 1, &fragmentShader.source, NULL); glCompileShader(fragmentShaderObject);

The above code when working prints out a white triangle in the middle of a black screen. However, when not working I just get a black screen. I tried checking the shaders for compile errors and I got no errors at all. I'm think perhaps something is wrong within the structure, something there I didn't do right. Thank you for reading my question, if you had a similar problem I hope my question helps you.

Improve this question
7
  • 2
    You let the allLines string go out of scope after you finished reading the code. So the memory containing the shader code will be freed before you use it. Commented Jun 12, 2015 at 3:56
  • Put the shaders into .txt files. Wrap each line in "" at the start and end. End it with a ;. Then const GLChar* some_shader =, newline, #include "shader.txt". Hey look! Compile time string, no file loading at runtime, and your shaders are out of the way? Commented Jun 12, 2015 at 3:59
  • Hey, @RetoKoradi how exactly did allLines go out of scope? @Yakk could you demonstrate an example as an answer if possible, thank you. Commented Jun 12, 2015 at 4:10
  • 1
    @BryantheLion You declare allLines in the constructor. So when the constructor ends allLines is destroyed. Commented Jun 12, 2015 at 4:14
  • 1
    You don't copy the content of allLines into source. You get a pointer to the internal data of allLines, and assign that pointer to source. Then when allLines goes out of scope at the end of the constructor, it frees its memory, and source is now pointing to freed memory. Commented Jun 12, 2015 at 4:28

3 Answers 3

Reset to default
3

You're using a mix of C++ (std::string) and C (char*) strings in an invalid way.

In the constructor, you're building up the code in a C++ string, which is an object that automatically allocates and re-allocates the memory to hold the string data as the string grows. It will also automatically free that data when it goes out of scope at the end of the constructor.

The root of the problem is here:

source = allLines.c_str();

where source is a class/struct member. The c_str() method on the string gives you a pointer to the internal string data of the object. As explained above, this data is freed when allLines goes out of scope at the end of the shaderReader constructor, so you end up with a pointer to freed memory. This results in undefined behavior, because this memory could now be used for something else.

The cleanest way to fix this is to use C++ strings consistently. Change the struct definition to:

struct shaderReader { shaderReader::shaderReader(std::string); std::string source; };

Then in the constructor, you can read the source code directly into this class member:

while (std::getline(theFile, line)) { source = source + line + "\n"; }

The only reason why you have to bother with a char* is because glShaderSource() needs one. The safest approach is to convert this at the last moment:

const GLchar* ob1 = vertexShader.source.c_str(); GLuint vertexShaderObject = glCreateShader(GL_VERTEX_SHADER); glShaderSource(vertexShaderObject, 1, &ob1, NULL); glCompileShader(vertexShaderObject);

This way, you only use the pointer to the internal string data very temporarily, and you don't have to bother with C-style strings and memory management otherwise.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

Thank you, this is actually very smart and I'm not sure why I didn't think about doing something like this before. This is probably the best method presented here.
I tried doing something a bit different which should work but I get a Debug Assertion Failed. All I did was created a new variable const GLchar* Source in my struct definition. Then after the while loop I simply made Source = source.c_str() and in my function glShaderSource(vertexShaderObject, 1, &vertexShader.Source, NULL); It works but when I close the application it gives me that error.
Nevermind, the reason was because I had kept the old destructor definition. if(source) { delete[] source; source = nullptr; }
@reto-koradi I tried converting your idea into a function but it crashes the opengl context when trying this ( consider fragment being a "string" that read a shader from a txt). I suspect I'm simply doing something stupid with the pointers. I can guarantee the string contains an usable shader. -------------------------------------------------------- glShaderSource(vertexShaderObject, 1, getFragmentShaderSrc(), NULL); ->using this function: ----------------------> const GLchar * const * getFragmentShaderSrc(){ const GLchar* d = fragment.c_str(); return &d; }
1

change

source = allLines.c_str();

into

source = new char[allLines.length()+1]; strcpy(source,allLines.c_str());

And in the destructor of shaderReader, release the memory allocated for source

if(source) { delete[] source; source = nullptr; }

Ref: https://stackoverflow.com/a/12862777/3427520

Improve this answer

5 Comments

This is good, I simply had to tweak it a bit in order to convert the char* to const GLchar*, thank you very much for your answer.
Any Idea why defining my destructor is causing a Debug Assertion Failed?
Why are you suggesting to store the code in a C string if the OP is using C++, and is already using C++ strings in the rest of the code?
Er, I'm sorry I missed that... Yes, your answer is better.
@BryantheLion Maybe you delete the memory of the allLines.c_str() accidentally, which will cause an Assertion failure.
1

I would be tempted to store your shader in a std::vector<GLchar> so you don't need to worry about allocating and deallocating memory:

Maybe something like this:

struct shaderReader { shaderReader(std::string); std::vector<GLchar> source; const GLchar* data; GLint size; }; shaderReader::shaderReader(std::string name) { std::string line, allLines; std::ifstream theFile(name); if(theFile.is_open()) { while(std::getline(theFile, line)) { allLines = allLines + line + "\n"; } source.assign(allLines.begin(), allLines.end()); size = source.size(); data = source.data(); theFile.close(); } else { std::cout << "Unable to open file."; } } int main() { shaderReader sr("fragment-shader.txt"); GLuint shader = glCreateShader(GL_FRAGMENT_SHADER); glShaderSource(shader, 1, &sr.data, &sr.size); }
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.