Skip to content

DO NOT MERGE:Feat/sensing/pointcloud preprocessor#125

Open
Bey9434 wants to merge 28 commits intomainfrom
feat/sensing/pointcloud_preprocessor
Open

DO NOT MERGE:Feat/sensing/pointcloud preprocessor#125
Bey9434 wants to merge 28 commits intomainfrom
feat/sensing/pointcloud_preprocessor

Conversation

@Bey9434
Copy link
Copy Markdown
Contributor

@Bey9434 Bey9434 commented Aug 10, 2024

pointcloud_preprocessorのtfを用いた座標変換のプログラミングを書いたので、改善点などのレビューをお願いします。

ノード立ち上げ(wheel_stuck_common_utilsのビルドも必要)
colcon build --packages-select pointcloud_preprocessor
source install/setup.bash
ros2 run pointcloud_preprocessor pointcloud_preprocessor_node

点群データはrosbagとrvizを使って確認しています。

@Bey9434 Bey9434 requested a review from Autumn60 August 10, 2024 16:02
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ファイル前半に余計な行が入っている?

Copy link
Copy Markdown
Collaborator

@Autumn60 Autumn60 Aug 11, 2024

Choose a reason for hiding this comment

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

PointXYZ(もしくはPointXYZI)で良いので不要

#include <wheel_stuck_common_utils/geometry/conversion.hpp>
#include <wheel_stuck_common_utils/pointcloud/transform_pointcloud.hpp>
#include <wheel_stuck_common_utils/ros/function_timer.hpp>
#include <wheel_stuck_common_utils/ros/no_callback_subscription.hpp>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

使ってないライブラリはインクルード不要


#ifndef LASERS_NUM
#define LASERS_NUM 32
#endif
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

使ってないプリプロセッサも不要


class PointCloudPreprocessor : public rclcpp::Node
{
private:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

↓の行が不要なら"private:"の行も不要

explicit PointCloudPreprocessor(const rclcpp::NodeOptions & options);

private:
void processCloud(const sensor_msgs::msg::PointCloud2::SharedPtr msg);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

関数名はスネークケースに統一

Copy link
Copy Markdown
Collaborator

@Autumn60 Autumn60 left a comment

Choose a reason for hiding this comment

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

メンバ変数である必要がないものはローカル変数に。

+@
どこかで"base_link"はパラメータ化したい

@Bey9434 Bey9434 requested a review from Autumn60 August 13, 2024 02:10
@RyodoTanaka RyodoTanaka marked this pull request as draft August 13, 2024 12:42
…l-Stuck/wheel_stuck_ws into feat/sensing/pointcloud_preprocessor
@Bey9434 Bey9434 marked this pull request as ready for review August 18, 2024 19:29
@Bey9434 Bey9434 requested a review from Autumn60 August 18, 2024 19:29
@Bey9434
Copy link
Copy Markdown
Contributor Author

Bey9434 commented Aug 18, 2024

指摘箇所修正しました。
問題なさそうであれば、クロッピングのコードを書きたいと思います。

@Bey9434 Bey9434 marked this pull request as draft August 21, 2024 14:50
@Bey9434 Bey9434 self-assigned this Aug 21, 2024
@Bey9434 Bey9434 marked this pull request as ready for review August 26, 2024 14:44
@Bey9434
Copy link
Copy Markdown
Contributor Author

Bey9434 commented Aug 26, 2024

・クロッピング機能を追加しました。
・spellにcropboxを追加しました。
・wheel_stuck_robot_utilsのパラメーターをlaunchで読み込んでいるので、試す際は適当な値が必要です。

colcon build --packages-select wheel_stuck_robot_utils
colcon build --packages-select pointcloud_preprocessor
source install/setup.bash
ros2 launch pointcloud_preprocessor pointcloud_preprocessor.launch.xml

です。よろしくおねがいします。

wheel_stuck_robot_utils::RobotInfoUtils robot_info_utils(node);
robot_info_ = robot_info_utils.get_info();
// パラメーターが読み込まれた後のログ出力
RCLCPP_INFO(node.get_logger(), "Front Overhang: %f", robot_info_.raw_info.front_overhang_m);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

デバッグ用の行は消してください

}

pcl::PointCloud<pcl::PointXYZ>::Ptr crop_box(
const pcl::PointCloud<pcl::PointXYZ>::Ptr & pcl_output,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

関数の引数の命名は「そのオブジェクトから見た命名」にするべきなので、outputではなくinputであるべきです。
+@ 可能なら第2引数と命名を揃えてください。

const pcl::PointCloud<pcl::PointXYZ>::Ptr & pcl_output,
pcl::PointCloud<pcl::PointXYZ>::Ptr & cropped_cloud)
{
Eigen::Vector4f self_min_pt(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

min_ptとmax_ptは毎ループ変わるわけではないので、メンバ変数としてキャッシュするべきです。

pcl::CropBox<pcl::PointXYZ> area_crop_box_filter;
area_crop_box_filter.setInputCloud(self_cropped_cloud);
Eigen::Vector4f area_min_pt(-5.0, -5.0, -5.0, 1.0);
Eigen::Vector4f area_max_pt(5.0, 5.0, 5.0, 1.0);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

クロップ範囲をパラメータ化してください

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

robot_infoはwheel_stuck_robot_launcherパッケージのload_robot_info.launch.pyによって配信されるため、このlaunchでは扱わないでください。


// 受信機を作る。
pc_sub_ = this->create_subscription<PointCloud2>(
"/velodyne_points", 10,
Copy link
Copy Markdown
Collaborator

@Autumn60 Autumn60 Aug 26, 2024

Choose a reason for hiding this comment

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

.launch.xml内のremapのトピック名と一致していません。

<arg name="config_file2" default="$(find-pkg-share wheel_stuck_robot_utils)/config/robot_info.param.yaml"/>

<node pkg="pointcloud_preprocessor" exec="pointcloud_preprocessor_node" name="pointcloud_preprocessor" output="screen">
<remap from="~/input/points" to="$(var points_topic)"/>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

.cpp内のトピック名と一致していません。

transform_matrix = to_matrix4f(transform_stamped.transform);

// 点群データの座標変換
pcl::PointCloud<pcl::PointXYZ>::Ptr pcl_output(new pcl::PointCloud<pcl::PointXYZ>());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

変数名として、pcl_transformedあたりが無難(?)

pcl::transformPointCloud(*pcl_input, *pcl_output, transform_matrix);

// クロッピング処理
cropbox_filter::CropBoxFilter crop_box_filter(*this);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

crop_box_filterの設定(クロップ範囲など)はループ毎に不変なので、crop_box_filterはメンバとしてキャッシュするべきです。

cropbox_filter::CropBoxFilter crop_box_filter(*this);
// デバッグ用のパラメータ確認メソッドを呼び出す。デバッグ用。
// crop_box_filter.debug_parameters();
pcl::PointCloud<pcl::PointXYZ>::Ptr cropped_cloud(new pcl::PointCloud<pcl::PointXYZ>());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

命名を揃えるならばpcl_cropped?

pcl::VoxelGrid<pcl::PointXYZ> voxel_grid;
voxel_grid.setInputCloud(cropped_cloud);
voxel_grid.setLeafSize(leaf_size_, leaf_size_, leaf_size_);
pcl::PointCloud<pcl::PointXYZ> downsampled_cloud;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

命名を揃えるならばpcl_downsampled?

return cropped_cloud;
}
// デバッグ用。パラメーターの取得の確認。
void debug_parameters() const
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

print_debug_parameters()のほうがより関数の目的が説明できそう?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+@
CropBoxFilterのパラメータのprintではなく、RobotInfoのパラメータのprintになっている。
WheelRadius, WheelTreadなどのパラメータをprintする関数はRobotInfo側に実装するべきで、CropBoxFilter側で実装するprint関数はCropBoxのパラメータ(self_min_pt)とかをprintするべき。

@Bey9434 Bey9434 requested a review from Autumn60 August 29, 2024 09:43
@Bey9434 Bey9434 marked this pull request as draft August 29, 2024 16:57
@Bey9434 Bey9434 marked this pull request as ready for review August 31, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants