Proposal: Source tree reorganization

[jslee02]

I think the structure of FCL classes is well designed, but the structure of the source tree doesn't seem to accord with it. Here are some issues and suggestions I found while working on #150 and #154.

Minimal classes in a header file

Some files have too many classes in them. This makes hard to learn FCL from navigating the source tree and to maintain the source code. Also, they are vulnerable to circular dependency issues as FCL will be templatized (#154). I propose splitting those files and having minimal classes (ideally single class) in a header file. One exception would be having specialized template classes in the same file of the template class.

Reorganizing source tree structure

The top level directories and files are scattered. I suggest grouping them as common, math, object, narrowphase, and broadphase. Here (or see this) is a source tree I imagine (bold denotes directory):

• common
  • deprecated.h, profiler.h, time.h, types.h, warning.h
• math
  • bv: Bounding volumes such as AABB.h and OBB.h
  • motion: Motions used for continuous collision detection
    • taylor_model
  • sampler
  • constants.h, geometry.h (utilities), rng.h, triangle.h, variance3.h
• object
  • geometry
    • shape: Shapes (e.g., box.h, capsule.h)
    • bvh: BVH_model.h and related files
    • octree
    • collision_geometry.h
  • collision_object.h, continuous_collision_object.h
• narrowphase
  • detail: Algorithm selectors and under-hood algorithms for collision geometries
    • traversal: Traversal nodes of BVH for different combination of collision geometries (a lot)
    • Algorithm selectors for primitive shapes: gjk_solver_(indep/libccd).h (could be renamed properly)
    • Algorithm table for collision geometries: (collision/distance/conservative_advancement)_func_matrix.h
  • Main interface for narrowphase queries: collision.h, distance.h, continuous_collision.h
  • Options and result data for the narrowphase queries: (collision/distance/continuous_collision)_(request/result).h
  • Basic data structures for the narrowphase queries: contact.h, contact_point.h, cost_source.h
• broadphase
  • detail: Internal data structure for the broadphase algorithms
  • broadphase related files

Lower cases for directory names

It seems using lower cases is the convention for directory names, but there are few directories doesn't obey the convention such as BV and BVH. It would be good to be consistent with the lower case convention.

Top level FCL header

It would be useful to provide a single header file, fcl.h that includes all the necessary fcl headers to use all the fcl functionalities. This would allow the user doesn't need to learn the structure of the source tree to find headers they want.

Unused code

There are several directories that include unused implementations: articulated_model, learning, simd, and interpolation. We could either entirely remove them or move them from include/fcl to somewhere like [fcl_root]/unused or [fcl_root]/deprecated.

detail namespace

There are many implementations that might not be necessary to be exposed to the user who is only interested in using FCL rather than understanding the detailed implementation. We could hide them by using nested namespace, detail, under fcl and putting them into detail directory. This would make easier to learn FCL for casual users. Also, this could be useful to developers because the developers would be less stressful about the API compatibility for the detail namespace.

[sherm1]

I like this reorganization. A few comments:

• I think the object directory isn't adding much - consider renaming it geometry and flattening a little.
• I like the detail namespace. Usually that's used in headers along with the user-visible stuff to mark things they don't have to think about, rather than being in a separate directory.
• Consider adopting a coding style guide. For Drake we decided to use the Google style guide plus local clarifications, exceptions, and additions. I'm not particularly wild about Google style, but it has actually worked out well because it saves having to make repeated decisions about unimportant stuff. Drake still has lots of non-compliant code, we just use the guide for new code and gradually convert the old code when someone feels like it.

[jslee02]

I think the object directory isn't adding much - consider renaming it geometry and flattening a little.

In that case, it's to me not clear whether [continuous]_collision_object.h should be in geometry or narrowphase. If we regard CollisionObject as a simple extension of CollisionGeometry with transformation data then it would make sense with being in geometry. Otherwise, geometry should hold only collision geometries, and CollisionObject could be in narrowphase since it's first used by the narrowphase API.

Usually that's used in headers along with the user-visible stuff to mark things they don't have to think about, rather than being in a separate directory.

I strongly prefer having detail implementation in separate directories unless there is a compelling reason. I believe one of quickest way of figuring out a project is scanning the source tree, and putting all the detail files into detail have the same effect of marking them they're details when we navigating the source tree without opening the files to see if they are in detail namespace.

Consider adopting a coding style guide.

I actually about to propose using code formatter (probably clang-format) with a style but haven't decided which style to adopt. I think Google style guide is comprehensive and mature, but I'm not a big fan of Google style guide because we have some disagreement on several code styles. One example is where to place curly brace for function definition and class: on the same line vs new line. If we have a chance to make some additions and exceptions as Drake did, it would be fine to me. I hope the final style isn't too far from my preference though 😓

[sherm1]

If you can separate the detail stuff that makes sense. But often it is closely tied to some more user-visible code has to be mixed in. Also, there are likely to be math details, geometry details, narrowphase details, etc. -- did you mean to have a subdirectory in each one?

We use clang-format with this style. We don't require people to use it, but we guarantee that if they do they won't get whining from cpplint about it!

[jslee02]

If you can separate the detail stuff that makes sense.

Yeah, I believe it's possible.

But often it is closely tied to some more user-visible code has to be mixed in.

For those user-visible code, I think they shouldn't be in detail. I define objects to be in detail as any objects that are not exposed to the public API. If an object is user-visible (through public members) then it means the user needs to know how to use it, which is not detail any more.

did you mean to have a subdirectory in each one?

Yes. I'm working on this branch for this and you could see how it would look like.

Edit:

We use clang-format with this style.

That's so simple! I will create a new issue for the code style when ready.

[sherm1]

I define objects to be in detail as any objects that are not exposed to the public API.

The problem is that when much of the code is in headers it is all exposed. I've seen detail used within headers for helper classes and the like that have to be physically present in the user's compilation unit but should only be used internally. I believe the standard template library, for example, has lots of internal junk hidden away like that. It is not always easy to segregate that kind of code into separate files.

Possibly we might be talking about different things?

[mamoll]

If header files are moved to different locations, all code that depends on FCL will break. Perhaps you could create stubs for the old headers that have a #warning and #include of the header file in the new location? OTOH, if enough changes are happening before the next release that things will break regardless, then perhaps that won't be necessary.

[jslee02]

Perhaps you could create stubs for the old headers that have a #warning and #include of the header file in the new location?

That's my first approach. But I realized FCL 0.6 will break the API anyways because of source tree reorganization. So as a band aid I propose the top level FCL header fcl.h (in #161) that includes all the necessary FCL headers at once.

[jslee02]

The problem is that when much of the code is in headers it is all exposed. I've seen detail used within headers for helper classes and the like that have to be physically present in the user's compilation unit but should only be used internally.

Oh, I see. I think it would be fine to have those helper classes in the same file. I intend to hide files that all the containing classes are in detail namespace rather than hiding all the code segment of detail namespace. For example, I moved all the traversal files to under detail directory that aren't necessary to take a look at for casual use of FCL.

[jslee02]

Addressed by #163.