Modified source code for added compatibility on MacOS #30

michizhou · 2019-07-17T17:35:42Z

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.

fwilliams

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.

fwilliams · 2019-07-17T18:43:52Z

src/main.cpp

    glEnable(GL_DEBUG_OUTPUT_SYNCHRONOUS);
    glDebugMessageCallback(log_opengl_debug, NULL);
+#endif


We want glDebugMessageCallback on Linux too.

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.

src/ui/bounding_polygon_plugin.cpp

src/ui/bounding_widget_2d.cpp

src/ui/endpoint_selection_plugin.cpp

src/ui/selection_plugin.cpp

fwilliams

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 💯

src/ui/bounding_polygon_plugin.cpp

fwilliams · 2019-07-24T16:58:10Z

src/main.cpp

    glEnable(GL_DEBUG_OUTPUT_SYNCHRONOUS);
    glDebugMessageCallback(log_opengl_debug, NULL);
+#endif


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.

src/ui/bounding_widget_2d.h

src/ui/endpoint_selection_plugin.cpp

src/ui/endpoint_selection_plugin.h

src/ui/state.cpp

src/ui/selection_plugin.h

src/ui/state.h

src/utils/utils.cpp

src/utils/utils.h

fwilliams · 2019-07-25T15:50:03Z

CMakeLists.txt

@@ -125,7 +130,8 @@ list(FILTER UTILS_HEADER EXCLUDE REGEX cgal*)
 add_library(utils STATIC ${UTILS_SRCS} ${UTILS_HEADER})
 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)


You shouldn't need to set this globally. The target glfw should handle setting up include paths. You can remove this line.

fwilliams · 2019-07-25T15:52:07Z

src/ui/bounding_widget_2d.h

@@ -39,6 +39,9 @@ class Bounding_Polygon_Widget {
    float split_point_size = 7.f;
    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


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.

fwilliams · 2019-07-25T15:52:31Z

src/ui/fish_ui_viewer_plugin.cpp

@@ -121,6 +129,9 @@ float FishUIViewerPlugin::hidpi_scaling() {

    float xscale, yscale;
    glfwGetWindowContentScale(window, &xscale, &yscale);
+#ifdef __APPLE__


Same thing as above. Let's have a descriptive comment.

fwilliams

last small changes, then let's do a run on all the platforms and we can merge

fwilliams · 2019-07-26T01:15:58Z

src/utils/utils.h

@@ -44,7 +44,8 @@ bool load_rawfile(const std::string& rawfilename, const Eigen::RowVector3i& dims

 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);


rm commented code

Sorry about that. Should have known better

fwilliams · 2019-07-26T01:16:09Z

src/utils/utils.cpp

@@ -189,10 +189,11 @@ bool load_rawfile(const std::string& rawfilename, const Eigen::RowVector3i& dims
    return true;
 }

-void init_opengl_debug() {
+// void init_opengl_debug(void (*)(GLenum, GLenum, GLuint, GLenum, GLsizei, const GLchar *, const void *) callback) {


rm commented code

Sorry about that. Should have known better

michizhou and others added 11 commits June 11, 2019 17:25

Modified AppVeyor build configuration settings

e10db84

Added installation of new software by AppVeyor

c56303d

Modified AppVeyor build compilation settings

339892e

Changed paths for AppVeyor build libraries

5645606

Added return value to header function

548d888

Revised AppVeyor library compilation paths

e52ce30

Modified AppVeyor after build command script

0c5fd0e

Added Travis build instance

7e186e0

Modified Travis compiler environment settings

d8799ed

Updated Travis build compiler configuration

1b7fb81

Modified source code for added compatibility on MacOS

fc05c3a

michizhou requested a review from fwilliams July 17, 2019 17:35

fwilliams requested changes Jul 17, 2019

View reviewed changes

michizhou added 2 commits July 22, 2019 15:03

Changed features for rendering and UI functionality on MacOS

525b13a

Resolved MacOS bounding polygon selection and rendering issues

0013a21

michizhou force-pushed the michael branch from ed0a6ef to a05e761 Compare July 24, 2019 11:54

Added dependencies for debug group functions

35e12f7

michizhou force-pushed the michael branch from 40125ac to 35e12f7 Compare July 24, 2019 15:35

michizhou requested a review from fwilliams July 24, 2019 15:55

fwilliams requested changes Jul 24, 2019

View reviewed changes

Added MacOS environment builds to Travis

b38e0ec

michizhou force-pushed the michael branch from d6be11f to 7dd1e62 Compare July 25, 2019 09:11

Modified OpenMP and Qt5 compilation settings

246fe30

michizhou force-pushed the michael branch from 3124f56 to 246fe30 Compare July 25, 2019 11:39

Refactored helper functions and state constants

91116d1

fwilliams requested changes Jul 25, 2019

View reviewed changes

Added onto comments and debug helper function

ccdc39a

michizhou requested a review from fwilliams July 25, 2019 19:28

fwilliams requested changes Jul 26, 2019

View reviewed changes

Modified debug helper function implementation

db06ce5

michizhou added 2 commits July 29, 2019 10:12

Corrected mouse coordinate discrepancies with scaling

1964a44

Addendum 1

441b955

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified source code for added compatibility on MacOS #30

Modified source code for added compatibility on MacOS #30

michizhou commented Jul 17, 2019

fwilliams left a comment

fwilliams Jul 17, 2019

fwilliams Jul 24, 2019

fwilliams left a comment

fwilliams Jul 24, 2019

fwilliams Jul 25, 2019

fwilliams Jul 25, 2019

fwilliams Jul 25, 2019

fwilliams left a comment

fwilliams Jul 26, 2019

michizhou Jul 26, 2019

fwilliams Jul 26, 2019

michizhou Jul 26, 2019

Modified source code for added compatibility on MacOS #30

Are you sure you want to change the base?

Modified source code for added compatibility on MacOS #30

Conversation

michizhou commented Jul 17, 2019

fwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment