Replace glut with glfw and imgui #718
Conversation
karjonas
force-pushed
the
glfw1
branch
5 times, most recently
from
bd8de9a to
8c63087
Compare
|
ping |
apps/BraynsViewer/CMakeLists.txt
Outdated
| braynsIO | ||
| braynsParameters | ||
| braynsUI | ||
| ${OPENGL_gl_LIBRARY} |
There was a problem hiding this comment.
Just a comment. I don't know since which version, but the recommended way of importing OpenGL is now different. See https://cmake.org/cmake/help/v3.10/module/FindOpenGL.html. This is going to be more future proof regarding platform independence as well.
apps/BraynsViewer/main.cpp
Outdated
|
|
||
| #include <deps/glfw/include/GLFW/glfw3.h> | ||
|
|
||
| Application* appInstance; | ||
|
|
||
| void cleanup() |
There was a problem hiding this comment.
If run is a function that returns (at it looks like), you don't need atexit, neither this function
apps/BraynsViewer/main.cpp
Outdated
|
|
||
| #include <deps/glfw/include/GLFW/glfw3.h> | ||
|
|
||
| Application* appInstance; |
There was a problem hiding this comment.
Not a necessity, but you can use a user pointer data attached to the GLFWindow to avoid the global variable: https://www.glfw.org/docs/latest/group__window.html#ga3d2fc6026e690ab31a13f78bc9fd3651
apps/BraynsViewer/main.cpp
Outdated
| int run(brayns::Brayns& brayns) | ||
| { | ||
| int windowWidth = 800; | ||
| int windowHeight = 600; |
There was a problem hiding this comment.
I've always found the default window size a bit small. @tribal-tec what's your opinion about increasing it?
apps/BraynsViewer/main.cpp
Outdated
| "] "; | ||
|
|
||
| GLFWwindow* window = glfwCreateWindow(windowWidth, windowHeight, | ||
| windowTitle.c_str(), NULL, NULL); |
There was a problem hiding this comment.
Where is this object destroyed?
apps/ui/Application.cpp
Outdated
| { | ||
| if (m_fbTexture) | ||
| { | ||
| glDeleteTextures(1, &m_fbTexture); |
There was a problem hiding this comment.
You're deleting Application with an atexit callback. Calling glDeleteTextures at that time is more than late. This is not critical, but you should review how you're doing the cleanup because segfaults at exit always give a bad impression (at least it does to me).
| if ((width != 0 && | ||
| height != 0) && // Zero sized interop buffers are not allowed in OptiX. | ||
| (m_width != width || m_height != height)) | ||
| { |
There was a problem hiding this comment.
Negate the condition for an early return.
| glVertex3f(frameSize.x, frameSize.y, 0); | ||
| glTexCoord2f(1.f, 0.f); | ||
| glVertex3f(frameSize.x, 0, 0); | ||
| glEnd(); |
There was a problem hiding this comment.
Since you're modernizing this code, you could try replacing the immediate mode draw calls with VBOs.
| ${CMAKE_CURRENT_SOURCE_DIR}/imgui/examples/imgui_impl_glfw.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/imgui/examples/imgui_impl_opengl3.cpp | ||
| ) | ||
| target_compile_definitions(imgui PRIVATE -DIMGUI_IMPL_OPENGL_LOADER_GLEW) |
There was a problem hiding this comment.
Does ImGui really need GLEW? The way you initialize it makes me think that it relies on glfw instead, but I may be wrong. I've been reading about it and it seems that removing GLEW would be a good idea as well as it's becoming a bit obsolete and it has some unresolved issues.
apps/ui/Application.cpp
Outdated
| @@ -47,6 +47,8 @@ | |||
| #include <algorithm> | |||
| #include <cassert> | |||
|
|
|||
| namespace | |||
| { | |||
| Application* appInstance = nullptr; | |||
|
|
|||
| static void glfwKeyCallback(GLFWwindow* /*window*/, int key, int /*scancode*/, | |||
There was a problem hiding this comment.
The idea of the anonymous is that you don't need the static qualifiers anymore.
No description provided.