PDA

View Full Version : ShaderProgram won't link and does't show any error log.



geomazzix
12-01-2017, 03:32 AM
When I was working on my openGL 2D platformer game, I noticed there was no color on my rectangle. So I thought this might be because of an error check I missed. Indeed I forgot to call the errorcheck function *facepalm* but this resulted in an error saying that my std::vector got out of index when creating it. I couldn't even start the program, it would respond with a big error message in my screen. Someone already pointed out that I could change the vector's range to a static range, but when I did this it did not log any error, while still showing the line: "Program failed to linkL:".

The function is called CheckShaderError, the place where the error occurs is:


///--------------------------------------------------------------------------------------------------------------------------------------------------///
#include "ShaderProgram.h"
#include <stdio.h>
#include "ErrorHandeler.h"
#include <fstream>
#include <vector>

ShaderProgram::ShaderProgram() : _ShaderProgramID(0){}

//Creates, compiles and links the shaders.
void ShaderProgram::Init()
{
//Create the shader program.
_ShaderProgramID = glCreateProgram();

//Create and compile shaders.
InstantiateShader(_Shaders[0], GL_VERTEX_SHADER, "Mesh/Mesh.vert");
InstantiateShader(_Shaders[1], GL_FRAGMENT_SHADER, "Mesh/Mesh.frag");

//Link the shaders after they are created and compiled.
LinkShaders();
}


//Creates a shader.
void ShaderProgram::InstantiateShader(GLuint shaderID, const GLenum
shaderType, const std::string dataPath)
{
//Create the shader.
shaderID = glCreateShader(shaderType);

//Set the shadersource.
std::string shaderSource = ReadShaderFromFile("../2D Platformer/Shaders/" + dataPath).c_str();
const GLchar* shaderSourcePtr = shaderSource.c_str();
glShaderSource(shaderID, 1, &shaderSourcePtr, 0);

//Compile the shader.
glCompileShader(shaderID);

//Check if the shader did compile.
CheckShaderError(shaderID, false);
}


//Links the program and the shaders.
void ShaderProgram::LinkShaders()
{
//Attach the shaders. the shaders.
int shaderCount = sizeof(_Shaders) / sizeof(GLuint);
for (int i = 0; i < shaderCount; i++)
{
glAttachShader(_ShaderProgramID, _Shaders[i]);
}

//Link the program.
glLinkProgram(_ShaderProgramID);


CheckShaderError(_ShaderProgramID, true);
}


//Check for any compile or linking errors.
void ShaderProgram::CheckShaderError(const GLuint shaderID, const bool
isProgram)
{
GLint compileCheck = 0;
if (isProgram) {
glGetProgramiv(_ShaderProgramID, GL_LINK_STATUS,(int *)&compileCheck);
}
else {
glGetShaderiv(shaderID, GL_COMPILE_STATUS, &compileCheck);
}

if (compileCheck == GL_FALSE)
{
//The application does not need any garbage collection here because the
deconstructor takes care of that.
GLint maxLength = 0;

if (isProgram) {
glGetProgramiv(_ShaderProgramID, GL_INFO_LOG_LENGTH, &maxLength);

std::vector<GLchar> errorlog(maxLength);
glGetProgramInfoLog(_ShaderProgramID, maxLength, &maxLength,
&errorlog[0]);

printf("Program failed to linkL: %s\n", &errorlog[0]);
FatalError();
}
else {
glGetShaderiv(shaderID, GL_INFO_LOG_LENGTH, &maxLength);

std::vector<GLchar> errorlog(maxLength);
glGetShaderInfoLog(shaderID, maxLength, &maxLength, &errorlog[0]);

printf("Shader failed to compile: %s\n", &errorlog[0]);
FatalError();
}
}
}


//Read shadercontent out of file.
std::string ShaderProgram::ReadShaderFromFile(std::string dataPath)
{
std::string shaderContent;
std::string currLine;
std::ifstream shaderContentFileStream(dataPath);
if (shaderContentFileStream.is_open())
{
while (std::getline(shaderContentFileStream, currLine))
{
shaderContent.append(currLine + "\n");
}
shaderContentFileStream.close();
return shaderContent;
}
else
{
printf("Failed to read the shader");
FatalError();
}

return NULL;
}


//Enables the current shaderprogram.
void ShaderProgram::EnableShaderProgram()
{
glUseProgram(_ShaderProgramID);
}



//Garbage collector.
ShaderProgram::~ShaderProgram()
{
//Detach and delete all shaders.
int shaderCount = sizeof(_Shaders) / sizeof(GLuint);
for (int i = 0; i < shaderCount; i++)
{
glDetachShader(_ShaderProgramID, _Shaders[i]);
glDeleteShader(_Shaders[i]);
}

//Delete the shader program.
glDeleteProgram(_ShaderProgramID);
}
///--------------------------------------------------------------------------------------------------------------------------------------------------///


If someone knows how to fix this please tell me.

DragonautX
12-02-2017, 01:14 AM
If you're using OpenGL 3.3 core, I can try to take a look at it. Can you post your entire source? Maybe a dropbox link or something. You can send me a private message if you don't want to post a public link. From just a random guess, maybe it has something to do with your shader source.

I use Visual Studio 2015. If you're not using a 2015 solution, I may still be able to add your code files and libraries and attempt to run the program.

If you're not using 3.3 core, I can't really say much. Even though other versions are similar, I haven't tried them yet, so I can't say anything confidently there. For general checks, I'd probably check on this:
-Is your shader source correct?
-Have you loaded your shader source correctly, with newlines separating source lines?
-Are all shaders compiled and checked for errors? (looks like you already did)
-Are all shaders you want attached to the program object you want? (looks like you already did)
-Did you link your program object and check for errors? (looks like you already did)
-Does glGetError give you anything?

---

About the snippet overall though, what I can say is that you may not want to write your destructor like that. Because if you pass a copy of your ShaderProgram object to a small function and that function exits, the destructor will be called on that copied object, potentially deleting your program object and shaders when you probably still want to use the overall ShaderProgram object. If you intended to use dynamic allocation with new/delete to pass pointers to functions, I'd say that destructor is fine. But it may be better to just have a public deleting function for stuff like that, which you can call at the end of your OpenGL program. I think it's more explicit, and you don't have to worry about possible memory leak problems with new/delete. That's just my opinion though.

geomazzix
12-02-2017, 02:44 PM
Thank you for replying to my problem.

- My shadersource is correct I checked it by printing the whole shaderstring I read from my files.
- Yes it did show up correctly.
- Both shaders do compile and attach and they are both checked for errors (none of them show any though).
- The linkProgram() also works, but when I check if the program linked correctly it goes wrong.

You can download the whole project here:
https://github.com/Geomazzix/2D-OpenGL-platformer
(I hope you are familiar git/github).

One final note: I do work with openGL 4.5, this is at least my focus. I hope you can find my problem :S

regards,
Geomazzix

DragonautX
12-03-2017, 12:54 AM
I took a look at your code. The main problem seems to be that in the ShaderProgram::InstantiateShader member function defined, your intention seems to be that for the "GLuint shaderID" parameter, you want to create a shader object, and assign it to this parameter in a way that it updates some private member variable called "GLuint _Shaders[2]".

However, your code doesn't do that. It does assign the parameter variable to the integer name returned by glCreateShader, but it has no interaction with your _Shaders array. It looks like you thought a line like "InstantiateShader(_Shaders[0], GL_VERTEX_SHADER, "Mesh/Mesh.vert")" would do an assignment like this, but it doesn't. In this case, it only passes the first element of your _Shaders array into the function, and that's it. What's passed is copied into the function and has no relevance to the array anymore. So, I modified your code with the intention of assigning the shader object names to the _Shaders array.

Overall, it's a C++ problem, not an OpenGL one. If you just want my solution, here's a dropbox link (https://www.dropbox.com/sh/0m49iho2r82o81o/AAAWDsptbyhGMftI820wCT2Qa?dl=0) to it. I'm seeing a window with a pink background, along with a console window. Hopefully that's right.

You seemed to have used Visual Studio 2017. I only have 2015. If you use my VS solution, I think you should get a prompt message to upgrade the solution. Other than that, it should work the same. Also, again, I'm only familiar with OpenGL 3.3 core, and I edited the code to test it based on that. My card can only support up to 4.0. You say you can use OpenGL 4.5, so 3.3 core should run too.

If you want to see 4.5 run, modify the DisplayManger::Init changes I made to request an OpenGL 4.5 core profile (I assume you want core, you didn't mention a profile in your shaders, and according to the GLSL 4.5 spec, that would mean those shaders would be for 4.5 core). I made a note about changes to the Init function below. And change your shaders back to your own source. Again, I think this'll work, but I don't have 4.5, so you have to see for yourself.

That's the summary of it. Below are be my complete notes on debugging your code, so you know what I changed.

===
First note, I've never used SDL seriously. The only time I've ever actually used it was to try to find out why some person was getting a link error in Visual Studio, and the person said he/she used SDL. I thought I could help with the link error, so I looked into SDL for that. Here's the link (https://www.opengl.org/discussion_boards/showthread.php/200005-LNK1561-entry-point-must-be-defined) to that post, if you're interested. Anyways, I made my own notes the best I could about SDL in this code.

===
File-wise, looking at your header files, it looks like you planned to use the dll versions of GLEW and SDL. In that case, for GLEW, in your Dependencies/libs folder, I'm guessing glew32s.lib is your GLEW static library. Since you didn't use it, I deleted it. From my knowledge of SDL, I've never seen an "SDL2test.lib" required. Not sure what that's for or where that came from, but the name suggests it was just for some certain test, so I deleted that too.

Also, neither of these libraries were mentioned in your VS Project Properties. So I thought that was a reason too to safely delete them. I did this when I was debugging so I could minimize the files to think about.

The main problem though is that you forgot to include the dll files in your Github repo. I had a spare glew32.dll and an SDL dll from that forum post I mentioned. Don't know if they're the right versions. They seemed to work, so it was okay in the end. Nonetheless, make sure to include your dlls in your own repo. I put the DLLs in the 2D Platformer folder, along with your source files.

===
Looking at your project properties, you only used the Debug x86 mode in Visual Studio. I also kept it to Debug x86.

===
So the main problem I mentioned in the beginning of this reply was that it looks like you wanted to do array asisgnment in your ShaderProgram::InstantiateShader member function, but you did it incorrectly.

In your code, you had this snippet in your ShaderProgram::Init function:



//Create and compile shaders.
InstantiateShader(_Shaders[0], GL_VERTEX_SHADER, "Mesh/Mesh.vert");
InstantiateShader(_Shaders[1], GL_FRAGMENT_SHADER, "Mesh/Mesh.frag");


And the first lines of code definition for your ShaderProgram::InstantiateShader function were:



void ShaderProgram::InstantiateShader(GLuint shaderID, const GLenum shaderType, const std::string dataPath)
{
//Create the shader.
shaderID = glCreateShader(shaderType);
...


You also had a ShaderProgram::LinkShaders functions that did this:



void ShaderProgram::LinkShaders()
{
//Attach the shaders. the shaders.
int shaderCount = sizeof(_Shaders) / sizeof(GLuint);
for (int i = 0; i < shaderCount; i++)
{
glAttachShader(_ShaderProgramID, _Shaders[i]);
}

//Link the program.
glLinkProgram(_ShaderProgramID);
}


So my guess at your intentions was that you wanted InstantiateShader to create shader objects whose integer names would be put into an array called _Shaders. And you later wanted to use this array in LinkShaders to attach the shader objects to some program object, where you would then link the program object. However, a line like "InstantiateShader(_Shaders[0], GL_VERTEX_SHADER, "Mesh/Mesh.vert")" only passes an integer to the "shaderID" parameter in your InstantiateShader function. There is no sense of assigning to the _Shaders array in doing this, and there is no mention of assigning to that array either in your actual InstantiateShader function. So I modified the and definiton and function call like so in your ShaderProgram.cpp:

(changed header definition in ShaderProgram.h as well)


void ShaderProgram::InstantiateShader(int index_to_store_shader_in, const GLenum shaderType, const std::string dataPath)
{
//Create the shader.
//shaderID = glCreateShader(shaderType);
GLuint shaderID = glCreateShader(shaderType);
_Shaders[index_to_store_shader_in] = shaderID;




//Creates, compiles and links the shaders.
void ShaderProgram::Init()
{
//Create the shader program.
_ShaderProgramID = glCreateProgram();

//Create and compile shaders.
InstantiateShader(0, GL_VERTEX_SHADER, "Mesh/Mesh.vert");
InstantiateShader(1, GL_FRAGMENT_SHADER, "Mesh/Mesh.frag");
...
}


The InstantiateShader code now expects the first parameter to be the index you want to store a shader object name in. I guess that's what you want.

Anyways, that's the fix. An std::vector can work well here, since you can change the size of it to use more shader object names. An array is just fine. You just need to be careful with indexing. Maybe a member variable mentioning the size of this array would be a good idea here, so you can explicitly reference it. You can make the array super large as well, so you don't have to worry about running out of indices.

===
In your post, you said you got the logging error "Program failed to linkL:". I took a look at your code. Your ShaderProgram::LinkShaders call in ShaderProgram::Init is where you link all your shaders. I found some ShaderProgram::CheckShaderError that seems to be used also for checking the program linking status. I thought there'd be that call after "LinkShaders()", but I didn't see anything there.

I don't know how you got that error message about program linking failure if you didn't actually call CheckShaderError to check it. Maybe I missed something. Nonetheless, I just added the call with the arguments for a program check instead of a shader check.



void ShaderProgram::Init()
{
...
LinkShaders();
CheckShaderError(_ShaderProgramID, true);
}


===
With the main problem out of the way, a first side detail I wanted to mention was that since I can't use 4.5 (I never tried it, and my card can't support it), I looked up the SDL docs to explicitly request a 3.3 core context. I was able to do this in your DisplayManager::Init function, with the SDL_GL_SetAttribute function provided by SDL.

I also noticed you had a "SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1)" call at the end of your Init funciton, after SDL_CreateWindow. I moved it to before the SDL_CreateWindow call, next to my GL version request call. The docs for SDL_GL_SetAttribute say that "The requested attributes should be set before creating an OpenGL window". It also makes sense to actually request certain OpenGL window and rendering context attributes before actual creating the window and context.

So anyways, I changed DisplayManger::Init to this:



void DisplayManager::Init()
{
//Create the window.
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, 3);
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 3);
SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK, SDL_GL_CONTEXT_PROFILE_CORE);
//Make sure to enable the doublebuffer.
SDL_GL_SetAttribute(SDL_GL_DOUBLEBUFFER, 1);
...
}


===
With using OpenGL 3.3 core, I changed your vertex and fragment shaders to use 3.3 core:

Mesh.vert


#version 330 core

void main()
{

}


Mesh.frag


#version 330 core

void main()
{

}


===
During debugging, I got some shader compilation errors, probably from attempting to use 4.5. I changed the last else statement in ShaderProgram::CheckShaderError to use a GLchar array instead of a vector because my Debugger was only showing the first character in your GLchar vector. I wanted to see a more complete message:



void ShaderProgram::CheckShaderError(const GLuint shaderID, const bool isProgram)
{

...
else {
glGetShaderiv(shaderID, GL_INFO_LOG_LENGTH, &maxLength);

//std::vector<GLchar> errorlog(maxLength);
//glGetShaderInfoLog(shaderID, maxLength, &maxLength, &errorlog[0]);
GLchar e[1024];
glGetShaderInfoLog(shaderID, maxLength, &maxLength, e);
FatalError();
...
}

===
In your ShaderProgram::ReadShaderFromFile function, the build output from Visual Studio had a warning about not all control paths returning a value. I modified your last else statement to return an empty std::string, to get rid of the warning. Change it as you want:




std::string ShaderProgram::ReadShaderFromFile(std::string dataPath)
{
...
else
{
printf("Failed to read the shader");
FatalError();
std::string dummy_return = "";
return dummy_return;

}
...
}


===
Lastly, in your ShaderProgram::InstantiateShader function, your line about assigning to your shaderSource string variable was showing garbage in my debugger. The line looks okay though, may have worked for you. Anyways, I made an explicit std::string variable to load the string from ReadShaderFromFile before assigning its C string to shaderSource:



void ShaderProgram::InstantiateShader(int index_to_store_shader_in, const GLenum shaderType, const std::string dataPath)
{
...
//Set the shadersource.
//const GLchar* shaderSource = (const GLchar*)ReadShaderFromFile("../2D Platformer/Shaders/" + dataPath).c_str();
std::string a = ReadShaderFromFile("../2D Platformer/Shaders/" + dataPath).c_str();
const GLchar* shaderSource = a.c_str();
...
}

GClements
12-03-2017, 08:50 AM
So my guess at your intentions was that you wanted InstantiateShader to create shader objects whose integer names would be put into an array called _Shaders. And you later wanted to use this array in LinkShaders to attach the shader objects to some program object, where you would then link the program object. However, a line like "InstantiateShader(_Shaders[0], GL_VERTEX_SHADER, "Mesh/Mesh.vert")" only passes an integer to the "shaderID" parameter in your InstantiateShader function. There is no sense of assigning to the _Shaders array in doing this, and there is no mention of assigning to that array either in your actual InstantiateShader function. So I modified the and definiton and function call like so in your ShaderProgram.cpp:

(changed header definition in ShaderProgram.h as well)


void ShaderProgram::InstantiateShader(int index_to_store_shader_in, const GLenum shaderType, const std::string dataPath)
{
//Create the shader.
//shaderID = glCreateShader(shaderType);
GLuint shaderID = glCreateShader(shaderType);
_Shaders[index_to_store_shader_in] = shaderID;



A simpler fix would be to just make the first parameter to InstantiateShader() a reference, i.e.


void ShaderProgram::InstantiateShader(GLuint& shaderID, const GLenum shaderType, const std::string dataPath)

That way, there's no need to change the body of InstantiateShader() or its callers.

DragonautX
12-03-2017, 09:41 AM
Oh, that works too, thanks. Never thought of actually doing something like that in my own code. I think I've only used C++ references for passing single objects or entire arrays, but never a reference to an element in an array. I guess geomazzix actually wanted to do that, but just forgot the "&" sign.

john_connor
12-03-2017, 01:02 PM
another way to get around these problems is to strictly avoid allocating objects/handles within any (helper) functions, most of the time shader objects arent needed after the corresponding program has been linked, so one can just delete the shaders after the program is ready. see the shader.h/.cpp

https://sites.google.com/site/john87connor/other/error-check

shader/program creation/deletion has to happen outside the c-style "helper functions"

geomazzix
12-03-2017, 03:25 PM
Never expected it to be such a rookie mistake, after working almost 1 year with C++, you would expect to get over this stuff.

geomazzix
12-03-2017, 03:26 PM
Can someone also explain me how to close the thread now the problem has been fixed?

GClements
12-03-2017, 10:05 PM
Can someone also explain me how to close the thread now the problem has been fixed?
The forum doesn't support "closing" threads.

There isn't really much need when there's rarely more than a dozen "active" threads.