-
Notifications
You must be signed in to change notification settings - Fork 669
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
feat(pointpainting_fusion): add test for painting util #7169
feat(pointpainting_fusion): add test for painting util #7169
Conversation
Signed-off-by: tzhong518 <[email protected]>
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.
Nice work! Could you check some suggenstions?
@@ -33,6 +33,13 @@ namespace image_projection_based_fusion | |||
{ | |||
using Label = autoware_auto_perception_msgs::msg::ObjectClassification; | |||
|
|||
inline bool isInsideBbox( |
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.
zc means scaling, right?
- I prefer more specific name like
roi_expand_scale
orscale_factor
- Current calculation will not properly expand original rois (see figure below)
How about following expression?
inline bool isInsideBbox(
float proj_x, float proj_y, const sensor_msgs::msg::RegionOfInterest& roi, float scale_factor)
{
// Calculate new width and height with scaling
float new_width = roi.width * scale_factor;
float new_height = roi.height * scale_factor;
// Calculate new offsets to keep the center fixed
float new_x_offset = roi.x_offset + (roi.width - new_width) / 2.0;
float new_y_offset = roi.y_offset + (roi.height - new_height) / 2.0;
// Check if the projected point is within the scaled ROI
return proj_x >= new_x_offset && proj_x <= (new_x_offset + new_width) &&
proj_y >= new_y_offset && proj_y <= (new_y_offset + new_height);
}
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.
zc means the z-axis value of the pointcloud in the camera coordinate, the formula is commented here. zc is multified to the roi offset to avoid division calculation.
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.
Now I understand your point. Would you mind add comment like below?
// z_c is scaling to normalize projection point
const sensor_msgs::msg::RegionOfInterest roi = createRoi(0, 0, 0, 0); | ||
bool result = image_projection_based_fusion::isInsideBbox(0.0, 0.0, roi, 1.0); | ||
EXPECT_TRUE(result); | ||
} |
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.
[imo]
We can add test on roi expansion
TEST(isInsideBboxTest, Expand_Border)
{
const sensor_msgs::msg::RegionOfInterest roi = createRoi(20, 20, 10, 10);
bool result = image_projection_based_fusion::isInsideBbox(15.0, 15.0, roi, 2.0);
EXPECT_TRUE(result);
}
[memo] I think following // project
Eigen::Vector3f normalized_projected_point = camera_projection * Eigen::Vector3f(p_x, p_y, p_z);
// iterate 2d bbox
for (const auto & feature_object : objects) {
sensor_msgs::msg::RegionOfInterest roi = feature_object.feature.roi;
// paint current point if it is inside bbox
int label2d = feature_object.object.classification.front().label;
if (
!isUnknown(label2d) &&
isInsideBbox(normalized_projected_point.x(), normalized_projected_point.y(), roi, p_z)) {
data = &painted_pointcloud_msg.data[0];
auto p_class = reinterpret_cast<float *>(&output[stride + class_offset]);
for (const auto & cls : isClassTable_) {
// add up the class values if the point belongs to multiple classes
*p_class = cls.second(label2d) ? (class_index_[cls.first] + *p_class) : *p_class;
}
} |
Signed-off-by: tzhong518 <[email protected]>
…ng518/autoware.universe into test/pointpainting_fusion/add_test2
@@ -33,6 +33,13 @@ namespace image_projection_based_fusion | |||
{ | |||
using Label = autoware_auto_perception_msgs::msg::ObjectClassification; | |||
|
|||
inline bool isInsideBbox( |
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.
Now I understand your point. Would you mind add comment like below?
// z_c is scaling to normalize projection point
Signed-off-by: tzhong518 <[email protected]>
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7169 +/- ##
==========================================
- Coverage 28.55% 28.29% -0.27%
==========================================
Files 1586 1587 +1
Lines 115879 115605 -274
Branches 49362 49204 -158
==========================================
- Hits 33094 32711 -383
- Misses 73745 73947 +202
+ Partials 9040 8947 -93
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: yoshiri <[email protected]>
…ation#7169) * add: unit test for inside bbox func Signed-off-by: tzhong518 <[email protected]> * style(pre-commit): autofix * fix: correct param name Signed-off-by: tzhong518 <[email protected]> * add: comment Signed-off-by: tzhong518 <[email protected]> --------- Signed-off-by: tzhong518 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: palas21 <[email protected]>
…ation#7169) * add: unit test for inside bbox func Signed-off-by: tzhong518 <[email protected]> * style(pre-commit): autofix * fix: correct param name Signed-off-by: tzhong518 <[email protected]> * add: comment Signed-off-by: tzhong518 <[email protected]> --------- Signed-off-by: tzhong518 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* add: unit test for inside bbox func Signed-off-by: tzhong518 <[email protected]> * style(pre-commit): autofix * fix: correct param name Signed-off-by: tzhong518 <[email protected]> * add: comment Signed-off-by: tzhong518 <[email protected]> --------- Signed-off-by: tzhong518 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…ation#7169) * add: unit test for inside bbox func Signed-off-by: tzhong518 <[email protected]> * style(pre-commit): autofix * fix: correct param name Signed-off-by: tzhong518 <[email protected]> * add: comment Signed-off-by: tzhong518 <[email protected]> --------- Signed-off-by: tzhong518 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
Add test for util function in pointpainting_fusion
Related links
https://tier4.atlassian.net/browse/RT1-6382
Tests performed
I checked this PR with running autoware.
Notes for reviewers
Interface changes
ROS Topic Changes
ROS Parameter Changes
Effects on system behavior
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.