-
Notifications
You must be signed in to change notification settings - Fork 33
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
refactor(lalenet2_map_validator): divide map loading process #153
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mamoru Sobue <[email protected]>
b58aa63
to
5988d6b
Compare
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.
I have three review points and two of them are critical when processing the prerequisites.
{ | ||
|
||
std::pair<lanelet::LaneletMapPtr, std::vector<lanelet::validation::DetectedIssues>> | ||
loadAndValidateMapLoad( |
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.
Is it necessary to have two load
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add prerequisite_issues to total_issues?
I also want to display and write whether the prerequisites are met or not.
const auto prerequisite_issues = check_prerequisite_completion(validators, validator_name); | ||
|
||
const auto issues = | ||
prerequisite_issues.empty() |
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.
I believe the condition is the opposite.
All validations that have correct prerequisites will not start and only PASSED
results appear.
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.