Robot Serialization #316
Robot Serialization #316
Conversation
varunagrawal
force-pushed
the
feature/robot-serialization
branch
from
df304fc to
3dab7bb
Compare
.github/workflows/linux-ci.yml
Outdated
| @@ -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] | |||
There was a problem hiding this comment.
Add a 18.04 target here so we don't lose 18.04
There was a problem hiding this comment.
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.
Help, we lost 18.04 for GTSAM???
There was a problem hiding this comment.
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.
Tell me again why we do not support the default boost version on 1804?
There was a problem hiding this comment.
@ProfFan any updates on this?
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
Ping @ProfFan
gtdynamics/universal_robot/Link.h
Outdated
| 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.
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.
Yeah I should get rid of joint serialization in Link.h
| // 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.
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 😂
There was a problem hiding this comment.
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.
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.
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. 😅
|
Serialization should deal fine with pointers, BTW. Not sure what the exact issue is. |
.github/workflows/linux-ci.yml
Outdated
| @@ -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] | |||
.github/workflows/linux-ci.yml
Outdated
| - 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.
This is so strange. As per commit 1b8707e, that change should have already been made.
This class provides serialization capabilities for the Robot class and all the currently defined links and joints.