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

Add CMake support #15

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

Conversation

phcerdan
Copy link

@phcerdan phcerdan commented Oct 8, 2019

  • Creates aabbcc library
  • Creates Targets and Config file for easy usage by
    third party libraries using find_package(aabbcc)
  • Add option to build the demo/hard_disc

Missing/TODO:

  • Add option to build python wrapping
  • Add target to create the header-only version

@lohedges
Copy link
Owner

lohedges commented Oct 9, 2019

I'll be happy to accept when the TODO list is complete, since those are the main ways that people use this library. At present, running cmake would obliterate the existing Makefile, which is still the only way to generate the Python wrapper and header-only library. (People could checkout the original file to get it back, but unexperienced users might not know what to do.)

I can help out if needed when I get a spare bit of time since I've done the same thing before for other projects. Basically you just need to use FindPythonLibs and FindSWIG. Alternatively, we could also switch to using pybind11, which could be bundled as a submodule. (Meaning that we don't have to worry about people having SWIG installed.) This comes with its own set of CMake build files which will find the appropriate Python library for you. For the header-only target you should be able to use something like ADD_CUSTOM_TARGET with an appropriate COMMAND to generate the required hpp file.

- Creates aabbcc library
- Creates Targets and Config file for easy usage by
  third party libraries using find_package(aabbcc)
- Add option to build the demo/hard_disc

Missing/TODO:
- Add option to build python wrapping
- Add target to create the header-only version
This includes some string replacements in header and source files
@phcerdan
Copy link
Author

phcerdan commented Oct 9, 2019

Makefile and CMakelists.txt can coexist with no problems. But CMakeLists.txt provide easier integration with other projects already using CMake.

I have added the add_custom_target header-only to generate the header file from current head and source files.

Why is the advantage of having a cpp file, instead of a template version? A library can be added with the instantiation of the Dimension (or multiple Dimensions) the user is interested in.

I have created a template version, with Dimension as a template parameter in a recent commit of my fork: phcerdan@19fc6fb

Let me know if you would be interested in integrating it here.

@lohedges
Copy link
Owner

lohedges commented Oct 9, 2019

Makefile and CMakelists.txt can coexist with no problems. But CMakeLists.txt provide easier integration with other projects already using CMake.

Agreed, e.g. by using a build directory. However, this would need to be documented so that the user knows how to build the library with Make or CMake.

Why is the advantage of having a cpp file, instead of a template version? A library can be added with the instantiation of the Dimension (or multiple Dimensions) the user is interested in.

No particular reason, I just hadn't considered templating the AABB at the time of writing, rather allowing the user to specify the dimension with an argument. I've had people using this library with additional dimensionality, e.g. time, so the existing version gave them the ability to work in higher dimension without updating the code. (Sorry if this isn't clear, but in your fork would one need to include a src/AABB_ND.cpp file for each dimension and then update the CMakeLists.txt file when building the library?)

I tried compiling your example, but get an error when I try to run the demo code:

Inserting large particles into AABB tree ...
Tree generated!

Inserting small particles into AABB tree ...
Tree generated!

Running dynamics ...
terminate called after throwing an instance of 'std::invalid_argument'
  what():  [ERROR]: AABB lower bound is greater than the upper bound!

(Note that I just compiled the demo by hand, linking against the library generated by your CMake build.)

@phcerdan
Copy link
Author

phcerdan commented Oct 9, 2019

(Note that I just compiled the demo by hand, linking against the library generated by your CMake build.)

Try building the demo with cmake:

cd build-folder;
cmake -DAABBCC_BUILD_DEMOS:BOOL=ON ../src-folder;
make -j8;
./demos/hard_disc;

It's working for me.

@phcerdan
Copy link
Author

phcerdan commented Oct 9, 2019

(Sorry if this isn't clear, but in your fork would one need to include a src/AABB_ND.cpp file for each dimension and then update the CMakeLists.txt file when building the library?)

The only advantage of having a library instead of a template class is that the user can save compilation time just linking to the library.

With a template, instead of calling setDimension on an existing tree, the user will define the type of the tree with the dimension.

AABB aabb;
aabb.setDimension(N)
// instead, using a templated class
AABB<N> aabb;

This will allow (also done in the fork) to use fixed size arrays (such as std::array) for positions and math-vectors, instead of the dynamic std::vector (which I guess the compiler can optimize better)

If the user wants to avoid recompilation, and he/she is only interested in a certain dimension, can compile a library just once. But if compilation time is not a problem, user does not have to worry about it, and just use the AABB<N> with whatever N.

But yes, for python wrappings the user would need to choose what dimensionality (can be many) is needed.
I am just exploring your code by doing, not sure what is best for your user base yet :)

@lohedges
Copy link
Owner

lohedges commented Oct 9, 2019

Thanks for taking the time to explain things, much appreciated.

Try building the demo with cmake:

Great, it's working for me now too. I somehow missed that the demo could be built separately when scanning your CMakeLists.txt file.

This will allow (also done in the fork) to use fixed size arrays (such as std::array) for positions and math-vectors, instead of the dynamic std::vector (which I guess the compiler can optimize better)

The use of fixed-size arrays has come up before. It turns out that, for the use cases seen so far, the speed gain is insignificant versus the time taken to traverse the AABBTree, hence sticking with std::vector for flexibility. Obviously, with a templated approach std::array containers make perfect sense.

But yes, for python wrappings the user would need to choose what dimensionality (can be many) is needed.

Perhaps there could be a separate version that's used for the Python wrappings, or use the templates to wrap specific objects for each dimension and expose the to Python, e.g. AABB2D, AABBTree2D, AABB3D, AABBTree3D, etc.

Cheers.

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.

2 participants