Sign in to follow this  

Trouble with GLSL shaders

Recommended Posts

Posted (edited)

Recently I was trying to add shaders to my game.

I have gotten it to work but I feel like this code is too "shonky" and too C-style.

Any suggestions for ways to clean this code up and remove the dangerous C-style casts would be welcome.

std::string shader_file;
std::ifstream is(shader_src);
is >> shader_file;
is.close();

GLchar *shader_file_c = new GLchar[shader_file.length() + 1];
std::strcpy(shader_file_c, shader_file.c_str());
delete[] shader_file_c;

GLint len = shader_file.length();
glShaderSourceARB(shader_frag, 1, (const GLchar **) &shader_file_c, &len);

glCompileShaderARB(shader_frag);
glAttachObjectARB(shader_prog, shader_frag);
glLinkProgramARB(shader_prog);
Edited by Balajanovski

Share this post


Link to post
Share on other sites

That code shouldn't even work: you load the data into shader_file, then copy it into shader_file_c, then deallocate shader_file_c, then tell GL to read from shader_file_c even though it's been deallocated.

 

You should be able to do something like:

const GLchar* shader_file_c = shader_file.c_str();

glShaderSourceARB(shader_frag, 1, &shader_file_c, &len);

Share this post


Link to post
Share on other sites

It could work, sometimes, but it is random at best. You have an ugly bug here, which could work most of the time and sometimes will result in a crash or compiler error etc. which could be hard to track down.

Share this post


Link to post
Share on other sites
Posted (edited)

Well, I've fixed it now, here is the new code:

The reason its longer is because I've now included the whole function

GLuint shader_frag = 0;
GLuint shader_prog = 0;

shader_prog = glCreateProgram();
shader_frag = glCreateShader(GL_FRAGMENT_SHADER);

std::string shader_file;
std::ifstream is(shader_src);
std::string temp_str;
while (is >> temp_str)
    shader_file += temp_str + ' ';
is.close();
std::cout << shader_file << std::endl;

GLchar *shader_file_c = new GLchar[shader_file.length() + 1];
std::strcpy(shader_file_c, shader_file.c_str());

GLint len = shader_file.length();
glShaderSource(shader_frag, 1, (const GLchar **) &shader_file_c, &len);
delete[] shader_file_c;

glCompileShader(shader_frag);


glAttachShader(shader_prog, shader_frag);
glLinkProgram(shader_prog);

return shader_prog;
Edited by Balajanovski

Share this post


Link to post
Share on other sites

I'd revisit Hodgman's suggestion.

It removes the need for new and std:strcpy calls when you set your GLchar array.

Then shows that your double indirection cast is unnecessary when you set your temporary glShaderSource which btw can be glDeleteShader()'d after program link.

 

I think you felt you needed that cast because you didn't declare your GLchar* shader_file_c as constant to begin with.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this