Refactored DNASim library #8
Conversation
This adds a refactored and significantly revised CMake file for the DNASim library (in DNASim/ directory). This CMake file only deals with the library sources at the moment (i.e. no tests). Remarks about the changes: - Each sub-directory in the DNASim/src directory (e.g. dna/) owns a CMakeLists.txt file which defines the list of sources in that sub-directory. - The DNASim/src CMakeLists.txt file loops over the list of sub-directories and collect the source files that are part of the library. - Within DNASim, headers are now referenced relative to the root of the source directory (DNASim/src). As a consequence a large part of this commit is made of changes in the include directives to account for this new pattern. - The DNASim/src CMakeLists.txt also includes install directives but this might need to be adapted. - A library header (DNASim.h) is now part of the library to simplify the include process. In addition, a README is added to the DNASim directory (placeholder as of now) and install notes are available at the root of the repository in INSTALL.md Relates to #3
- Googletest is now included as a project submodule in the deps/ directory. The submodule is tracking v1.10.0. - The include directives in the test definitions are updated to simply include the DNASim library header instead of picking the required ones. - Tests run successfully. Relates to #1
- Add top-level cereal submodule in deps/ directory to track current master branch of cereal project (this seems to be v1.3.0 with a few minor fixes and additions). - The DNASim unit test for serialization (test_DNASimArchive) is now fixed because original implementation was ill-designed: it is not possible to save polymorphic types via base pointers and reload them to derived pointers. - The cereal registration calls for the DNASim polymorphic types are now included in the DNASim library header. Relates to #1
- Add nanoflann as a project submodule in the deps/ directory tracking v1.3.2. Relates to #1
Collision detection is not used in emDNA so it is removed from the codebase. This also makes it possible of getting rid of the ODE (Open Dynamic Engine) dependency. This commit also fixes a missing override keyword in `SphereShape` implementation which was causing a lot of compiler warnings. Relates to #1
- TCLAP library is relocated to the deps/ directory. There is not GitHub repo available so a submodule will be difficult to find. - The code from DNASim/src/dna_force_field_packager is relocated to the root of the repository. Discussion about how to handle that tool is in #5. Given that TCLAP was the last dependency to refactor for DNASim this commit closes #1.
- This commit adds a CLion project to the repository (under .idea). - Install notes are updated with the git clone instructions. Relates to #3
|
@stodolli just noticing now that you are a member of this project ... GitHub suggests you as a reviewer for the PR so here it is 😄; I let you bounce it back to @rty10 if this is more appropriate. Side note: this PR targets release/v1.0.0 because I am a bit reluctant to merge on master without running some test cases (see one of the question in #5). |
There was a problem hiding this comment.
Sorry for being a little bit late to the party here. There was quite a bit of browsing around I needed to do to get a sense of what had happened.
I managed to build the project and run the DNASim tests on Ubuntu just fine.
Comments:
- I like the more straightforward build structure. Never used git submodules before but it's kinda neat.
- The Install instructions are pretty clear. Ideally we can flesh them out further later.
Questions: - Do we need to include .idea in the repo? I thought that was usually ignored and left up to the individual contributor? It works ok for me but just wondering what's the common practice.
- I see you are targeting the release branch with this PR but what workflow are we keeping for this project? One of my confusions is also, are we keeping both main and master? Are we doing away with master altogether, now that the convention is to use main as default?
|
Never mind, I just realized now what must have happened with the main/master. @rty10 had an existing project in Bitbucket with a master branch so when pushing that project here with a default main, now we have both. Sorry, this is unrelated to the pull request. |
Well ... yes and no. It is pretty straightforward at the moment to just re-create a CLion project each time but once you start organizing targets and setting parameters on them it will quickly be painful. IMHO it is fine to leave the CLion project in the
Yes the main/master is a bit odd right now. I guess we can resolve that when doing the final merge of the release/v1.0.0 branch by targeting main. In terms of workflow ... well it is a bit ad-hoc right now. I wanted to keep working on release/v1.0.0 until everything is cleaned up and more importantly tested/validated. |
|
Ok it all makes sense. So to the main point, I am assuming any of us 3 can approve the merge commit to the release now, is that correct? Or since you initiated it, you cannot approve it yourself @nicocvn? |
|
Well I have no idea how this is setup. I asked @rty10 for admin permissions to actually take care of these things but it seems he had some issues with the user/permissions stuff (see #4). It seems I can approve it myself but I think it is better if someone else takes a look until we adjust permissions. |
Refactored DNASim library - Cleaned up CMake setup - Dependencies now handled via git submodules - Incomplete: emDNA code still needs to be refractored
Summary
This PR brings a cleaned up CMake setup for the project and the DNASim library as well as re-organized dependencies. From a code base point of view functionalities have not been changed nor altered with the exception of the OpenDE collision detection feature which has been removed. Note that, this is still a work in progress (see #3) as this does not cover the emDNA part of the code base.
Closes #1.