Improved handling of multiple trajectories and bags in the offline node #4

ojura · 2017-11-09T11:42:18Z

wohe · 2017-11-09T13:04:08Z

text/0000-offline-node-improved-trajectories-bags.md

+Note that some combinations are invalid, such as __A3__ (a single robot can't be on multiple concurrent trajectories), and __B/C 1__ (there can't be a single trajectory if there are multiple robots).
+Furthermore, __A/B 2 a/b__ would require us to be able to specify when a trajectory stops and begins within the same bag aggregate and on the same range data topics, which would be too complicated and out of scope.
+
+The aim of the RFC is to enable the user to use the offline node for __C2/3__ like in the online node, to preserve the ability for __A/B 2c__, and to possibly offer additional functionality: for example, __b__, and __B3c__, which is otherwise impossible with the online node.


What I'm missing here is a true motivation, i.e. why are you interested in seeing trajectories being built at the same time in the offline node. Especially given that it has an unclear effect on the end result (all data will be processed whether in parallel or sequential) and it does not change the computational effort. For the main use case of the offline node right now, i.e. process data as quickly as possible and output the result, this seems irrelevant.

You are right about this part - being able to watch concurrent trajectories being built at the same time does not influence the end result in any way besides potentially increasing the coolness factor. However, it's possible to do it with the online node. Why not have the ability to do the same thing in the offline node, at increased processing speed? I believe that the offline node should offer everythihg that the online node can do, but at greater speed.

However, if I am correct, the offline node does not currently support concurrent trajectories at all. Or different robots.

The true motivation is stated at the beginning - to make the offline node more flexible and possible to be used in these cases. Someone who has bagged a swarm of robots and wants to try out cooperative SLAM with Cartographer can also be interested in crunching the bags as fast as possible. Perhaps they have a bag for each robot, perhaps they have all robots in a single bag - I think we can handle all these cases without much problems if we use Configuration #2 or #3.

I guess what you explained now is what I was missing. Add these use cases (concurrent trajectories being built for additional coolness, different robots) at the very beginning. This seems to be the motivation to me. The other discussion is already the way how to get there.

cschuet · 2017-11-10T13:27:35Z

text/0000-offline-node-improved-trajectories-bags.md

+
+If we suppose that there is a mandated 1:1 relationship between bags and trajectories in the offline node, the following configuration can be proposed.
+
+### Configuration #1


I would advise to keep the design and scope limited to what is absolutely necessary to solve your usecase. Sure it would be nice to have the additional functionality of options 2 and 3, but with some bag surgery it seems you can reduce both of these to option 1. I would suggest to move ahead with that.

+1. All functionality needs to be taught somehow. Cartographer is already a complex system, so adding more features will make it even harder to understand. Multi trajectory is not a self evident concept in general, so making it as simple to understand as possible is important.

My suggestion: Move this explanation to discussion points, explain why handling one trajectory per bag is sufficient with some bag filtering and that we limited the scope to this, then simplify the rest of the rfc to only deal with this use case.

cschuet · 2017-11-10T13:28:10Z

text/0000-offline-node-improved-trajectories-bags.md

+__For each bag (trajectory)__, the configuration is a __tuple__ consisting of:
+  - `TrajectoryOptions` (given in a `.lua` file)
+  - A set of sensor data topics
+  - Boolean `use_bag_transforms` which allows us to ignore transforms in the bag


How would this work with multiple bags setting use_bag_transforms to true? Couldn't the frame names clash and how do you want to deal with that?

Each bag (or BagAggregate in option 3) would have its own transform buffer, as specified in the RFC. This would enable you to track multiple concurrent robots with same frame names.

SirVer · 2017-11-14T12:42:41Z

text/0000-offline-node-improved-trajectories-bags.md

+
+For example, a user has bagged a swarm of robots and wants to try out cooperative SLAM with Cartographer.
+They are interested in crunching the bags as fast as possible.
+Perhaps they have a bag for each robot, perhaps they have all robots in a single bag - the offline node should have a flexible configuration interface to allow for various possibilities.


I disagree with this still. Why should we support one bag - multiple robots? That is just an rosbag filter away from a sane input to cartographer_ros. It also deals better with the problem of how to replay TF messages in the bag - if you have one per robot, they are namespaced to the robot.

SirVer · 2017-11-14T12:44:18Z

text/0000-offline-node-improved-trajectories-bags.md

+Perhaps they have a bag for each robot, perhaps they have all robots in a single bag - the offline node should have a flexible configuration interface to allow for various possibilities.
+
+The concurrent trajectories could be processed either sequentially or in parallel - it should not influence the end result in any way, since local SLAM is not influenced by other trajectories, and global SLAM should arrive at the same pose graph at the end, no matter what the order of insertion is.
+However, processing and displaying the concurrent trajectories in parallel, like the online node can do, would be more useful and increase the coolness factor.


This could also be handled by a rosbag filter or a short python script. Essentially translate each header.stamp in the bag to a common start time for multiple bags. We could add a script into cartographer_ros to do this. No need for the offline node to support that I think.

To clarify: that means that the offline node could just merge sort from multiple bags: always process the newest message and push it through the pipeline. It would still work exactly the same in the current use case (just relaxing that the bags must be provided in the same time order), but would automagically support playing bags with overlapping timestamps.

To clarify: that means that the offline node could just merge sort from multiple bags: always process the newest message and push it through the pipeline. It would still work exactly the same in the current use case (just relaxing that the bags must be provided in the same time order), but would automagically support playing bags with overlapping timestamps.

@SirVer That's right! So there is no need to remap time in the bags, right?

I agree with you on the rest (user can split one bag into multiple bags, or join multiple bags into one, so that in the end, the data obeys 1 trajectory : 1 bag format). I think you misunderstood the text in the RFC: the user should never remap time in the bags. The offline node should accept both simultaneous and sequential trajectories/bags.

@wohe suggested that we could process simultaneous trajectories sequentially, to which I responded that it would be better if we processed them truly simultaneously like you described, by merge sorting them (I called this collating, was that wrong? Better change it to merge-sort.).

process the newest message and push it through the pipeline

You mean the oldest? :-)

@SirVer That's right! So there is no need to remap time in the bags, right?

Only if you want to have the bags from one robot (i.e. the deutsches museum runs) looking like they were recorded at the same time. You know, for the coolness factor.

I called this collating, was that wrong? Better change it to merge-sort.).

I think collating is correct as well - merge sorting is the means to get collated data here.

You mean the oldest? :-)

yes, sure.

Will you update the RFC with these new information?

SirVer · 2017-11-14T12:49:30Z

text/0000-offline-node-improved-trajectories-bags.md

+### Overview of possible dataset scenarios
+Let's give an overview of all possibilities how a dataset layout and content could look like:
+
+| Robot plurality                                                         | Trajectory plurality                                 | Bag plurality                                                          |


I think the goal here is to support N bags, from N robots, with N configurations. Everything else is a subset of this. We should not care if the data overlaps or not - as said, that can be changed rewriting the bags data.

SirVer · 2017-11-14T12:52:32Z

text/0000-offline-node-improved-trajectories-bags.md

+
+For both Configuration #1 and Configuration #2, all concurrent trajectories (whether in the same bag as permitted by Configuration #2, or in different bags) are __collated__.
+
+### Handling split bags


Joining bags can be done by a script too. Let's not rebuild features that ROS tools already give us. I am fine with adding some scripts (to be outlined here) to cartographer_ros that help with joining and reshuffling timestamps for bags. This will also allow to drop the concept of a BagAggregate - which seems an unnecessary complexity in this RFC.

SirVer · 2017-11-14T12:54:30Z

text/0000-offline-node-improved-trajectories-bags.md

+As before, any concurrent trajectories are collated.
+Each `BagAggregate` has its own transform buffer (which is shared by all bags in the aggregate).
+
+### Data format of the configuration


This section is most interesting to me - but it needs to be written. For example would we replace the current command line options or add a new one that reads a lua file as input? How would such a Lua file look?

#helpneeded :D I am currently a bit short on time, but I'll get back to this RFC when I can.

ojura · 2017-12-26T15:10:18Z

Hello everyone, and happy holidays! @SirVer I finally got around to making some progress on this.

The implementation which follows the proposed configuration #1 in the rfc has been implemented in this branch: cartographer-project/cartographer_ros@master...larics:simultaneous-bags (PR cartographer-project/cartographer_ros#636)

I have expanded the comma-separated list approach, as already used for bag_filenames, to configuration_basenames, urdf_filenames, and there is now sensor_topics as well.

Here is an example of launching the offline node for two robots named alpha and delta, which were driven simultaneously:

rosrun cartographer_ros cartographer_offline_node
-configuration_directory /somewhere
-configuration_basenames alpha.lua,delta.lua -urdf_filenames alpha.urdf,delta.urdf
-bag_filenames alpha.bag,delta.bag -sensor_topics alpha/scan:alpha/pose,delta/scan:delta/pose

Note how sensor topics for different trajectories are separated by commas, and further separated with colons. Leaving sensor topics unspecified causes the default computed topic names to be used.

If only one configuration basename or set of topics is specified, it will be used for all trajectories.

ojura · 2017-12-27T18:46:36Z

I have just realized that multiple trajectories in single bags are supported by this as well. This can be achieved by simply specifying the same bag multiple times, but with different sets of sensor topics.

cschuet · 2018-01-03T14:30:03Z

@ojura Thanks for the PR. I looked at it and the implemented features make sense to me. Be aware though that we will introduce a gRPC-ified version of the cartographer_offline_node in next days and your changes should also be added there. In essence this will mean moving most of your (then shared) code out of offline_node_main.cc into some other place so that both offline_node_main.cc and offline_node_grpc_main.cc can use it.

But before that, could you update the RFC with the most recent approach and move all other options into a "Alternatives considered" section or something like this? We should get the RFC approved first before discussing implementation in detail.

ojura · 2018-01-03T18:56:16Z

Okay, I updated the rfc to be more in-line with the discussion and the PR.

cschuet · 2018-01-04T12:51:06Z

@ojura Thanks! Can you address open comments by @SirVer which is asking to add certain information into this RFC? Otherwise this RFC LGTM.

@SirVer @wohe Can you take a look at this RFC again? Are you ok with moving forward on this? Tentative implementation is in cartographer-project/cartographer_ros#636

ojura · 2018-01-05T14:23:42Z

@cschuet No problem. I think I covered @SirVer's request for adding data format description in the section Data format of the configuration. Do you think something is missing?

SirVer · 2018-01-05T14:33:16Z

@ojura @cschuet I am fine with moving forward.

cschuet · 2018-01-15T13:04:24Z

@wohe @SirVer Can you formally approve this. Then I will assign an RFC ID and merge it.

…e-node-improved-trajectories-bags.md

cschuet · 2018-01-17T19:50:58Z

Assigned RFC=0009

RFC=[0009](cartographer-project/rfcs#4)

RFC=[0009](cartographer-project/rfcs#4) Original commit: cartographer-project/cartographer_ros@d65f3c0

ojura mentioned this pull request Nov 9, 2017

Simultaneous trajectories in offline node cartographer-project/cartographer_ros#585

Closed

Add RFC text.

0fd32b6

ojura force-pushed the master branch from 20d4710 to 0fd32b6 Compare November 9, 2017 12:52

wohe reviewed Nov 9, 2017

View reviewed changes

ojura added 3 commits November 9, 2017 16:08

Update 0000-offline-node-improved-trajectories-bags.md

350d5fd

Update 0000-offline-node-improved-trajectories-bags.md

22ee1f9

Newlines.

6880769

cschuet reviewed Nov 10, 2017

View reviewed changes

SirVer reviewed Nov 14, 2017

View reviewed changes

ojura mentioned this pull request Dec 26, 2017

Simultaneous offline multi trajectories cartographer-project/cartographer_ros#636

Merged

ojura added 2 commits January 3, 2018 18:20

Update 0000-offline-node-improved-trajectories-bags.md

d7c8817

Update 0000-offline-node-improved-trajectories-bags.md

94d5d84

Merge branch 'master' into master

daaaeb8

cschuet approved these changes Jan 15, 2018

View reviewed changes

SirVer approved these changes Jan 15, 2018

View reviewed changes

Rename 0000-offline-node-improved-trajectories-bags.md to 0009-offlin…

aa44390

…e-node-improved-trajectories-bags.md

cschuet merged commit b530495 into cartographer-project:master Jan 17, 2018

wally-the-cartographer pushed a commit to cartographer-project/cartographer_ros that referenced this pull request Jan 24, 2018

Simultaneous offline multi trajectories (#636)

d65f3c0

RFC=[0009](cartographer-project/rfcs#4)

ojura added a commit to larics/cartographer_combined that referenced this pull request Jan 24, 2018

[cartographer_ros] Simultaneous offline multi trajectories (#636)

b17b3ca

RFC=[0009](cartographer-project/rfcs#4) Original commit: cartographer-project/cartographer_ros@d65f3c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved handling of multiple trajectories and bags in the offline node #4

Improved handling of multiple trajectories and bags in the offline node #4

ojura commented Nov 9, 2017 •

edited

Loading

wohe Nov 9, 2017

ojura Nov 9, 2017 •

edited

Loading

wohe Nov 9, 2017

cschuet Nov 10, 2017

SirVer Nov 10, 2017

cschuet Nov 10, 2017

ojura Nov 10, 2017 •

edited

Loading

SirVer Nov 14, 2017

SirVer Nov 14, 2017

SirVer Nov 14, 2017

ojura Nov 15, 2017 •

edited

Loading

SirVer Nov 16, 2017

SirVer Nov 14, 2017

SirVer Nov 14, 2017

SirVer Nov 14, 2017

ojura Nov 15, 2017

ojura commented Dec 26, 2017 •

edited

Loading

ojura commented Dec 27, 2017 •

edited

Loading

cschuet commented Jan 3, 2018

ojura commented Jan 3, 2018

cschuet commented Jan 4, 2018

ojura commented Jan 5, 2018

SirVer commented Jan 5, 2018 •

edited

Loading

cschuet commented Jan 15, 2018

cschuet commented Jan 17, 2018


		If we suppose that there is a mandated 1:1 relationship between bags and trajectories in the offline node, the following configuration can be proposed.

		### Configuration #1


		For both Configuration #1 and Configuration #2, all concurrent trajectories (whether in the same bag as permitted by Configuration #2, or in different bags) are __collated__.

		### Handling split bags

Improved handling of multiple trajectories and bags in the offline node #4

Improved handling of multiple trajectories and bags in the offline node #4

Conversation

ojura commented Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

ojura Nov 9, 2017 • edited Loading

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

ojura Nov 10, 2017 • edited Loading

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

ojura Nov 15, 2017 • edited Loading

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

Choose a reason for hiding this comment

ojura commented Dec 26, 2017 • edited Loading

ojura commented Dec 27, 2017 • edited Loading

cschuet commented Jan 3, 2018

ojura commented Jan 3, 2018

cschuet commented Jan 4, 2018

ojura commented Jan 5, 2018

SirVer commented Jan 5, 2018 • edited Loading

cschuet commented Jan 15, 2018

cschuet commented Jan 17, 2018

ojura commented Nov 9, 2017 •

edited

Loading

ojura Nov 9, 2017 •

edited

Loading

ojura Nov 10, 2017 •

edited

Loading

ojura Nov 15, 2017 •

edited

Loading

ojura commented Dec 26, 2017 •

edited

Loading

ojura commented Dec 27, 2017 •

edited

Loading

SirVer commented Jan 5, 2018 •

edited

Loading