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

fix(map_loader): fix warnings with single point cloud map metadata #6384

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

xmfcx
Copy link
Contributor

@xmfcx xmfcx commented Feb 12, 2024

Description

Part of:

Right now, even when we create a metadata with a single pcd, we receive "Create PCD metadata, as the pointcloud is a single file." warning from pointcloud_map_loader.

But it should throw the warning if there is no metadata but there is no metadata at all.

I've reordered the logic structure so:
In order:

  1. if metadata exists -> use new method
  2. if not, but only 1 pcd -> use exceptional method and warn
  3. if not -> throw warning (no metadata)

cc. @ahmeddesokyebrahim

Tests performed

Help appreciated.

Effects on system behavior

Not applicable.

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.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@github-actions github-actions bot added the component:map Map creation, storage, and loading. (auto-assigned) label Feb 12, 2024
@xmfcx xmfcx added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Feb 12, 2024
@xmfcx
Copy link
Contributor Author

xmfcx commented Feb 12, 2024

I don't have a multi pcd map to test this.

@kminoda could you test this with a multi pcd map?

I can test it with single pcd map with and without metadata.

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@ahmeddesokyebrahim
Copy link
Contributor

@xmfcx That sounds great!!
I do really appreciate the amazing support and collaboration from your side. 👍

M. Fatih Cırıt and others added 3 commits February 12, 2024 17:25
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (a10f9c4) 14.88% compared to head (fd00125) 14.88%.

Files Patch % Lines
...intcloud_map_loader/pointcloud_map_loader_node.cpp 0.00% 8 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6384   +/-   ##
=======================================
  Coverage   14.88%   14.88%           
=======================================
  Files        1838     1838           
  Lines      126708   126707    -1     
  Branches    38042    38040    -2     
=======================================
  Hits        18865    18865           
+ Misses      86544    86542    -2     
- Partials    21299    21300    +1     
Flag Coverage Δ *Carryforward flag
differential 26.65% <0.00%> (?)
total 14.88% <ø> (+<0.01%) ⬆️ Carriedforward from a10f9c4

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kminoda
Copy link
Contributor

kminoda commented Feb 13, 2024

@xmfcx Hi, thank you for the PR. Unfortunately I am no longer a Perception team, so let me redirect this to Localization/Mapping team in TIER IV.

@Motsu-san @YamatoAndo Hi, would you check this?

Copy link
Contributor

@SakodaShintaro SakodaShintaro left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request.

I have confirmed the following scenarios:

  • Single pcd with and without metadata: logging_simulator with sample map and sample rosbag
    • Works fine without log messages
  • Multi-PCD with metadata: AWSIM on divided nishishinjuku map
    • Works fine
  • Multi-pcd without metadata: AWSIM on divided nishishinjuku map
    • Outputs error logs properly (but only to the screen)
    • [Error] [1707783487.253310648] [map.pointcloud_map_loader]: PCD metadata file not found

I also don't think it's a problem from a code review perspective. Thank you for your improvement.

[Additional Note] If you want a divided map, you can use one provided as a sample or manually divide map using pointcloud_divider.

https://autowarefoundation.github.io/autoware-documentation/main/how-to-guides/others/using-divided-map/

@xmfcx xmfcx self-assigned this Feb 13, 2024
@xmfcx xmfcx merged commit 19f36ac into main Feb 13, 2024
24 of 26 checks passed
@xmfcx xmfcx deleted the feat/fix-map-loader-warning-msg branch February 13, 2024 05:46
StepTurtle pushed a commit to StepTurtle/autoware.universe that referenced this pull request Feb 28, 2024
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:map Map creation, storage, and loading. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants