replace much boost usage with C++11 equivalents #104

mamoll · 2016-03-23T15:08:07Z

A first step in eliminating Boost usage. Mostly low-hanging fruit.

jslee02 · 2016-03-23T15:16:54Z

src/shape/geometric_shapes_utility.cpp

-  // TODO: Replace with constexpr when we migrate to C++11 or
-  //       boost::math::constants::phi<FCL_REAL>() if boost version is greater
-  //       than 1.50.
+  const FCL_REAL phi = (1.0 + std::sqrt(5.0)) / 2.0;  // golden ratio


Can we consider using constexpr for phi, a, and b as we're migrating to using C++11?

Yes, but sqrt is not allowed in a constexpr (at least not with latest Xcode).

I found that none of the math functions in C++11 are not allowed to use with constexpr (gcc allows it, but it seems not to be allowed in C++14). Okay, we can't use constexpr here.

Could do it constexpr by precalculation (as I suggested for pi) if you can find a very high-precision calculator.

isucan · 2016-03-23T15:30:40Z

+1 for merging.

sherm1 · 2016-03-23T15:33:29Z

I'll test this on Windows since CI doesn't. Also, Travis failed.

mamoll · 2016-03-23T15:38:18Z

Travis is failing probably because of ancient compilers: gcc 4.6 and clang 3.4. I guess the Travis build matrix needs to be updated. We should also set up Windows builds on Appveyor. I'll make the change. I'll wait a bit to give others a chance to respond.

sherm1 · 2016-03-23T15:38:50Z

include/fcl/BV/RSS.h

@@ -111,7 +111,7 @@ class RSS
  /// @brief Volume of the RSS
  inline FCL_REAL volume() const
  {
-    return (l[0] * l[1] * 2 * r + 4 * boost::math::constants::pi<FCL_REAL>() * r * r * r);
+    return (l[0] * l[1] * 2 * r + 4 * M_PI * r * r * r);


Two problems with M_PI:

it is a macro and hence untyped; needs to have type FCL_REAL

it is non-standard (it is a Posix feature, not a C++11 one)

Since "pi" is the only boost math constant FCL used, I think the simplest thing would be to define an fcl::kPi or whatever that could be defined in a common header with something like

constexpr FCL_REAL kPi=FCL_REAL(/*lots of digits of pi copied from a reliable source*/);

M_PI caused compilation failures in Visual Studio 2015.

M_PI works with visual studio if you define _USE_MATH_DEFINES before including cmath

#ifndef _USE_MATH_DEFINES # define _USE_MATH_DEFINES #endif

M_PI works with visual studio if you define _USE_MATH_DEFINES

Thanks -- I know about that but it's a non-standard hack so I think Mark's fcl::constants::pi idea is better.

sounds good

jslee02 · 2016-03-23T15:45:26Z

Travis is failing probably because of ancient compilers: gcc 4.6 and clang 3.4.

This can be addressed by using trusty environment of Travis.

sherm1 · 2016-03-23T15:46:14Z

src/broadphase/broadphase_interval_tree.cpp

-    std::sort(endpoints[2].begin(), endpoints[2].end(), boost::bind(&EndPoint::value, _1) < boost::bind(&EndPoint::value, _2));
+    std::sort(endpoints[0].begin(), endpoints[0].end());
+    std::sort(endpoints[1].begin(), endpoints[1].end());
+    std::sort(endpoints[2].begin(), endpoints[2].end());


That looks a lot nicer in C++11 than in boost!

This is actually not a C++11-related change. (Well, sort of, s/boost/std/g didn't work.) The simplification is because of the operator< declaration in the Endpoint class.

sherm1 · 2016-03-23T15:55:33Z

This is great, Mark -- it's so nice to see all those boosts disappear! I hope it still works -- the test suite seems kind of short.

mamoll · 2016-03-23T16:01:43Z

Does anyone object to defining all mathematical constants (pi and phi) in, say, include/fcl/math/constants.h in the fcl::constants namespace as hard-coded double values:

namespace fcl
{
   namespace constants
   {
       constexpr FCL_REAL pi = 3.14159....;
       constexpr FCL_REAL phi = 1.618....;
   }
}

isucan · 2016-03-23T16:03:06Z

I think that's a good idea!

jslee02 · 2016-03-23T16:07:48Z

I was considering letting the compiler do the calculation of phi in compile like this, but @mamoll 's idea looks much simpler!

sherm1 · 2016-03-23T16:15:54Z

CMakeModules/CompilerSettings.cmake

@@ -1,3 +1,6 @@
+# force C++11 mode
+add_definitions(-std=c++11)


This causes zillions of warnings on Windows -- that should only be added for gcc & clang.

sherm1 · 2016-03-23T16:23:51Z

The top-level CMakeLists.txt specifies boost components here. Can you remove any of those with this PR?

scpeters · 2016-03-23T16:28:32Z

include/fcl/BVH/BVH_model.h


  /// @brief Fitting rule to fit a BV node to a set of geometry primitives
-  boost::shared_ptr<BVFitterBase<BV> > bv_fitter;
+  std::shared_ptr<BVFitterBase<BV> > bv_fitter;


this is a change to the public API, should we bump the major version number?

As far as I know, we might don't need to bump the major version number because FCL is still in 0.y.z.

good point, we should at least bump the minor version then

scpeters · 2016-03-23T16:30:24Z

I personally prefer separating big changes into smaller pull requests, but I'll defer to the crowd here.

sherm1 · 2016-03-23T16:30:40Z

include/fcl/profile.h

@@ -58,7 +58,7 @@
 #include <map>
 #include <string>
 #include <iostream>
-#include <boost/thread.hpp>
+#include <thread>


Add #include <mutex> here -- compile failed on Windows. C++11 doesn't promise that mutex will be defined in thread.

sherm1 · 2016-03-23T16:34:09Z

With these changes, this PR builds on Windows VS 2015 Win64, with only the same 4 test failures I saw before:

defined pi
commented out "-std=c++11"
included <mutex>

sherm1 · 2016-03-23T16:46:02Z

I like the constants proposal! I suggest using higher-precision constants that will get rounded down to FCL_REAL, to make sure the last bit is correct. For example:

constexpr FCL_REAL pi = FCL_REAL(3.141592653589793238462643383279502884197169399375105820974944592L);

That is a long double constant because of the trailing L.

…ode per compiler (MS VS doesn't like -std=c++11). eliminate some boost components in CMakeLists.txt

mamoll · 2016-03-23T21:21:05Z

I have hopefully addressed the main concerns. After this is merged, I'll fix the Travis CI config.

scpeters · 2016-03-23T22:16:13Z

There's still packaging issues:

bump minor version number
export c++11 flags in pkgconfig / cmake config files

scpeters · 2016-03-23T22:16:31Z

I guess those can be addressed later too?

mamoll · 2016-03-23T22:26:56Z

I can fix the pkgconfig / cmake config files, but wouldn't it make more sense to bump the version when you actually release? There's still quite a bit of boost stuff to eliminate.

mamoll · 2016-03-23T22:35:14Z

Actually, what cmake config files are you talking about? I thought you meant files like fcl-config.cmake, but this is currently not generated by FCL.

scpeters · 2016-03-23T22:49:41Z

You're right; there is no cmake config file. I just assumed this had one.

I like to bump the version number in the master branch (CMakeModules/FCLVersion.cmake) when breaking changes happen. This way people building from source can see a difference by the version number. I consider the package released when a new tag has been created.

scpeters · 2016-03-23T22:53:42Z

Thanks!

jslee02 · 2016-03-23T22:56:47Z

export c++11 flags in pkgconfig / cmake config files

~~Actually, FCL has a pkgconfig file. Can we simply add -std=c++11 like gazebo does?~~

Edit: Never mind. @mamoll already added in 65b948e.

mamoll · 2016-03-23T23:00:39Z

Apparently, MSVC doesn't like -std=c++11. This is why I add the flag only when not building with MSVC.

sherm1 · 2016-03-23T23:43:51Z

I have hopefully addressed the main concerns. After this is merged, I'll fix the Travis CI config.

Mark, can you fix Travis as part of this PR so we can confirm that it passes? I think that just means changing .travis.yml as a commit with the PR.

sherm1 · 2016-03-23T23:57:20Z

Reviewed changes -- looks great! Ready to merge pending build success.

scpeters · 2016-03-24T00:47:09Z

I think adding the following to .travis.yml would fix it:

sudo: required
dist: trusty

since trusty has a c++11 compiler

…t system library: test programs link against it

replace much boost usage with C++11 equivalents

9703a24

jslee02 reviewed Mar 23, 2016
View reviewed changes

sherm1 reviewed Mar 23, 2016
View reviewed changes

scpeters reviewed Mar 23, 2016
View reviewed changes

sherm1 reviewed Mar 23, 2016
View reviewed changes

sherm1 mentioned this pull request Mar 23, 2016

Added FCL to build and added FCLModel RobotLocomotion/drake#1890

Closed

define math constants in a separate file. specify C++11 compilation m…

5368a47

…ode per compiler (MS VS doesn't like -std=c++11). eliminate some boost components in CMakeLists.txt

bump minor version due to API/ABI change, update pkg-config file

65b948e

update Travis CI settings to use more modern compilers, put back Boos…

5b82c3b

…t system library: test programs link against it

mamoll merged commit a22c65d into flexible-collision-library:master Mar 24, 2016

jslee02 added this to the FCL 0.5.0 milestone Mar 24, 2016

This was referenced Jul 24, 2016

Consider eliminating FCL's dependence on boost #100

Closed

The nuke-boost TODO list #102

Closed

		@@ -1,3 +1,6 @@
		# force C++11 mode
		add_definitions(-std=c++11)

replace much boost usage with C++11 equivalents #104

replace much boost usage with C++11 equivalents #104

Conversation

mamoll commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isucan commented Mar 23, 2016

sherm1 commented Mar 23, 2016

mamoll commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherm1 commented Mar 23, 2016

mamoll commented Mar 23, 2016

isucan commented Mar 23, 2016

jslee02 commented Mar 23, 2016

Choose a reason for hiding this comment

sherm1 commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters commented Mar 23, 2016

Choose a reason for hiding this comment

sherm1 commented Mar 23, 2016

sherm1 commented Mar 23, 2016

mamoll commented Mar 23, 2016

scpeters commented Mar 23, 2016

scpeters commented Mar 23, 2016

mamoll commented Mar 23, 2016

mamoll commented Mar 23, 2016

scpeters commented Mar 23, 2016

scpeters commented Mar 23, 2016

jslee02 commented Mar 23, 2016

mamoll commented Mar 23, 2016

sherm1 commented Mar 23, 2016

sherm1 commented Mar 23, 2016

scpeters commented Mar 24, 2016