-
Notifications
You must be signed in to change notification settings - Fork 11
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
Robot Serialization #316
Changes from 7 commits
73b7772
b85bb93
710e2b6
22a1dbb
3dab7bb
d3e58e9
da13f7f
8be7e90
1b8707e
fb36a65
c535417
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They need to be updated^ There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); } | ||
|
||
|
@@ -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_); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
gchenfc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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). | ||
|
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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???
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI borglab/gtsam#590 (comment)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @ProfFan
There was a problem hiding this comment.
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