Closed jslee02 closed 7 years ago
I like this reorganization. A few comments:
object
directory isn't adding much - consider renaming it geometry
and flattening a little.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. 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 :sweat:
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!
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.
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?
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.
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.
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.
Addressed by #163.
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
, andbroadphase
. Here (or see this) is a source tree I imagine (bold denotes directory):deprecated.h
,profiler.h
,time.h
,types.h
,warning.h
AABB.h
andOBB.h
constants.h
,geometry.h
(utilities),rng.h
,triangle.h
,variance3.h
box.h
,capsule.h
)BVH_model.h
and related filescollision_geometry.h
collision_object.h
,continuous_collision_object.h
gjk_solver_(indep/libccd).h
(could be renamed properly)(collision/distance/conservative_advancement)_func_matrix.h
collision.h
,distance.h
,continuous_collision.h
(collision/distance/continuous_collision)_(request/result).h
contact.h
,contact_point.h
,cost_source.h
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
andBVH
. 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
, andinterpolation
. We could either entirely remove them or move them frominclude/fcl
to somewhere like[fcl_root]/unused
or[fcl_root]/deprecated
.detail
namespaceThere 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
, underfcl
and putting them intodetail
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 thedetail
namespace.