Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS compatibility #167

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ruberith
Copy link

@ruberith ruberith commented Apr 9, 2022

These adaptations allow builds using Clang on recent versions of macOS (tested on Apple silicon):

  • Updated and adapted versions of CompactNSearch, Discregrid and PositionBasedDynamics are used (see CompactNSearch#7, Discregrid#19 and PositionBasedDynamics#124). If the pull requests are accepted, CMake/NeighborhoodSearch.cmake and CMake/SetUpExternalProjects.cmake have to be updated to point to the original repositories again.
  • Clang seems to mistake some version files for headers, so these are given an extension.
  • The optimization flag -mcpu=apple-m1 is used for Apple silicon, for which -march=native is not yet supported.
  • OpenGL is deprecated for years on macOS, but still supported, so the corresponding warnings are silenced.
  • _NSGetExecutablePath is used to find program paths on macOS.
  • numpy is available for Apple silicon since version 1.21.0. The highest supported version can possibly be updated in setup.py.
  • AVX is disabled for ARM.
  • Catch2 is updated to v2.13.8.
  • Clang adheres more closely to the C++ standard regarding lookups, so overloaded operators with template parameters are moved to the namespace std.
  • Higher versions of OpenGL are only supported with core profiles on macOS which are not compatible with removed functionality. Since MiniGL follows the fixed function pipeline, OpenGL has to fall back to version 2.1. In this case, the corresponding implementation of ImGui is now used.
  • Recent versions of CMake automatically determine the correct flags for OpenMP and provide a target to link against. Nevertheless, Clang does not include an OpenMP implementation on macOS by default, but the user can easily install a build of LLVM libomp (e.g. via Homebrew) to be shared by all projects requiring OpenMP which is then detected by CMake.

@ruberith
Copy link
Author

To allow for the same visual experience on all platforms, I now have ported MiniGL to the OpenGL 3.3 Core Profile by replacing all fixed-function calls with "modern" equivalents.

@ruberith
Copy link
Author

AVX is now supported on ARM via SIMDe which forwards to the native intrinsics on x86 and provides equivalent implementations using NEON on ARM.

@digitalillusions
Copy link
Member

I actually tested this PR on my 2013 intel macbook air. while i was able to compile everything, it got a grey background in the gui and when trying to start the simulation i got a segfault. i will try to look into it some more soon to isolate the problem.

@ruberith
Copy link
Author

After further testing, I couldn't spot a noticeable performance difference with the SIMD option active, so I removed the dependency on SIMDe again. As recommended by Apple, I substituted the intrinsics in the AVX math wrapper with equivalent calls to the Accelerate framework for macOS systems which yielded the expected performance boost. Since I do not have an Intel Mac, I could not check whether the existing AVX calls or Accelerate calls are faster or if effectively the same intrinsics are used; the implementation details of the framework calls are only partially accessible.

@digitalillusions
Copy link
Member

Ok, so I can confirm that this indeed compiles for me on an M2 macbook air. I will also make another attempt using the intel macbook air in a couple of days.

@yangfengzzz
Copy link

I can't find CLOTH_BENDING_STIFFNESS in PBD because it already upgrade to 2.0.1, How can I fix PBDWrapper?

@digitalillusions
Copy link
Member

So I confirmed that this works on both an intel macbook air and an m2 macbook air. I think we should consider merging this soon since there were multiple requests for macos compatibility in the past. It should just be noted that its hard for us to maintain macos compatibility and that this will probably have to be done by the community.

@ruberith
Copy link
Author

@digitalillusions Thank you for verifying. I have just merged in the latest updates, so together with CompactNSearch#7, Discregrid#19 and PositionBasedDynamics#123, the compatibility should be ensured for now.

@digitalillusions
Copy link
Member

So we went through the PR in a bit more detail and were wondering if all of the proposed changes are actually strictly necessary to make it functional. Specifically, are all the changes to the GUI system and shaders actually necessary? Also, is the change to the alignment allocator necessary?

If not, would it be possible for you to split up the PR into changes actually required to run on MacOS, and optional improvements.

@ruberith
Copy link
Author

macOS does unfortunately only support the core profile of OpenGL 3.3, in which the fixed function calls (including GLU) do not work. All changes are specific replacements of these calls according to their documentation to have the same visual experience.
Similarly, for AVX compatibility on macOS ARM, I used equivalent calls from the Accelerate framework. Concerning the alignment allocator, I ran into errors when not choosing the exact alignment of the underlying data structure (16 bytes for Accelerate's simd::float8), even if 32 bytes are supposed to be fine. This seems to be fixed in a recent version of Apple Clang, so I will revert these changes.

Copy link
Member

@digitalillusions digitalillusions left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice job, thanks for the PR! Overall the changes look good to us. I think having some comments at the specified places would be helpful for future understanding of the code.

Also, we are currently working on some new features for splishsplash, so merging the PR will probably be delayed until after the new version is released. Once its released, you could rebase and then there should be no problem merging the PR.

CMakeLists.txt Show resolved Hide resolved
Utilities/CMakeLists.txt Show resolved Hide resolved
@ruberith
Copy link
Author

ruberith commented Dec 1, 2022

Thank you for the review. I will check where I can add comments in the code for clarification.

@digitalillusions
Copy link
Member

Thanks @ruberith for your continued efforts! It seems good to me so far, but somehow the selection box does not align correctly with the mouse position for me on macos. Have you encountered something similar? Tbf it seems to work on linux and windows.

@ruberith
Copy link
Author

This was due to retina displays having a framebuffer size of double the window size and should now be fixed. I have an external monitor connected, so I didn't notice that, thank you. :D Feel free to tell me if there is anything else I should change. Otherwise I would rebase as suggested for a clearer commit history.

@digitalillusions
Copy link
Member

Thanks for all the hard work! It would have probably taken me ages to find that particular problem! The changes look good to me, the only other minor problem that I experience is that key presses dont seem to be captured if you launch splishsplash with the interactive file selector. I.e. if you directly pass a file on the command line everything works as expected. However, if you just run the program and use the native file dialogue, the key presses dont do anything. Can you also reproduce this?

@ruberith
Copy link
Author

The pull request is now rebased on top of the main branch. Regarding the issues with the file dialog, I found a warning in the README of NFDe that might apply. When a scene file is specified on the command line, so that NFDe is initialized after GLFW, everything seems to work just fine if, for instance, a state file is loaded via the file dialog.

@digitalillusions
Copy link
Member

Sorry for the delay @ruberith, we have been very busy last month. I have taken some time to look through the PR and already notice some issues.

  1. when compiling on MacOS the main binary target SPHSimulator seems to work fine. However, when compiling the python bindings I get segmentation faults when trying to import the module. A part of this seems related to missing CMake flags from here Link time warning with pybind11 BlueBrain/nmodl#9, but this does not seem to completely fix the problem.
  2. when running the basic SPHSimulator executable on linux, a SIGSEGV occurs during teardown of the application in the destructor of the Shader object. Since this is something that was also part of this PR maybe you could take a look if you have access to a linux installation?

@ruberith
Copy link
Author

ruberith commented May 5, 2023

Unfortunately, I can not reproduce the first error. Using the latest versions of Apple Clang, CMake, and miniconda with Python 3.8, the interop builds and works in both directions on my system. I get the visibility warnings as well; however, they have no impact on the functionality of the bindings.

The second problem is to be expected as ~Shader is called after the destruction of the context by glfwDestroyWindow. I don't know why the application only segfaults on Linux, but we are here in the territory of undefined behavior. :D The latest commit should ensure that everything is destroyed properly.

@ruberith ruberith force-pushed the fix-macos branch 2 times, most recently from 6ed5df0 to 538d0c8 Compare September 20, 2024 23:07
@ruberith
Copy link
Author

I‘ve added some minor fixes for double-precision builds and rebased to bring the PR in a mergeable state again. After reviewing this PR and merging CompactNSearch#10 or CompactNSearch#11, I could also squash everything together into one single commit if you‘d like me to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants