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.

Sign up for GitHub

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

Jump to bottom

fix(compare_map_segmentation): throw runtime error when using non-split map pointcloud for DynamicMapLoader #9024

Merged
merged 8 commits into from
Oct 17, 2024
Merged

fix(compare_map_segmentation): throw runtime error when using non-split map pointcloud for DynamicMapLoader #9024

merged 8 commits into from
Oct 17, 2024

Conversation

badai-nguyen
Copy link
Contributor

@badai-nguyen badai-nguyen commented Oct 3, 2024 •
edited
Loading

Description

  • This PR will limit autoware to use split map pointcloud for dynamic_map_loader in compare_map_segmentation. Otherwise, ERROR message will be output as below
[component_container_mt-1] [ERROR] [1728371768.729818629] [perception.object_recognition.detection.voxel_based_compare_map_filter]: Map was not split or split map grid size is too large. Split map with grid size smaller than 100.000000

Related links

Parent Issue:

  • Link

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@badai-nguyen
fix(compare_map_segmentation): throw runtime error when using non-spl…
291e270 
…it map pointcloud for DynamicMapLoader

Signed-off-by: badai-nguyen <[email protected]>
@github-actions github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:require-cuda-build-and-test labels Oct 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 3, 2024 •
edited
Loading

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@badai-nguyen
chore: typo
50da316 
Signed-off-by: badai-nguyen <[email protected]>
@badai-nguyen
fix: launch
bcd2015 
Signed-off-by: badai-nguyen <[email protected]>
@github-actions github-actions bot added the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Oct 3, 2024
@badai-nguyen badai-nguyen added tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) labels Oct 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 3, 2024

Documentation URL: https://autowarefoundation.github.io/autoware.universe/pr-9024/
Modified URLs:

@badai-nguyen badai-nguyen mentioned this pull request Oct 3, 2024
Merged
4 tasks
@codecov Codecov
Copy link

codecov bot commented Oct 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 26.33%. Comparing base (0de8669) to head (e6197a6).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
...rc/voxel_grid_map_loader/voxel_grid_map_loader.hpp 0.00% 2 Missing ⚠️
...rc/voxel_grid_map_loader/voxel_grid_map_loader.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9024      +/-   ##
==========================================
- Coverage   27.09%   26.33%   -0.76%     
==========================================
  Files        1300     1301       +1     
  Lines       95810    95905      +95     
  Branches    39122    38656     -466     
==========================================
- Hits        25955    25253     -702     
- Misses      67257    68093     +836     
+ Partials     2598     2559      -39
Flag Coverage Δ *Carryforward flag
differential 7.53% <0.00%> (?)
total 26.33% <ø> (-0.76%) ⬇️ Carriedforward from 83a8323

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

YoshiRi
YoshiRi reviewed Oct 3, 2024
View reviewed changes
...eption/autoware_compare_map_segmentation/src/voxel_grid_map_loader/voxel_grid_map_loader.hpp Outdated
@@ -270,6 +271,10 @@ class VoxelGridDynamicMapLoader : public VoxelGridMapLoader
{
map_grid_size_x_ = map_cell_to_add.metadata.max_x - map_cell_to_add.metadata.min_x;
map_grid_size_y_ = map_cell_to_add.metadata.max_y - map_cell_to_add.metadata.min_y;
if (map_grid_size_x_ > max_map_grid_size_ || map_grid_size_y_ > max_map_grid_size_) {
throw std::runtime_error(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use RCLCPP_ERROR to handle ROS2 error like other nodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this error invoke with dynamic map loading mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YoshiRi

Should this error invoke with dynamic map loading mode?

Yes, Since function addMapCellAndFilter is member of VoxelGridDynamicMapLoade Class so this error invoked during Dynamic map loader mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please use RCLCPP_ERROR to handle ROS2 error like other nodes.

I fixed at 1416bfb

...autoware_compare_map_segmentation/schema/voxel_distance_based_compare_map_filter.schema.json Outdated Show resolved Hide resolved
@badai-nguyen @YoshiRi
Update perception/autoware_compare_map_segmentation/schema/voxel_dist…
b5dbabf 
…ance_based_compare_map_filter.schema.json

Co-authored-by: Yoshi Ri <[email protected]>
@badai-nguyen badai-nguyen marked this pull request as ready for review October 3, 2024 23:15
@github-actions github-actions bot removed the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Oct 4, 2024
@badai-nguyen
Copy link
Contributor Author

@yukkysaito Could you review this PR also as codeowner?

YoshiRi
YoshiRi approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

LGTM. We should fix launcher caller, right?

@badai-nguyen
Copy link
Contributor Author

badai-nguyen commented Oct 8, 2024 •
edited
Loading

LGTM. We should fix launcher caller, right?

@YoshiRi Thank you for your review. I disabled the use_dynamic_map_loading in default and tested on Evaluator. All tests were passed for PerceptionPerformancTest.

However, IMO, on evaluator's side, it's better to execute test with customizable use_dynamic_map_loading to ensure consistency with the loaded map.

@yukkysaito Could you approve this PR as codeowner?

@badai-nguyen badai-nguyen merged commit 37808bf into autowarefoundation:main Oct 17, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yukkysaito yukkysaito yukkysaito approved these changes

@YoshiRi YoshiRi YoshiRi approved these changes

@amc-nu amc-nu Awaiting requested review from amc-nu amc-nu is a code owner

Assignees
No one assigned
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) tag:require-cuda-build-and-test tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
Sensing & Perception Working Group
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@badai-nguyen @yukkysaito @YoshiRi