-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Save map builder options in options proto. Rename it to BuilderOptions. #1504
base: master
Are you sure you want to change the base?
Conversation
I verified that the pbstream info tool correctly prints out the map builder options for .pbstreams produced with this version of Cartographer. For older pbstreams it just prints out an empty string for map builder options. |
CHECK(ReadNextSerializedData(&all_trajectory_builder_options_)) | ||
<< "Serialized stream misses `AllTrajectoryBuilderOptions`."; | ||
CHECK(all_trajectory_builder_options_.has_all_trajectory_builder_options()) | ||
CHECK(ReadNextSerializedData(&builder_options_)) |
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.
Does this break compatibility?
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.
It is the same proto, just renamed to builder options (instead of all trajectory builder options), since it now contains the map builder options as well.
Also I mentioned above that I tested it with bags built by upstream Cartographer, it works okay (prints a blank line for nonexisting map builder options).
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.
Oh yes, true!
@ojura this was reviewed and approved a long time ago but somehow not merged. It is now outdated and has many conflicts. Do you still have time to resolve the conflict? |
RFC=cartographer-project/rfcs#38