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

Save map builder options in options proto. Rename it to BuilderOptions. #1504

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions cartographer/cloud/client/map_builder_stub.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ int MapBuilderStub::AddTrajectoryBuilder(

int MapBuilderStub::AddTrajectoryForDeserialization(
const mapping::proto::TrajectoryBuilderOptionsWithSensorIds&
options_with_sensor_ids_proto) {
trajectory_builder_options_with_sensor_ids_proto) {
LOG(FATAL) << "Not implemented";
}

Expand Down Expand Up @@ -197,12 +197,11 @@ std::map<int, int> MapBuilderStub::LoadState(
request.set_load_frozen_state(load_frozen_state);
CHECK(client.Write(request));
}
// Request with an AllTrajectoryBuilderOptions should be third.
// Request with BuilderOptions should be third.
{
proto::LoadStateRequest request;
*request.mutable_serialized_data()
->mutable_all_trajectory_builder_options() =
deserializer.all_trajectory_builder_options();
*request.mutable_serialized_data()->mutable_builder_options() =
deserializer.builder_options();
request.set_load_frozen_state(load_frozen_state);
CHECK(client.Write(request));
}
Expand Down
2 changes: 1 addition & 1 deletion cartographer/cloud/client/map_builder_stub.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class MapBuilderStub : public mapping::MapBuilderInterface {
LocalSlamResultCallback local_slam_result_callback) override;
int AddTrajectoryForDeserialization(
const mapping::proto::TrajectoryBuilderOptionsWithSensorIds&
options_with_sensor_ids_proto) override;
trajectory_builder_options_with_sensor_ids_proto) override;
mapping::TrajectoryBuilderInterface* GetTrajectoryBuilder(
int trajectory_id) const override;
void FinishTrajectory(int trajectory_id) override;
Expand Down
49 changes: 24 additions & 25 deletions cartographer/cloud/internal/client_server_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ constexpr char kPoseGraphProtoString[] = R"(pose_graph {
submap: {}
}
})";
constexpr char kAllTrajectoryBuilderOptionsProtoString[] =
R"(all_trajectory_builder_options {
options_with_sensor_ids: {}
constexpr char kBuilderOptionsProtoString[] =
R"(builder_options {
trajectory_builder_options_with_sensor_ids: {}
map_builder_options: {}
})";
constexpr char kSubmapProtoString[] = "submap {}";
constexpr char kNodeProtoString[] = "node {}";
Expand Down Expand Up @@ -637,17 +638,16 @@ TEST_P(ClientServerTestByGridType, LoadStateAndDelete) {
InitializeStub();

// Load text proto into in_memory_reader.
auto reader =
ProtoReaderFromStrings(kSerializationHeaderProtoString,
{
kPoseGraphProtoString,
kAllTrajectoryBuilderOptionsProtoString,
kSubmapProtoString,
kNodeProtoString,
kImuDataProtoString,
kOdometryDataProtoString,
kLandmarkDataProtoString,
});
auto reader = ProtoReaderFromStrings(kSerializationHeaderProtoString,
{
kPoseGraphProtoString,
kBuilderOptionsProtoString,
kSubmapProtoString,
kNodeProtoString,
kImuDataProtoString,
kOdometryDataProtoString,
kLandmarkDataProtoString,
});

auto trajectory_remapping = stub_->LoadState(reader.get(), true);
int expected_trajectory_id = 0;
Expand Down Expand Up @@ -676,17 +676,16 @@ TEST_P(ClientServerTestByGridType, LoadUnfrozenStateAndDelete) {
InitializeStub();

// Load text proto into in_memory_reader.
auto reader =
ProtoReaderFromStrings(kSerializationHeaderProtoString,
{
kPoseGraphProtoString,
kAllTrajectoryBuilderOptionsProtoString,
kSubmapProtoString,
kNodeProtoString,
kImuDataProtoString,
kOdometryDataProtoString,
kLandmarkDataProtoString,
});
auto reader = ProtoReaderFromStrings(kSerializationHeaderProtoString,
{
kPoseGraphProtoString,
kBuilderOptionsProtoString,
kSubmapProtoString,
kNodeProtoString,
kImuDataProtoString,
kOdometryDataProtoString,
kLandmarkDataProtoString,
});

auto trajectory_remapping =
stub_->LoadState(reader.get(), false /* load_frozen_state */);
Expand Down
31 changes: 17 additions & 14 deletions cartographer/io/internal/mapping_state_serialization.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,18 @@ using mapping::SubmapId;
using mapping::TrajectoryNode;
using mapping::proto::SerializedData;

mapping::proto::AllTrajectoryBuilderOptions
CreateAllTrajectoryBuilderOptionsProto(
mapping::proto::BuilderOptions CreateBuilderOptionsProto(
const std::vector<mapping::proto::TrajectoryBuilderOptionsWithSensorIds>&
all_options_with_sensor_ids,
all_trajectory_builder_options_with_sensor_ids,
const mapping::proto::MapBuilderOptions& map_builder_options,
const std::vector<int>& trajectory_ids_to_serialize) {
mapping::proto::AllTrajectoryBuilderOptions all_options_proto;
mapping::proto::BuilderOptions builder_options_proto;
for (auto id : trajectory_ids_to_serialize) {
*all_options_proto.add_options_with_sensor_ids() =
all_options_with_sensor_ids[id];
*builder_options_proto.add_trajectory_builder_options_with_sensor_ids() =
all_trajectory_builder_options_with_sensor_ids[id];
}
return all_options_proto;
*builder_options_proto.mutable_map_builder_options() = map_builder_options;
return builder_options_proto;
}

// Will return all trajectory ids, that have `state != DELETED`.
Expand Down Expand Up @@ -67,14 +68,15 @@ SerializedData SerializePoseGraph(const mapping::PoseGraph& pose_graph,
return proto;
}

SerializedData SerializeTrajectoryBuilderOptions(
SerializedData SerializeBuilderOptions(
const std::vector<mapping::proto::TrajectoryBuilderOptionsWithSensorIds>&
trajectory_builder_options,
const mapping::proto::MapBuilderOptions& map_builder_options,
const std::vector<int>& trajectory_ids_to_serialize) {
SerializedData proto;
*proto.mutable_all_trajectory_builder_options() =
CreateAllTrajectoryBuilderOptionsProto(trajectory_builder_options,
trajectory_ids_to_serialize);
*proto.mutable_builder_options() =
CreateBuilderOptionsProto(trajectory_builder_options, map_builder_options,
trajectory_ids_to_serialize);
return proto;
}

Expand Down Expand Up @@ -213,13 +215,14 @@ void SerializeLandmarkNodes(
void WritePbStream(
const mapping::PoseGraph& pose_graph,
const std::vector<mapping::proto::TrajectoryBuilderOptionsWithSensorIds>&
trajectory_builder_options,
all_trajectory_builder_options,
const mapping::proto::MapBuilderOptions& map_builder_options,
ProtoStreamWriterInterface* const writer, bool include_unfinished_submaps) {
writer->WriteProto(CreateHeader());
writer->WriteProto(
SerializePoseGraph(pose_graph, include_unfinished_submaps));
writer->WriteProto(SerializeTrajectoryBuilderOptions(
trajectory_builder_options,
writer->WriteProto(SerializeBuilderOptions(
all_trajectory_builder_options, map_builder_options,
GetValidTrajectoryIds(pose_graph.GetTrajectoryStates())));

SerializeSubmaps(pose_graph.GetAllSubmapData(), include_unfinished_submaps,
Expand Down
3 changes: 2 additions & 1 deletion cartographer/io/internal/mapping_state_serialization.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ static constexpr int kFormatVersionWithoutSubmapHistograms = 1;
void WritePbStream(
const mapping::PoseGraph& pose_graph,
const std::vector<mapping::proto::TrajectoryBuilderOptionsWithSensorIds>&
builder_options,
all_trajectory_builder_options,
const mapping::proto::MapBuilderOptions& map_builder_options,
ProtoStreamWriterInterface* const writer, bool include_unfinished_submaps);

} // namespace io
Expand Down
8 changes: 6 additions & 2 deletions cartographer/io/internal/pbstream_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,14 @@ void Run(const std::string& pbstream_filename, bool all_debug_strings) {
const auto header = deserializer.header();
LOG(INFO) << "Header: " << header.DebugString();
for (const mapping::proto::TrajectoryBuilderOptionsWithSensorIds&
trajectory_options : deserializer.all_trajectory_builder_options()
.options_with_sensor_ids()) {
trajectory_options :
deserializer.builder_options()
.trajectory_builder_options_with_sensor_ids()) {
LOG(INFO) << "Trajectory options: " << trajectory_options.DebugString();
}
LOG(INFO)
<< "Map builder options: "
<< deserializer.builder_options().map_builder_options().DebugString();
const mapping::proto::PoseGraph pose_graph = deserializer.pose_graph();
for (const mapping::proto::Trajectory& trajectory : pose_graph.trajectory()) {
LOG(INFO) << "Trajectory id: " << trajectory.trajectory_id()
Expand Down
14 changes: 7 additions & 7 deletions cartographer/io/proto_stream_deserializer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,17 @@ ProtoStreamDeserializer::ProtoStreamDeserializer(
"`SerializationHeader`, but got field tag "
<< pose_graph_.data_case();

CHECK(ReadNextSerializedData(&all_trajectory_builder_options_))
<< "Serialized stream misses `AllTrajectoryBuilderOptions`.";
CHECK(all_trajectory_builder_options_.has_all_trajectory_builder_options())
CHECK(ReadNextSerializedData(&builder_options_))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this break compatibility?

Copy link
Contributor Author

@ojura ojura Feb 14, 2019

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, true!

<< "Serialized stream misses `BuilderOptions`.";
CHECK(builder_options_.has_builder_options())
<< "Serialized stream order corrupt. Expecting "
"`AllTrajectoryBuilderOptions` after "
"`BuilderOptions` after "
"PoseGraph, got field tag "
<< all_trajectory_builder_options_.data_case();
<< builder_options_.data_case();

CHECK_EQ(pose_graph_.pose_graph().trajectory_size(),
all_trajectory_builder_options_.all_trajectory_builder_options()
.options_with_sensor_ids_size());
builder_options_.builder_options()
.trajectory_builder_options_with_sensor_ids_size());
}

bool ProtoStreamDeserializer::ReadNextSerializedData(
Expand Down
7 changes: 3 additions & 4 deletions cartographer/io/proto_stream_deserializer.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,8 @@ class ProtoStreamDeserializer {
return pose_graph_.pose_graph();
}

const mapping::proto::AllTrajectoryBuilderOptions&
all_trajectory_builder_options() {
return all_trajectory_builder_options_.all_trajectory_builder_options();
const mapping::proto::BuilderOptions& builder_options() {
return builder_options_.builder_options();
}

// Reads the next `SerializedData` message of the ProtoStream into `data`.
Expand All @@ -63,7 +62,7 @@ class ProtoStreamDeserializer {

mapping::proto::SerializationHeader header_;
mapping::proto::SerializedData pose_graph_;
mapping::proto::SerializedData all_trajectory_builder_options_;
mapping::proto::SerializedData builder_options_;
};

} // namespace io
Expand Down
14 changes: 6 additions & 8 deletions cartographer/io/proto_stream_deserializer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ constexpr char kSerializationHeaderProtoString[] = "format_version: 1";
constexpr char kUnsupportedSerializationHeaderProtoString[] =
"format_version: 123";
constexpr char kPoseGraphProtoString[] = "pose_graph {}";
constexpr char kAllTrajectoryBuilderOptionsProtoString[] =
"all_trajectory_builder_options {}";
constexpr char kBuilderOptionsProtoString[] = "builder_options {}";
constexpr char kSubmapProtoString[] = "submap {}";
constexpr char kNodeProtoString[] = "node {}";
constexpr char kTrajectoryDataProtoString[] = "trajectory_data {}";
Expand All @@ -63,7 +62,7 @@ TEST_F(ProtoStreamDeserializerTest, WorksOnGoldenTextStream) {
reader_ = ProtoReaderFromStrings(kSerializationHeaderProtoString,
{
kPoseGraphProtoString,
kAllTrajectoryBuilderOptionsProtoString,
kBuilderOptionsProtoString,
kSubmapProtoString,
kNodeProtoString,
kTrajectoryDataProtoString,
Expand All @@ -84,11 +83,10 @@ TEST_F(ProtoStreamDeserializerTest, WorksOnGoldenTextStream) {
ProtoFromStringOrDie<SerializedData>(kPoseGraphProtoString)
.pose_graph()));

EXPECT_TRUE(
MessageDifferencer::Equals(deserializer.all_trajectory_builder_options(),
ProtoFromStringOrDie<SerializedData>(
kAllTrajectoryBuilderOptionsProtoString)
.all_trajectory_builder_options()));
EXPECT_TRUE(MessageDifferencer::Equals(
deserializer.builder_options(),
ProtoFromStringOrDie<SerializedData>(kBuilderOptionsProtoString)
.builder_options()));

SerializedData serialized_data;
EXPECT_TRUE(deserializer.ReadNextSerializedData(&serialized_data));
Expand Down
10 changes: 4 additions & 6 deletions cartographer/io/serialization_format_migration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,9 @@ bool ReadPoseGraph(cartographer::io::ProtoStreamReaderInterface* const input,
bool ReadBuilderOptions(
cartographer::io::ProtoStreamReaderInterface* const input,
ProtoMap* proto_map) {
auto& options_vec =
(*proto_map)[SerializedData::kAllTrajectoryBuilderOptions];
auto& options_vec = (*proto_map)[SerializedData::kBuilderOptions];
options_vec.emplace_back();
return input->ReadProto(
options_vec.back().mutable_all_trajectory_builder_options());
return input->ReadProto(options_vec.back().mutable_builder_options());
}

mapping::proto::Submap MigrateLegacySubmap2d(
Expand Down Expand Up @@ -246,7 +244,7 @@ ProtoMap ParseLegacyData(
CHECK(ReadBuilderOptions(input, &proto_map))
<< "Input stream seems to differ from original stream format. Could "
"not "
"read AllTrajectoryBuilderOptions as second message.";
"read BuilderOptions as second message.";
if (migrate_grid_format) {
do {
} while (DeserializeNextAndMigrateGridFormat(input, &proto_map));
Expand All @@ -269,7 +267,7 @@ void SerializeToVersion1Format(
cartographer::io::ProtoStreamWriterInterface* const output) {
const std::vector<int> kFieldSerializationOrder = {
SerializedData::kPoseGraphFieldNumber,
SerializedData::kAllTrajectoryBuilderOptionsFieldNumber,
SerializedData::kBuilderOptionsFieldNumber,
SerializedData::kSubmapFieldNumber,
SerializedData::kNodeFieldNumber,
SerializedData::kTrajectoryDataFieldNumber,
Expand Down
2 changes: 1 addition & 1 deletion cartographer/io/serialization_format_migration.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace cartographer {
namespace io {

// This helper function, migrates the input stream, which is supposed to match
// to the "old" stream format order (PoseGraph, AllTrajectoryBuilderOptions,
// to the "old" stream format order (PoseGraph, BuilderOptions,
// SerializedData*) to the version 1 stream format (SerializationHeader,
// SerializedData*).
void MigrateStreamFormatToVersion1(
Expand Down
8 changes: 4 additions & 4 deletions cartographer/io/serialization_format_migration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class MigrationTest : public ::testing::Test {
template <class LegacySerializedDataType>
void AddLegacyDataToReader(InMemoryProtoStreamReader& reader) {
mapping::proto::PoseGraph pose_graph;
mapping::proto::AllTrajectoryBuilderOptions all_options;
mapping::proto::BuilderOptions builder_options;
LegacySerializedDataType submap;
submap.mutable_submap();
LegacySerializedDataType node;
Expand All @@ -61,7 +61,7 @@ class MigrationTest : public ::testing::Test {
landmark_data.mutable_landmark_data();

reader.AddProto(pose_graph);
reader.AddProto(all_options);
reader.AddProto(builder_options);
reader.AddProto(submap);
reader.AddProto(node);
reader.AddProto(imu_data);
Expand Down Expand Up @@ -181,7 +181,7 @@ TEST_F(MigrationTest, SerializedDataOrderIsCorrect) {
}

EXPECT_TRUE(serialized[0].has_pose_graph());
EXPECT_TRUE(serialized[1].has_all_trajectory_builder_options());
EXPECT_TRUE(serialized[1].has_builder_options());
EXPECT_TRUE(serialized[2].has_submap());
EXPECT_TRUE(serialized[3].has_node());
EXPECT_TRUE(serialized[4].has_trajectory_data());
Expand All @@ -206,7 +206,7 @@ TEST_F(MigrationTest, SerializedDataOrderAfterGridMigrationIsCorrect) {
}

EXPECT_TRUE(serialized[0].has_pose_graph());
EXPECT_TRUE(serialized[1].has_all_trajectory_builder_options());
EXPECT_TRUE(serialized[1].has_builder_options());
EXPECT_TRUE(serialized[2].has_submap());
EXPECT_TRUE(serialized[3].has_node());
EXPECT_TRUE(serialized[4].has_trajectory_data());
Expand Down
29 changes: 14 additions & 15 deletions cartographer/mapping/internal/testing/mock_map_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,30 @@ class MockMapBuilder : public mapping::MapBuilderInterface {
public:
MOCK_METHOD3(
AddTrajectoryBuilder,
int(const std::set<SensorId> &expected_sensor_ids,
const mapping::proto::TrajectoryBuilderOptions &trajectory_options,
int(const std::set<SensorId>& expected_sensor_ids,
const mapping::proto::TrajectoryBuilderOptions& trajectory_options,
mapping::MapBuilderInterface::LocalSlamResultCallback
local_slam_result_callback));
MOCK_METHOD1(AddTrajectoryForDeserialization,
int(const mapping::proto::TrajectoryBuilderOptionsWithSensorIds
&options_with_sensor_ids_proto));
int(const mapping::proto::TrajectoryBuilderOptionsWithSensorIds&
trajectory_builder_options_with_sensor_ids_proto));
MOCK_CONST_METHOD1(GetTrajectoryBuilder,
mapping::TrajectoryBuilderInterface *(int trajectory_id));
mapping::TrajectoryBuilderInterface*(int trajectory_id));
MOCK_METHOD1(FinishTrajectory, void(int trajectory_id));
MOCK_METHOD2(SubmapToProto,
std::string(const mapping::SubmapId &,
mapping::proto::SubmapQuery::Response *));
MOCK_METHOD2(SerializeState, void(bool, io::ProtoStreamWriterInterface *));
MOCK_METHOD2(SerializeStateToFile, bool(bool, const std::string &));
std::string(const mapping::SubmapId&,
mapping::proto::SubmapQuery::Response*));
MOCK_METHOD2(SerializeState, void(bool, io::ProtoStreamWriterInterface*));
MOCK_METHOD2(SerializeStateToFile, bool(bool, const std::string&));
MOCK_METHOD2(LoadState,
std::map<int, int>(io::ProtoStreamReaderInterface *, bool));
MOCK_METHOD2(LoadStateFromFile,
std::map<int, int>(const std::string &, bool));
std::map<int, int>(io::ProtoStreamReaderInterface*, bool));
MOCK_METHOD2(LoadStateFromFile, std::map<int, int>(const std::string&, bool));
MOCK_CONST_METHOD0(num_trajectory_builders, int());
MOCK_METHOD0(pose_graph, mapping::PoseGraphInterface *());
MOCK_METHOD0(pose_graph, mapping::PoseGraphInterface*());
MOCK_CONST_METHOD0(
GetAllTrajectoryBuilderOptions,
const std::vector<mapping::proto::TrajectoryBuilderOptionsWithSensorIds>
&());
const std::vector<
mapping::proto::TrajectoryBuilderOptionsWithSensorIds>&());
};

} // namespace testing
Expand Down
Loading