Builds up RoadGeometry out of parsed information. #40
Conversation
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
|
As this is a proposal the code might change, however, the essence will remain, for so, I'd like a general review from you @agalbachicar on this. The bottom line here is to move up to maliput_sparse all that is not backend dependent and that could easier the life to the backend's creators. maliput/maliput_osm#30 already shows how the implementation can be reduced to mainly just a parser for the underlying format and fill up some segment structs. CC: @stonier |
| /// Gets the segments parsed from the input description. | ||
| const std::unordered_map<Segment::Id, Segment>& GetSegments() const {return DoGetSegments();} |
There was a problem hiding this comment.
I think it is a bit premature to move things here. This is an example of that. We would likely need also the Junction information and we are not even configuring other RoadNetwork entities (e.g. Intersections, Phases, etc.). The Parser should evolve to to provide all of them most likely.
I would refrain adding this class for the moment.
| #include "maliput_sparse/geometry/line_string.h" | ||
|
|
||
| namespace maliput_sparse { | ||
| namespace parser { |
There was a problem hiding this comment.
Why change it to parser from loader?
There was a problem hiding this comment.
It was under maliput_osm/osm.
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
|
Now that junctions are populated and the rest of the entities are also added to the road network I moved forwarded and continued this work. This PR pairs with maliput/maliput_osm#30 @agalbachicar PTAL |
|
Once approved, I will fill up an issue for adding tests for the road network loader here proposed. |
| /// Create a BuilderConfiguration from a string dictionary. | ||
| static BuilderConfiguration FromMap(const std::map<std::string, std::string>& config); | ||
|
|
||
| /// @details The keys of the map are listed at @ref builder_configuration_keys. | ||
| /// @returns A string-string map containing the builder configuration. | ||
| std::map<std::string, std::string> ToStringMap() const; |
There was a problem hiding this comment.
I think that we should make these free functions to keep the style of other repositories.
Consider deleting constructors if you want to enforce a specific way of contring from a map this struct.
There was a problem hiding this comment.
Sorry, I am not seeing the full picture here, why make them free functions?
There was a problem hiding this comment.
It's a nit.
The comment was to keep PODs as structs or convert them to classes to have functions like these.
There was a problem hiding this comment.
Ah I see, thanks for the clarification. Let me move forward with this PR and create an issue for improving the style on that.
| namespace maliput_sparse { | ||
| namespace loader { | ||
|
|
||
| class RoadGeometryLoader { |
There was a problem hiding this comment.
Nit: missing class docstring.
include/maliput_sparse/parser/lane.h
Outdated
| // The lanelet id. | ||
| std::string lane_id; | ||
| // The endpoint of the lanelet. | ||
| Which end; |
There was a problem hiding this comment.
Shall we add default values?
| bool operator==(const Segment& other) const { return id == other.id && lanes == other.lanes; } | ||
|
|
||
| /// Id of the segment. | ||
| Id id; |
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
🎉 New feature
Related to #34
Summary
This is a base proposal on #34, once this is given the thumbs up the code should be polished, tests should be changed/added and documentation should be added.
maliput/maliput_osm#30 matches these changes here added.
Checklist
Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.