Improving handling game shaders #302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
changed the title
|
Its a bigger change but I find its nice to create RAII wrappers for the oGL stuff. |
ghost
commented
Aug 31, 2017
| @@ -212,6 +212,7 @@ GameRenderer::GameRenderer(Logger* log, GameData* _data) | |||
|
|
|||
| GameRenderer::~GameRenderer() { | |||
| glDeleteFramebuffers(1, &framebufferName); | |||
| glDeleteProgram(ssRectProgram); | |||
There was a problem hiding this comment.
Or maybe should we convert ssRectProgram to ShaderProgram?
There was a problem hiding this comment.
It should be doable. But isn't required for this PR.
ghost
changed the title
Merged
danhedron
requested changes
Oct 17, 2017
| const std::string& frag) override; | ||
| void setProgramBlockBinding(ShaderProgram* p, const std::string& name, | ||
| void setProgramBlockBinding(std::unique_ptr<ShaderProgram>& p, const std::string& name, |
There was a problem hiding this comment.
I think it would make more sense for the setProgramBlockBinding, setUniform and useProgram to take either a ShaderProgram* or a ShaderProgram&, the interface shouldn't care if they're in a unique_ptr or not, and they shouldn't be null anyway.
| @@ -212,6 +212,7 @@ GameRenderer::GameRenderer(Logger* log, GameData* _data) | |||
|
|
|||
| GameRenderer::~GameRenderer() { | |||
| glDeleteFramebuffers(1, &framebufferName); | |||
| glDeleteProgram(ssRectProgram); | |||
There was a problem hiding this comment.
It should be doable. But isn't required for this PR.
added 6 commits
October 26, 2017 23:02
https://github.com/rwengine/openrw/search?utf8=%E2%9C%93&q=glDeleteProgram&type= In project there's no necessary clearing call of glDeleteProgram.
Each shader program should be taken care by glDeleteProgram. To make it works/usefull, each shader's raw ptr will convertet to unique_ptr in next commit.
Actually deleting isn't handled, so this commit removes memory leak.
danhedron
approved these changes
Oct 29, 2017
ghost
deleted the
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ok, I've edited pull request, because now happens much more. ;)
I've found some problems. First OpenGL programs have never been removed/closed. So I've added code doing it (in commit 75b188f), but now destructor have never been called, because memory allocated by OpenGLShaderPrograms are handled by raw pointers and never is freed. So I converted them unique pointers(commit 30a12ea).
I also improved the management of shaders in function compileProgram(commits 86d9122, 49f38e3, c858454). It should reduce memory usage.
Template I used:
glCreateProgram
glCreateShader
glShaderSource
glCompileShader
glAttachShader
glLinkProgram <- after this line opengl got everything it needs, you can free shader resources
glDetachShader
glDeleteShader
for{//render loop
glUseProgram
//...drawing operations
glUseProgram(0);
}
glDeleteProgram
Removes this type of memory leak:
==21845== 1,075 bytes in 1 blocks are possibly lost in loss record 2,921 of 3,413
==21845== at 0x4C2BE7F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21845== by 0xDE31CA2: ??? (in /usr/lib/xorg/modules/dri/radeonsi_dri.so)
==21845== by 0x577114: compileShader(unsigned int, char const*) (OpenGLRenderer.cpp:10)
==21845== by 0x5772F6: compileProgram(char const*, char const*) (OpenGLRenderer.cpp:47)
==21845== by 0x577BE0: OpenGLRenderer::createShader(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&, std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&) (OpenGLRenderer.cpp:220)
==21845== by 0x57F7B0: WaterRenderer::WaterRenderer(GameRenderer*) (WaterRenderer.cpp:12)
==21845== by 0x5611D2: GameRenderer::GameRenderer(Logger*, GameData*) (GameRenderer.cpp:77)
==21845== by 0x46B9A4: RWGame::RWGame(Logger&, int, char**) (RWGame.cpp:35)
==21845== by 0x45B0F9: main (main.cpp:13)
References:
#239