Modified source code for added compatibility on MacOS#30
Conversation
fwilliams
left a comment
fwilliams
left a comment
There was a problem hiding this comment.
It seems to me that this PR is doing several things at once. The first is changes to the OpenGL rendering code and general changes to make the application compile and render on OSX. The second are changes to the viewport in the bounding_polygon_plugin.. The third is adding mouse events in the selection_plugin.
The rendering code and compiler changes look good to me. We should wrap the glDebugCallback stuff in a cleaner way so we don't have #ifdefs everywhere but this can be done when all the issues have been resolved.
For the viewport changes, I don't quite understand the problem being solved here. Could you give me a detailed explanation?
For the mouse event changes. I think it's a very nice feature to have, but I would like to limit the functionality changes per pull request. If there are too many changes in one PR, it's hard to track down bugs.
| #ifdef WIN32 | ||
| glEnable(GL_DEBUG_OUTPUT_SYNCHRONOUS); | ||
| glDebugMessageCallback(log_opengl_debug, NULL); | ||
| #endif |
There was a problem hiding this comment.
We want glDebugMessageCallback on Linux too.
There was a problem hiding this comment.
You moved other stuff to a helper function, let's wrap this in a helper function called init_opengl_debug() (or something more appropriate if you want). You can put the function alongside the other helper functions you wrote.
fwilliams
left a comment
fwilliams
left a comment
There was a problem hiding this comment.
Awesome that it works! There is still some small cleanup to do and we need to get the CI working, but this is almost done 💯
| #ifdef WIN32 | ||
| glEnable(GL_DEBUG_OUTPUT_SYNCHRONOUS); | ||
| glDebugMessageCallback(log_opengl_debug, NULL); | ||
| #endif |
There was a problem hiding this comment.
You moved other stuff to a helper function, let's wrap this in a helper function called init_opengl_debug() (or something more appropriate if you want). You can put the function alongside the other helper functions you wrote.
| set_property(TARGET utils PROPERTY CXX_STANDARD 14) | ||
| set_property(TARGET utils PROPERTY CXX_STANDARD_REQUIRED ON) | ||
| target_link_libraries(utils igl::core igl::opengl igl::cgal igl::triangle spdlog Qt5::Core Qt5::Widgets spdlog) | ||
| include_directories(external/libigl/external/glfw/include) |
There was a problem hiding this comment.
You shouldn't need to set this globally. The target glfw should handle setting up include paths. You can remove this line.
| float center_point_size = 12.f; | ||
| float selected_center_point_size = 14.f; | ||
| #ifdef __APPLE__ | ||
| float macos_widget_scaling_factor = 4.5f; // Scaling factor for widget window range on macOS |
There was a problem hiding this comment.
can you explain why it's set to 4.5. At some point, somebody is going to read this code and have to figure out why this is the magic number that works. Explain the different conversions and what's going on. This comment should be detailed.
|
|
||
| float xscale, yscale; | ||
| glfwGetWindowContentScale(window, &xscale, &yscale); | ||
| #ifdef __APPLE__ |
There was a problem hiding this comment.
Same thing as above. Let's have a descriptive comment.
fwilliams
left a comment
fwilliams
left a comment
There was a problem hiding this comment.
last small changes, then let's do a run on all the platforms and we can merge
| bool load_rawfile(const std::string& rawfilename, const Eigen::RowVector3i& dims, std::vector<uint8_t> &out, std::shared_ptr<spdlog::logger> logger); | ||
|
|
||
| void init_opengl_debug(); | ||
| // void init_opengl_debug(typedef void (APIENTRY *GLDEBUGPROC)(GLenum source,GLenum type,GLuint id,GLenum severity,GLsizei length,const GLchar *message,const void *userParam) callback); |
There was a problem hiding this comment.
Sorry about that. Should have known better
| } | ||
|
|
||
| void init_opengl_debug() { | ||
| // void init_opengl_debug(void (*)(GLenum, GLenum, GLuint, GLenum, GLsizei, const GLchar *, const void *) callback) { |
There was a problem hiding this comment.
Sorry about that. Should have known better
2 participants
Updated the source code of files involved with rendering of 3D and 2D image scans and volumes with modifications to implementation of OpenGL shaders for feature selection and replacement of shader storage buffer objects with texture buffers for compatibility on MacOS. Also changed dimensions for viewports of application menu windows (i.e. feature selection, endpoint selection, bounding polygon) for increased usability on MacOS systems. Updated CMake file to streamline initial compilation.