search

Palm-Studios / sh3redux

SILENT HILL 3 Engine Remake in OpenGL and C++
GNU General Public License v3.0
162 stars 16 forks source link

Initial Implementation of Shader Programs #73

Closed Quaker762 closed 7 years ago

Quaker762 commented 7 years ago

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 commented 7 years ago

Also see #74

Quaker762 commented 7 years ago

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.

Quaker762 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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