Update Drake to RL master; InstQP is MATLAB-free

gizatt commented 8 years ago

Big pile of changes to enable us to bring Drake back up to close to RL master. Two big pain points for compatibility:

1) Drake master requires an upgrade of Eigen to 3.2.9. This brakes compatibility at a couple of places:

common_utils need a minor fix. @iamwolf already did this fix on a branch of that pod; if that's ready to be merged to master of that pod then maybe now's the time?
kinematics-utils needs a fix in orocos_kdl. I've made minor changes in a branch and can pull request them. This'll need to be done before this can be merged.

2) In prep for running the MIT controller on Val, Robin made is possible to remove all MATLAB from the Inst QP controller, from the Drake side. This finished the job from the OH side. We just tested on Atlas and walked for the first time with no MATLAB in the controller! (It's so fast to start up that we stopped the controller and restarted it, and the robot maintained balance. :))

rdeits commented 8 years ago

This is so awesome. Thanks for making it happen, @gizatt :)

mauricefallon commented 8 years ago

This is so awesome. The improvements this can enable are huge. Fantastic!

Stopping the controller for the hell of it ... Crazy!

Sent from my phone

On 9 Mar 2016, at 23:06, Robin Deits notifications@github.com wrote:

This is so awesome. Thanks for making it happen, @gizatt :)

— Reply to this email directly or view it on GitHub.

The University of Edinburgh is a charitable body, registered in Scotland, with registration number SC005336.

patmarion commented 8 years ago

I added a new commit to remove the python package symlink for lcmtypes/drake.

rdeits commented 8 years ago

Looks good otherwise :)

rdeits commented 8 years ago

As a side note: after the recent changes to drake & OH, the control rate has gone down substantially (from 800-1000 Hz to around 400-600 Hz). I believe this is because we've switched from sending body IDs in the qp_controller_input to sending body names as strings. That's necessary because the PlanEval (which still uses the Matlab URDF tools) and the controller (which is all c++) now disagree about the indices of various bodies. We could make this more efficient, but I think the correct solution is to just get all of the Matlab out of the PlanEval as well, in which case everyone will agree about the IDs again and we can stop using strings.

rdeits commented 8 years ago

Walking appears to be just fine with the current control rates, though, so I don't think this is a major issue.

wxmerkt commented 8 years ago

Thanks for pushing this forward, this is amazing!

The build server says everything's fine (it's also using precompiled versions of PCL and OpenCV!) but I've noticed compilation issues when upgrading to Eigen 3.2.92 on my machines. I made a fresh clone of this branch (and for that matter of the one when I tried to upgrade oh/drake to RL/drake for research ~three weeks ago) and when compiling PCL gets completely messed up with the new Eigen (I'll post the errors in a bit when the show up again).

Could you confirm whether you've built everything from scratch including PCL? Because this might mess up our externals for fresh clones that do not use precompiled versions of PCL. So we might want to consider at this point to add the PCL ppa that's already mentioned to our core instructions and remove it from externals.

[Update: I can get around the Eigen-PCL-error by running make pcl on a single job in externals. When used with the general make it does not work. I can repeat this over and over on two different machines. Odd. Did the new eigen break the externals build as-is?]

wxmerkt commented 8 years ago

Here's the PCL Eigen error (gist). Where it starts throwing:


-- Build files have been written to: /home/wxm/oh-distro/software/externals/pod-build/src/pcl-build
[  6%] Performing build step for 'pcl'
[  1%] Building CXX object common/CMakeFiles/pcl_common.dir/src/point_types.cpp.o
In file included from /home/wxm/oh-distro/software/externals/pcl/common/include/pcl/point_types.h:311:0,
                 from /home/wxm/oh-distro/software/externals/pcl/common/src/point_types.cpp:37:
/home/wxm/oh-distro/software/externals/pcl/common/include/pcl/impl/point_types.hpp: In member function ‘Eigen::Map<Eigen::Matrix<float, 4, 1>, 16> pcl::_PointXYZ::getVector4fMap()’:
/home/wxm/oh-distro/software/externals/pcl/common/include/pcl/impl/point_types.hpp:224:420: error: could not convert ‘Eigen::PlainObjectBase<Derived>::MapAligned(Eigen::PlainObjectBase<Derived>::Scalar*) [with Derived = Eigen::Matrix<float, 4, 1>; Eigen::PlainObjectBase<Derived>::AlignedMapType = Eigen::Map<Eigen::Matrix<float, 4, 1>, 32, Eigen::Stride<0, 0> >; Eigen::PlainObjectBase<Derived>::Scalar = float]()’ from ‘Eigen::PlainObjectBase<Eigen::Matrix<float, 4, 1> >::AlignedMapType {aka Eigen::Map<Eigen::Matrix<float, 4, 1>, 32, Eigen::Stride<0, 0> >}’ to ‘Eigen::Map<Eigen::Matrix<float, 4, 1>, 16>’
     PCL_ADD_POINT4D; // This adds the members x,y,z which can also be accessed using the point (which is float[4])
                                                                                                                                                                                                                                                                                                                                                                                                                                    ^
/home/wxm/oh-distro/software/externals/pcl/common/include/pcl/impl/point_types.hpp: In member function ‘const Eigen::Map<const Eigen::Matrix<float, 4, 1>, 16> pcl::_PointXYZ::getVector4fMap() const’:
/home/wxm/oh-distro/software/externals/pcl/common/include/pcl/impl/point_types.hpp:224:556: error: could not convert ‘Eigen::PlainObjectBase<Derived>::MapAligned(const Scalar*) [with Derived = Eigen::Matrix<float, 4, 1>; Eigen::PlainObjectBase<Derived>::ConstAlignedMapType = const Eigen::Map<const Eigen::Matrix<float, 4, 1>, 32, Eigen::Stride<0, 0> >; Eigen::PlainObjectBase<Derived>::Scalar = float]()’ from ‘Eigen::PlainObjectBase<Eigen::Matrix<float, 4, 1> >::ConstAlignedMapType {aka const Eigen::Map<const Eigen::Matrix<float, 4, 1>, 32, Eigen::Stride<0, 0> >}’ to ‘const Eigen::Map<const Eigen::Matrix<float, 4, 1>, 16>’
     PCL_ADD_POINT4D; // This adds the members x,y,z which can also be accessed using the point (which is float[4])

I can repeat this on 2 machines when building externals using this branch. Nuking pcl, pod-build/src/pcl-*, pod-build/, and running make pcl sometimes resolves the issue

patmarion commented 8 years ago

Perhaps we could try pcl master? I looked at pcl master and it appears point_types.h has not been modified in a year, so it may not have been fixed yet.

Perhaps we can find a fix and patch pcl (and contribute the fix upstream)?

I think it would be best to use a consistent version of eigen if we can, since we do have code that mixes drake and pcl.

gizatt commented 8 years ago

Haven't been able to replicate the build issue over here, even with a completely clean build, but it definitely sounds like an issue if you've seen it on so many machines.

Is there a deep (or potentialy future) issue with just moving to the ppa?

On Thu, Mar 10, 2016 at 1:41 PM, Pat Marion notifications@github.com wrote:

Perhaps we could try pcl master? I looked at pcl master and it appears point_types.h has not been modified in a year, so it may not have been fixed yet.

Perhaps we can find a fix and patch pcl (and contribute the fix upstream)?

I think it would be best to use a consistent version of eigen if we can, since we do have code that mixes drake and pcl.

— Reply to this email directly or view it on GitHub https://github.com/openhumanoids/oh-distro/pull/102#issuecomment-194995895 .

wxmerkt commented 8 years ago

I don't think that there's an issue with moving to the PPA.

Something else, @gizatt have you upgraded to 4.9 for gcc? We've been seeing it with 4.8.4 and I remember some discussion over at drake to move to 4.9. @tkoolen, @rdeits would you recommend to move to gcc 4.9 by default for the new eigen?

tkoolen commented 8 years ago

Yeah, 4.9 is required now. Sorry about that.

patmarion commented 8 years ago

Hmm, not sure about that. My desktop also has gcc 4.8.4, I build drake master without problems. We did clean builds on several machines (atlas onboard and operator machines) with gcc 4.8.4.

@tkoolen why do you say 4.9 is required? for which code?

tkoolen commented 8 years ago

See https://github.com/RobotLocomotion/drake/issues/1649 and https://github.com/RobotLocomotion/drake/issues/1650:

now that #1663 is resolved, let's update the minimum gcc version to 4.9.

wxmerkt commented 8 years ago

It now compiles just fine with gcc 4.8.4 and without any changes to PCL, so please disregard until we see this again. From my point of view this is ready to merge. Thanks everyone!

wxmerkt commented 8 years ago

Actually, @gizatt can you rebase oh/drake with the new RL/drake changes because of https://github.com/RobotLocomotion/drake/issues/1835 - your branch in this PR uses the one that has been purged from RL/drake's history

wxmerkt commented 8 years ago

Also, when starting director from this branch:

oh-distro/software/build/lib/python2.7/dist-packages/director/drakevisualizer.py:19: FutureWarning: The drake python lcmtype bindings have been moved back to the `drake` package. You may want to upgrade your version of drake
  warnings.warn("The drake python lcmtype bindings have been moved back to the `drake` package. You may want to upgrade your version of drake", FutureWarning)
Traceback (most recent call last):
  File "<string>", line 7, in <module>
  File "/home/wxm/oh-distro/software/build/lib/python2.7/dist-packages/director/startup.py", line 49, in <module>
    from director import drakevisualizer
  File "/home/wxm/oh-distro/software/build/lib/python2.7/dist-packages/director/drakevisualizer.py", line 20, in <module>
    import lcmtypes.drake as lcmdrake
  File "/home/wxm/oh-distro/software/build/lib/python2.7/dist-packages/lcmtypes/drake/__init__.py", line 7, in <module>
    from .lcmt_body_motion_data import lcmt_body_motion_data
  File "/home/wxm/oh-distro/software/build/lib/python2.7/dist-packages/lcmtypes/drake/lcmt_body_motion_data.py", line 12, in <module>
    import drake.lcmt_piecewise_polynomial
ImportError: No module named drake.lcmt_piecewise_polynomial

patmarion commented 8 years ago

@iamwolf can you check your build/lib/python2.7/dist-packages?

You should rm the lcmtypes/ directory there, it might be stale. You should have a drake/ directory which is not a symlink. If that's not the case, it might be a problem on this branch, or it might be a problem with stale files in your build tree.

patmarion commented 8 years ago

also just in case, make sure you have all the commits on this topic branch, i added a commit to this branch yesterday.

gizatt commented 8 years ago

I'm make these tweaks and get the Drake history correct, there are a few other submodule refs to improve too.

On Fri, Mar 11, 2016 at 1:24 PM, Pat Marion notifications@github.com wrote:

also just in case, make sure you have all the commits on this topic branch, i added a commit to this branch yesterday.

— Reply to this email directly or view it on GitHub https://github.com/openhumanoids/oh-distro/pull/102#issuecomment-195488050 .

gizatt commented 8 years ago

OK, happy with the changes and submodule refs now if everyone else is. If the test works then probably good to merge?

gizatt commented 8 years ago

Pat and I figured out that director issues were because I forgot to also point to the new director cmake submodule. Ugh. Fixed and maybe that'll make travis happier?

mauricefallon commented 8 years ago

I'll mode this final open PR into private by the end of today, if that's ok.

gizatt commented 8 years ago

That would be great

On Wed, Mar 16, 2016 at 7:47 AM, Maurice Fallon notifications@github.com wrote:

I'll mode this final open PR into private by the end of today, if that's ok.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/openhumanoids/oh-distro/pull/102#issuecomment-197280096

mauricefallon commented 8 years ago

Closing in favour of https://github.com/openhumanoids/oh-distro-private/pull/2304

openhumanoids / oh-distro

Update Drake to RL master; InstQP is MATLAB-free #102