Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Robot Serialization #316

Merged
merged 11 commits into from
Dec 20, 2021
10 changes: 5 additions & 5 deletions .github/workflows/linux-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,17 @@ jobs:
matrix:
# Github Actions requires a single row to be added to the build matrix.
# See https://help.github.com/en/articles/workflow-syntax-for-github-actions.
name: [ubuntu-18.04-gcc-9, ubuntu-18.04-clang-10]
name: [ubuntu-20.04-gcc-9, ubuntu-20.04-clang-10]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a 18.04 target here so we don't lose 18.04

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18.04 has Boost 1.65 and not the minimum 1.67 that GTSAM supports. As such, that serialization bug that we had about deepcopies rears its head again with 18.04.

We'll have to do a custom install of Boost if you want 18.04 which will significantly increase CI time. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help, we lost 18.04 for GTSAM???

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. We do a custom Boost install for 1.67 in GTSAM. That adds a good 5 minutes to the CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me again why we do not support the default boost version on 1804?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ProfFan any updates on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have a resolution for this: I am just setting Ubuntu 20.04 for the clang CI. That way we are still supporting Boost 1.65 (at least for GCC).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @ProfFan

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good, but


build_type: [Debug, Release]
include:
- name: ubuntu-18.04-gcc-9
os: ubuntu-18.04
- name: ubuntu-20.04-gcc-9
os: ubuntu-20.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They need to be updated^

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so strange. As per commit 1b8707e, that change should have already been made.

compiler: gcc
version: "9"

- name: ubuntu-18.04-clang-10
os: ubuntu-18.04
- name: ubuntu-20.04-clang-10
os: ubuntu-20.04
compiler: clang
version: "10"

Expand Down
6 changes: 6 additions & 0 deletions gtdynamics.i
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,12 @@ class Robot {
gtsam::Values forwardKinematics(
const gtsam::Values &known_values, size_t t,
const boost::optional<string> &prior_link_name) const;

// enabling serialization functionality
void serialize() const;

// enable pickling in python
void pickle() const;
};

#include <gtdynamics/universal_robot/sdf.h>
Expand Down
4 changes: 2 additions & 2 deletions gtdynamics/statics/StaticsSlice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* -------------------------------------------------------------------------- */

/**
* @file testStaticsSlice.cpp
* @brief Test Statics in single time slice.
* @file StaticsSlice.cpp
* @brief Statics in single time slice.
* @author: Frank Dellaert
*/

Expand Down
33 changes: 29 additions & 4 deletions gtdynamics/universal_robot/HelicalJoint.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ class HelicalJoint : public Joint {
}

public:
/// Constructor for serialization
HelicalJoint() {}

/**
* @brief Create HelicalJoint using JointParams, joint name, joint pose in
* world frame, screw axes, and parent and child links.
Expand All @@ -53,17 +56,39 @@ class HelicalJoint : public Joint {
* @param[in] parameters JointParams struct.
*/
HelicalJoint(uint8_t id, const std::string &name, const gtsam::Pose3 &bTj,
const LinkSharedPtr &parent_link, const LinkSharedPtr &child_link,
const gtsam::Vector3 &axis, double thread_pitch,
const JointParams &parameters = JointParams())
const LinkSharedPtr &parent_link,
const LinkSharedPtr &child_link, const gtsam::Vector3 &axis,
double thread_pitch,
const JointParams &parameters = JointParams())
: Joint(id, name, bTj, parent_link, child_link,
getScrewAxis(axis, thread_pitch), parameters) {}

/// Constructor directly from screwAxis
using Joint::Joint;

/// Return joint type for use in reconstructing robot from JointParams.
Type type() const final override { return Type::Screw; }

private:
/// @name Advanced Interface
/// @{

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_BASE_OBJECT_NVP(Joint);
}

/// @}
};

} // namespace gtdynamics

namespace gtsam {

template <>
struct traits<gtdynamics::HelicalJoint>
: public Testable<gtdynamics::HelicalJoint> {};

} // namespace gtsam
49 changes: 47 additions & 2 deletions gtdynamics/universal_robot/Joint.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
#include <gtsam/geometry/Pose3.h>
#include <gtsam/linear/GaussianFactorGraph.h>
#include <gtsam/nonlinear/Expression.h>
#include <gtsam/nonlinear/expressions.h>
#include <gtsam/nonlinear/NonlinearFactorGraph.h>
#include <gtsam/nonlinear/Values.h>
#include <gtsam/nonlinear/expressions.h>

#include <boost/enable_shared_from_this.hpp>
#include <boost/shared_ptr.hpp>
Expand Down Expand Up @@ -80,6 +80,20 @@ struct JointParams {

/// Constructor
JointParams() {}

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_NVP(velocity_limit);
ar &BOOST_SERIALIZATION_NVP(velocity_limit_threshold);
ar &BOOST_SERIALIZATION_NVP(acceleration_limit);
ar &BOOST_SERIALIZATION_NVP(acceleration_limit_threshold);
ar &BOOST_SERIALIZATION_NVP(torque_limit);
ar &BOOST_SERIALIZATION_NVP(torque_limit_threshold);
ar &BOOST_SERIALIZATION_NVP(damping_coefficient);
ar &BOOST_SERIALIZATION_NVP(spring_coefficient);
}
};

/// Joint is the base class for a joint connecting two Link objects.
Expand Down Expand Up @@ -213,6 +227,10 @@ class Joint : public boost::enable_shared_from_this<Joint> {

bool operator!=(const Joint &other) const { return !(*this == other); }

bool equals(const Joint &other, double tol = 0) const {
return *this == other;
}

friend std::ostream &operator<<(std::ostream &stream, const Joint &j);

friend std::ostream &operator<<(std::ostream &stream,
Expand Down Expand Up @@ -408,8 +426,35 @@ class Joint : public boost::enable_shared_from_this<Joint> {
* @brief Create expression that constraint the relation between
* wrench and torque on each link.
*/
gtsam::Double_ torqueConstraint(uint64_t t=0) const;
gtsam::Double_ torqueConstraint(uint64_t t = 0) const;

private:
/// @name Advanced Interface
/// @{

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_NVP(name_);
ar &BOOST_SERIALIZATION_NVP(id_);
ar &BOOST_SERIALIZATION_NVP(jMp_);
ar &BOOST_SERIALIZATION_NVP(jMc_);
ar &BOOST_SERIALIZATION_NVP(parent_link_);
ar &BOOST_SERIALIZATION_NVP(child_link_);
ar &BOOST_SERIALIZATION_NVP(pScrewAxis_);
ar &BOOST_SERIALIZATION_NVP(cScrewAxis_);
ar &BOOST_SERIALIZATION_NVP(parameters_);
}

/// @}
};

} // namespace gtdynamics

namespace gtsam {

template <>
struct traits<gtdynamics::Joint> : public Testable<gtdynamics::Joint> {};

} // namespace gtsam
32 changes: 32 additions & 0 deletions gtdynamics/universal_robot/Link.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ class Link : public boost::enable_shared_from_this<Link> {

bool operator!=(const Link &other) const { return !(*this == other); }

bool equals(const Link &other, double tol = 0) const {
return *this == other;
}

/// return a shared pointer of the link
LinkSharedPtr shared(void) { return shared_from_this(); }

Expand Down Expand Up @@ -206,6 +210,34 @@ class Link : public boost::enable_shared_from_this<Link> {

/// Unfix the link
void unfix() { is_fixed_ = false; }

/// @name Advanced Interface
/// @{

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_NVP(id_);
ar &BOOST_SERIALIZATION_NVP(name_);
ar &BOOST_SERIALIZATION_NVP(mass_);
ar &BOOST_SERIALIZATION_NVP(centerOfMass_);
ar &BOOST_SERIALIZATION_NVP(inertia_);
ar &BOOST_SERIALIZATION_NVP(bMcom_);
ar &BOOST_SERIALIZATION_NVP(bMlink_);
ar &BOOST_SERIALIZATION_NVP(is_fixed_);
ar &BOOST_SERIALIZATION_NVP(fixed_pose_);
ar &BOOST_SERIALIZATION_NVP(joints_);
Copy link
Member

@gchenfc gchenfc Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checking, will everything work ok if Joint is serializing Link and Link is serializing Joint?

Like they're serializing eachother?

Actually this brings up another question (not for this PR but just puzzling): how does Joint/Link even work? Won't our code have circular pointer counts so Joints/Links will never get cleaned up? e.g. if a link and joint hold shared_ptr's to eachother, then even if their "owning" robot drops its pointer then both their pointer counts will still be 1 and they won't get freed right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I should get rid of joint serialization in Link.h

}

/// @}
};

} // namespace gtdynamics

namespace gtsam {

template <>
struct traits<gtdynamics::Link> : public Testable<gtdynamics::Link> {};

} // namespace gtsam
24 changes: 24 additions & 0 deletions gtdynamics/universal_robot/PrismaticJoint.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class PrismaticJoint : public Joint {
}

public:
/// Constructor for serialization
PrismaticJoint() {}

/**
* @brief Create PrismaticJoint using JointParams, joint name, joint pose in
* world frame, screw axes, and parent and child links.
Expand All @@ -58,6 +61,27 @@ class PrismaticJoint : public Joint {

/// Return joint type for use in reconstructing robot from JointParams.
Type type() const final override { return Type::Prismatic; }

private:
/// @name Advanced Interface
/// @{

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_BASE_OBJECT_NVP(Joint);
}

/// @}
};

} // namespace gtdynamics

namespace gtsam {

template <>
struct traits<gtdynamics::PrismaticJoint>
: public Testable<gtdynamics::PrismaticJoint> {};

} // namespace gtsam
24 changes: 24 additions & 0 deletions gtdynamics/universal_robot/RevoluteJoint.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ class RevoluteJoint : public Joint {
}

public:
/// Constructor for serialization
RevoluteJoint() {}

/**
* @brief Create RevoluteJoint using JointParams, joint name, joint pose in
* world frame, screw axes, and parent and child links.
Expand All @@ -57,6 +60,27 @@ class RevoluteJoint : public Joint {

/// Return joint type for use in reconstructing robot from JointParams.
Type type() const final override { return Type::Revolute; }

private:
/// @name Advanced Interface
/// @{

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_BASE_OBJECT_NVP(Joint);
}

/// @}
};

} // namespace gtdynamics

namespace gtsam {

template <>
struct traits<gtdynamics::RevoluteJoint>
: public Testable<gtdynamics::RevoluteJoint> {};

} // namespace gtsam
46 changes: 46 additions & 0 deletions gtdynamics/universal_robot/Robot.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,32 @@ class Robot {
/// Print links and joints of the robot, for debug purposes
void print(const std::string &s = "") const;

/// Overload equality operator.
bool operator==(const Robot &other) const {
// Define comparators for easy std::map equality checking
auto link_comparator = [](decltype(*this->name_to_link_.begin()) a,
decltype(a) b) {
// compare the key name and the underlying shared_ptr object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you _emph_ or make a note in the comment "Define comparators ..." or something that clarifies that the only reason this is needed is for the shared_ptr ? Spent a couple minutes trying to figure out why you didn't just use the default map comparator 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not a fan of this shared pointer implementation of Links and Joints. Other than efficiency (which can also be achieved with references), was there any other reason for choosing to use them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

references won't work for polymorphic types, e.g. RevoluteJoint vs ScrewJoint vs PrismaticJoint

specifically, to hold a container of polymorphic types you need pointers (unless use fancy std::variant stuff but I don't think that suits our use case - inherently we need runtime polymorphism to hold all the joints in one data structure)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I figured we could just pass references to the Joint class since the public API for each joint type is the same. I am not going to tackle this anytime soon so we can actually drop it. 😅

return a.first == b.first && (*a.second) == (*b.second);
};
auto joint_comparator = [](decltype(*this->name_to_joint_.begin()) a,
decltype(a) b) {
// compare the key name and the underlying shared_ptr object
return a.first == b.first && (*a.second) == (*b.second);
};

return (this->name_to_link_.size() == other.name_to_link_.size() &&
std::equal(this->name_to_link_.begin(), this->name_to_link_.end(),
other.name_to_link_.begin(), link_comparator) &&
this->name_to_joint_.size() == other.name_to_joint_.size() &&
std::equal(this->name_to_joint_.begin(), this->name_to_joint_.end(),
other.name_to_joint_.begin(), joint_comparator));
}

bool equals(const Robot &other, double tol = 0) const {
return *this == other;
}

/**
* Calculate forward kinematics by performing BFS in the link-joint graph
* (will throw an error when invalid joint angle specification detected).
Expand All @@ -136,5 +162,25 @@ class Robot {
LinkSharedPtr findRootLink(
const gtsam::Values &values,
const boost::optional<std::string> &prior_link_name) const;

/// @name Advanced Interface
/// @{

/** Serialization function */
friend class boost::serialization::access;
template <class ARCHIVE>
void serialize(ARCHIVE &ar, const unsigned int /*version*/) {
ar &BOOST_SERIALIZATION_NVP(name_to_link_);
ar &BOOST_SERIALIZATION_NVP(name_to_joint_);
}

/// @}
};
} // namespace gtdynamics

namespace gtsam {

template <>
struct traits<gtdynamics::Robot> : public Testable<gtdynamics::Robot> {};

} // namespace gtsam
Loading