refactor(lalenet2_map_validator): divide map loading process #153
Conversation
soblin
force-pushed
the
feat/add-map-load-validator
branch
from
b58aa63 to
5988d6b
Compare
soblin
requested review from
anhnv3991,
Motsu-san,
SakodaShintaro,
TaikiYamada4 and
YamatoAndo
as code owners
| { | ||
|
|
||
| std::pair<lanelet::LaneletMapPtr, std::vector<lanelet::validation::DetectedIssues>> | ||
| loadAndValidateMapLoad( |
There was a problem hiding this comment.
Is it necessary to have two loads in the function name here?
| replace_validator( | ||
| validator_config.command_line_config.validationConfig, validator_name))); | ||
| } | ||
| const auto prerequisite_issues = check_prerequisite_completion(validators, validator_name); |
There was a problem hiding this comment.
Could you add prerequisite_issues to total_issues?
I also want to display and write whether the prerequisites are met or not.
There was a problem hiding this comment.
I added them to total_issues in 5d8f6d3
| const auto prerequisite_issues = check_prerequisite_completion(validators, validator_name); | ||
|
|
||
| const auto issues = | ||
| prerequisite_issues.empty() |
There was a problem hiding this comment.
I believe the condition is the opposite.
All validations that have correct prerequisites will not start and only PASSED results appear.
There was a problem hiding this comment.
I fixed and added comment on it 5d8f6d3
| replace_validator( | ||
| validator_config.command_line_config.validationConfig, validator_name))); | ||
| } | ||
| const auto prerequisite_issues = check_prerequisite_completion(validators, validator_name); |
There was a problem hiding this comment.
I added them to total_issues in 5d8f6d3
| const auto prerequisite_issues = check_prerequisite_completion(validators, validator_name); | ||
|
|
||
| const auto issues = | ||
| prerequisite_issues.empty() |
There was a problem hiding this comment.
I fixed and added comment on it 5d8f6d3
| { | ||
| to.insert(to.end(), std::make_move_iterator(from.begin()), std::make_move_iterator(from.end())); | ||
| if constexpr (std::is_rvalue_reference<decltype(from)>::value) { |
There was a problem hiding this comment.
I fixed appendIssue to take universal reference of from to optimize the copy.
The trailing return type denotes the type constraint (concept) that T and std::vector<T>::value_type matches
|
@soblin |
Signed-off-by: Mamoru Sobue <[email protected]>
Signed-off-by: Mamoru Sobue <[email protected]>
Signed-off-by: Mamoru Sobue <[email protected]>
Signed-off-by: Mamoru Sobue <[email protected]>
soblin
force-pushed
the
feat/add-map-load-validator
branch
from
ed4841e to
b74497a
Compare
Description
Current implementation tries to load the lanelet_map for each validation process which is inefficient and contaminates the report. With this PR, the map is loaded at first and then passed to each validation process.
Related links
Tests performed
unit test passes
Notes for reviewers
Interface changes
None
Effects on system behavior
None
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.