Added directional lights #2
Conversation
There was a problem hiding this comment.
Thank you for doing this! Looks mostly good, although there are some issues with the handling of the uniforms, which I commented on.
This approach is fine for now, but I'd eventually like to get the lights to work a little more like the objects work, where the objects are encapsulated in a group, and that group has a subsystem that owns its shader program and all the uniforms and such. The solution where I'm passing the uniforms down into the scene when rendering the lights is pretty awkward.
| @@ -5,17 +5,36 @@ | |||
| namespace rev { | |||
|
|
|||
| class Light { | |||
There was a problem hiding this comment.
I think having a shared base class here isn't really worth it, IMO. They only really share the one member and some getters, and the inheritance looks kind of like polymorphism even though they can't be polymorphic (no virtual functions). I they do share some properties and there may be a way to leverage that at some point, but let's not worry about that right now. After all, the fragment shaders they use are mostly copy/pasted anyway even though they share a lot of the same code.
engine/include/rev/Light.h
Outdated
| const glm::vec3& getBaseColor() const; | ||
| void setBaseColor(const glm::vec3& color); | ||
|
|
||
| const glm::mat4x4& getLightSpaceTransform() const; |
There was a problem hiding this comment.
I think these are unimplemented and uncalled.
There was a problem hiding this comment.
Forgot to delete these
|
|
||
| uniform vec3 lightDirection; | ||
| uniform vec3 lightBaseColor; | ||
| uniform vec3 camPosition; |
There was a problem hiding this comment.
OMFG this uniform was never set before! I knew there was something weird about how the specular highlights were moving in relation to the camera.
There was a problem hiding this comment.
Actually it was set above, nevermind.
engine/src/SceneView.cpp
Outdated
| // Render all point lights | ||
| { | ||
| ProgramContext programContext(_pointLightingProgram); | ||
| _lightPosition = _pointLightingProgram.getUniform<glm::vec3>("lightPosition"); |
There was a problem hiding this comment.
You should get these uniforms once, and store them (probably above, where all the other uniforms are initialized). This also means you'll have to store separate versions of base color and cam position for the two shaders.
Also, make sure you actually set the value of the camera uniform before you actually call renderAll[Directional|Point]Lights on the scene. As of right now, it is only being set above right after getting the render context, which means the first time the uniform object is bound to nothing (not sure what the behavior is there), and all subsequent times it is only bound to the last bound shader (which in this case would be the directional one.
freaknbigpanda
force-pushed
the
adding_directional_lights
branch
from
7cc04e5 to
4bdd4b0
Compare
freaknbigpanda
force-pushed
the
adding_directional_lights
branch
from
4bdd4b0 to
1132b88
Compare
So I just did the super simple way of doing this and added a new shader for the directional lights and then inside the render loop I am switching between the two shaders using glUseProgram (i.e. instantiating a ProgramContext with the appropriate ProgramResource).
I do want to try to use the same shader for everything so I am going to look into that next, I think it will simplify the code. Let me know what you think too!