Initial Implementation of Shader Programs #73

Quaker762 · 2017-01-27T13:43:09Z

Can load, compile and link shaders from files. Currently untested (as we
don't have a GLContext working as of yet!)

@z33ky I'm going to add more to this over the next few days. We should probably start doing some GL stuff to test drawing/some other systems (like textures). Dis you want to try out fast-forwaring instead of merging this time?

Quaker762 · 2017-01-27T13:48:32Z

Also see #74

Quaker762 · 2017-01-29T12:26:20Z

Okay I've fixed a weird segfault. This should be good to go. glShaderSource is a bizarre function...

EDIT: It isn't, I just realised my file IO on about line 93 of glprogram.cpp is REALLY bad. I'll fix it ASAP.

z33ky

Perhaps the glDelete* stuff should just be in the destructor of the class (even in the error case). This way resources will always be released when the sh3_glprogram is destroyed, be it because it couldn't be loaded or when it simply isn't needed anymore.

z33ky · 2017-01-29T20:15:14Z

source/SH3/system/glprogram.cpp

+    std::string         fname;      // Name of the file we want to open
+    std::vector<GLchar> errorLog;   // Error log
+
+    if(type == GL_VERTEX_SHADER)


There's plenty of code duplication in these branches.
For each shader type, only fname seems to be the difference.

Yeah I'm getting on fixing this ASAP.

z33ky · 2017-01-29T20:18:56Z

source/SH3/system/glprogram.cpp

+        die(&errorLog[0]);
+    }
+
+    source.close();


No need for that, RAII will do that.

z33ky · 2017-01-29T20:20:54Z

source/SH3/system/glprogram.cpp

+
+        errorLog.resize(logLength);
+        glGetProgramInfoLog(programID, logLength, &logLength, &errorLog[0]);
+        glDeleteProgram(vertShader);


Should be glDeleteShader I think.

Whoops haha

z33ky · 2017-01-29T20:32:03Z

source/SH3/system/glprogram.cpp

@@ -51,21 +53,24 @@ GLint sh3_glprogram::Load(const std::string& name)

 void sh3_glprogram::Bind()
 {
-
+    if(programID != 0)


if(programID == 0) { //warn return; }

?

z33ky · 2017-01-29T20:35:48Z

source/SH3/system/glprogram.cpp

 }

 void sh3_glprogram::Unbind()
 {
-
+    glUseProgram(0); // The 'correct' way to unbind a shader, but apparently undefined?!?!


I don't have a huge issue with this, but as a fix we could use a dummy shader (and have a static class variable refer to it) that is set on Unbind().
The dummy shader would either just render a solid color or an error message if we want to be fancy.

Yeah that sounds like a good idea. Maybe even a checkered square like the Source engine haha.

z33ky · 2017-01-29T20:50:28Z

source/SH3/system/glprogram.cpp

@@ -75,11 +80,17 @@ GLuint sh3_glprogram::Compile(std::uint32_t type)

        source.open(fname);

+        if(!source.is_open())
+            die("sh3_glprogram::Compile( ): Unable to open a handle to %s!", fname.c_str());
+
        while(!source.eof())


We could require the shader sources to have UNIX-style line endings ('\n'), in which case we can open the file in binary mode and use a std::ostringstream and read the whole file via stringstream << source.rdbuf();.
Reading the file like this is safer though.

Ahh I see. I'm using Windows, so I think the problem is stemming from the stupid \r\n at the end of each line.

Why is this method safer?

Safer as in, doesn't lead to errors when using different line endings.
You should be able to switch to different line endings in your editor.

Quaker762 · 2017-01-30T08:35:33Z

Regarding glDelete*() calls, the OpenGL spec seems to recommend that glDeleteShader be called if compile or link fails. I've added glDeleteShader() and glDeleteProgram() calls to the destructor nonetheless.

z33ky · 2017-01-30T20:19:58Z

source/SH3/system/glprogram.cpp

@@ -39,7 +41,7 @@ GLint sh3_glprogram::Load(const std::string& name)

        errorLog.resize(logLength);
        glGetProgramInfoLog(programID, logLength, &logLength, &errorLog[0]);
-        glDeleteProgram(vertShader);
+        glDeleteShader(vertShader);
        glDeleteShader(fragShader);

        die(&errorLog[0]);


When the string contains something looking like a format specifier, things will go wrong. Use die("%s", ...) instead.

Completely missed that, it's fixed now.

z33ky · 2017-01-30T20:23:02Z

The glDestroy* functions will still be called when the destructor is run. Would this be insufficient?
Also, now it will be called twice when a shader fails to compile, once in Load() and once in sh3_glprogram.

Quaker762 · 2017-01-31T08:59:39Z

The glDestroy* functions will still be called when the destructor is run. Would this be insufficient?

If anything, they should be destroyed right after the link is performed, as they are no longer needed and are just a waste of memory (as I have read, but it seems everyone has their own opinion on this stuff haha), though with the amount most people have in their system, this is probably a non-issue (though if we preload stuff it may be).

Also, now it will be called twice when a shader fails to compile, once in Load() and once in sh3_glprogram

Will the constructor still be run if a call to die() is made??? I thought that was only the case if they're static/global objects.
Also, now that I think about it, should we actually die if it fails to compile, or just replace the shader with a default one as you've suggested somewhere in the comment chain?

z33ky · 2017-01-31T09:58:36Z

I though that die() was just a sort of "error-handling to be implemented later" in most cases.
Like stuff not being found in an arc doesn't seem to be a truly fatal error in many cases. At worst it should just abort loading the current scene, not the whole program.

Quaker762 · 2017-01-31T10:07:51Z

I though that die() was just a sort of "error-handling to be implemented later" in most cases.

True, we should probably just keep them to aid with debugging shaders at this point.

Like stuff not being found in an arc doesn't seem to be a truly fatal error in many cases

Wouldn't that imply the file is corrupt or damaged in some way though?

Also, do you want me to move the glDeleteShader() calls to straight after the program has been linked, and leave just glDeleteProgram() in the destructor?

z33ky · 2017-02-02T10:46:20Z

Wouldn't that imply the file is corrupt or damaged in some way though?

That or driver issues probably.
I think we should make a best effort. If something goes wrong we should definitely log, but just fallback to the dummy shader. It probably won't make for a good playing experience, but if multiple shader fail for instance, then you get all errors in one run, instead of a cycle of run, die, fix shader until all are fixed.

z33ky · 2017-02-02T10:54:27Z

Also, do you want me to move the glDeleteShader() calls to straight after the program has been linked, and leave just glDeleteProgram() in the destructor?

Yeah, that seems good. Although now glDeleteShader() is missing for the successful case.
The spec is okay with deleting the shader while still attached; It will then be deleted when the shader is detached. So we should just always do that after the shader was attached.
The program will also automatically detach the shaders, so we don't need to do that manually.

Quaker762 · 2017-02-02T12:21:01Z

I think we should make a best effort. If something goes wrong we should definitely log, but just fallback to the dummy shader. It probably won't make for a good playing experience, but if multiple shader fail for instance, then you get all errors in one run, instead of a cycle of run, die, fix shader until all are fixed.

Good point, I'll get rid of all of the calls to die() and replace them with Log() with the ERROR status.

Yeah, that seems good. Although now glDeleteShader() is missing for the successful case.

Bloody hell, more of my 1AM programming... I think I should make myself a coffee before I attempt to program (that or crack open a beer). I've fixed it now.

The spec is okay with deleting the shader while still attached

I had no clue this was the case. Everything I read 'recommended' a detach before delete. Very interesting. I've removed all calls to detach.

z33ky · 2017-02-02T13:07:19Z

source/SH3/system/glprogram.cpp

@@ -93,7 +95,7 @@ GLuint sh3_glprogram::Compile(GLenum type)
    source.open(fname);

    if(!source.is_open())
-        die("sh3_glprogram::Compile( ): Unable to open a handle to %s!", fname.c_str());
+        Log(LogLevel::ERROR, "sh3_glprogram::Compile( ): Unable to open a handle to %s!", fname.c_str());


I think we can just return at this point.
//TODO: default shader fallback.

z33ky · 2017-02-02T13:09:58Z

You can also squash the commits. I don't see a good reason to keep them separate.

z33ky · 2017-02-02T13:30:45Z

source/SH3/system/glprogram.cpp

+    {
+        id = glCreateShader(GL_FRAGMENT_SHADER); // Generate a shader ID
+        fname = "data/shaders/" + programName + ".frag";
+    }


else die()

z33ky · 2017-02-02T13:31:44Z

source/SH3/system/glprogram.cpp

+#include <fstream>
+#include <vector>
+
+#define BAD_SHADER 0


This should be a static const GLuint IMO.

Can load, compile and link shaders from files. Currently untested (as we don't have a GLContext working as of yet!)

Quaker762 requested a review from z33ky January 27, 2017 13:43

Quaker762 force-pushed the shaderloader branch 2 times, most recently from 5a376ca to 4518d34 Compare January 29, 2017 13:57

z33ky requested changes Jan 29, 2017

View reviewed changes

z33ky reviewed Jan 29, 2017

View reviewed changes

Quaker762 force-pushed the shaderloader branch from 89e1dcf to 08779e4 Compare January 30, 2017 08:47

z33ky requested changes Jan 30, 2017

View reviewed changes

Quaker762 force-pushed the shaderloader branch from b49b9bd to 256d5c9 Compare January 31, 2017 09:02

Quaker762 force-pushed the shaderloader branch from d96d423 to ddf1425 Compare February 2, 2017 12:24

z33ky reviewed Feb 2, 2017

View reviewed changes

Quaker762 force-pushed the shaderloader branch from 5486af6 to c9dbaac Compare February 2, 2017 13:21

z33ky reviewed Feb 2, 2017

View reviewed changes

z33ky approved these changes Feb 2, 2017

View reviewed changes

Initial Implementation of Shader Programs

a5257b8

Can load, compile and link shaders from files. Currently untested (as we don't have a GLContext working as of yet!)

Quaker762 force-pushed the shaderloader branch from f55481f to a5257b8 Compare February 2, 2017 13:42

z33ky merged commit f6a8464 into Palm-Studios:master Feb 2, 2017

Quaker762 deleted the shaderloader branch February 3, 2017 03:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Implementation of Shader Programs #73

Initial Implementation of Shader Programs #73

Quaker762 commented Jan 27, 2017

Quaker762 commented Jan 27, 2017

Quaker762 commented Jan 29, 2017 •

edited

Loading

z33ky left a comment

z33ky Jan 29, 2017

Quaker762 Jan 29, 2017

z33ky Jan 29, 2017

z33ky Jan 29, 2017

Quaker762 Jan 30, 2017

z33ky Jan 29, 2017

z33ky Jan 29, 2017

Quaker762 Jan 30, 2017

z33ky Jan 29, 2017

Quaker762 Jan 30, 2017

z33ky Jan 30, 2017

Quaker762 commented Jan 30, 2017

z33ky Jan 30, 2017

Quaker762 Jan 31, 2017

z33ky commented Jan 30, 2017

Quaker762 commented Jan 31, 2017 •

edited

Loading

z33ky commented Jan 31, 2017

Quaker762 commented Jan 31, 2017 •

edited

Loading

z33ky commented Feb 2, 2017 •

edited

Loading

z33ky commented Feb 2, 2017

Quaker762 commented Feb 2, 2017

z33ky Feb 2, 2017

z33ky commented Feb 2, 2017

z33ky Feb 2, 2017

z33ky Feb 2, 2017

Initial Implementation of Shader Programs #73

Initial Implementation of Shader Programs #73

Conversation

Quaker762 commented Jan 27, 2017

Quaker762 commented Jan 27, 2017

Quaker762 commented Jan 29, 2017 • edited Loading

z33ky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quaker762 commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky commented Jan 30, 2017

Quaker762 commented Jan 31, 2017 • edited Loading

z33ky commented Jan 31, 2017

Quaker762 commented Jan 31, 2017 • edited Loading

z33ky commented Feb 2, 2017 • edited Loading

z33ky commented Feb 2, 2017

Quaker762 commented Feb 2, 2017

Choose a reason for hiding this comment

z33ky commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quaker762 commented Jan 29, 2017 •

edited

Loading

Quaker762 commented Jan 31, 2017 •

edited

Loading

Quaker762 commented Jan 31, 2017 •

edited

Loading

z33ky commented Feb 2, 2017 •

edited

Loading