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

robot_description_loaderを実装 #42

Merged
merged 21 commits into from
Sep 22, 2023
Merged

Conversation

YusukeKato
Copy link

@YusukeKato YusukeKato commented Sep 14, 2023

What does this implement/fix?

robot_description_loaderを実装しました。

Does this close any currently open issues?

しません。

How has this been tested?

pytestを実行してテストが通ることを確認しました。

Any other comments?

colcon testが通るように空行を削除した箇所があります。

Checklists

@YusukeKato YusukeKato added the Type: Feature New Feature label Sep 14, 2023
@YusukeKato YusukeKato self-assigned this Sep 14, 2023
@ShotaAk
Copy link

ShotaAk commented Sep 15, 2023

display.launch.pyでdescription_loaderを使うように変更お願いします

@ShotaAk
Copy link

ShotaAk commented Sep 15, 2023

github上でテストを実行したいのでCIファイルを追加してください

package.xml Show resolved Hide resolved
package.xml Outdated Show resolved Hide resolved
Copy link

@ShotaAk ShotaAk left a comment

Choose a reason for hiding this comment

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

レビューコメント反映し忘れていました。。。コメントの確認お願いします。

Comment on lines 16 to 24
<xacro:arg name="use_gazebo" default="false" />
<xacro:arg name="use_rgb_camera" default="false" />
<xacro:arg name="use_line_sensor" default="false" />
<xacro:arg name="use_lidar_urg" default="false" />
<xacro:arg name="use_lidar_lds" default="false" />
<xacro:arg name="use_lidar_rplidar" default="false" />
<xacro:arg name="use_imu" default="false" />
<xacro:arg name="gz_control_config_package" default="" />
<xacro:arg name="gz_control_config_file_path" default="" />
Copy link

Choose a reason for hiding this comment

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

今回の変更でこれらの引数は使用されていません。
機能追加時に引数を追加してください。

Suggested change
<xacro:arg name="use_gazebo" default="false" />
<xacro:arg name="use_rgb_camera" default="false" />
<xacro:arg name="use_line_sensor" default="false" />
<xacro:arg name="use_lidar_urg" default="false" />
<xacro:arg name="use_lidar_lds" default="false" />
<xacro:arg name="use_lidar_rplidar" default="false" />
<xacro:arg name="use_imu" default="false" />
<xacro:arg name="gz_control_config_package" default="" />
<xacro:arg name="gz_control_config_file_path" default="" />

Comment on lines 35 to 43
self.use_gazebo = 'false'
self.use_rgb_camera = 'false'
self.use_line_sensor = 'false'
self.use_lidar_urg = 'false'
self.use_lidar_lds = 'false'
self.use_lidar_rplidar = 'false'
self.use_imu = 'false'
self.gz_control_config_package = ''
self.gz_control_config_file_path = ''
Copy link

Choose a reason for hiding this comment

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

これらの引数は今回の変更で使用されていないので削除お願いします。

get_package_share_directory('raspimouse_description'),
'urdf',
'raspimouse.urdf.xacro')
self.use_gazebo = 'false'
Copy link

Choose a reason for hiding this comment

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

display.launch.pyでも使用されている引数、lidarlidar_frameに対応してください。

Copy link

Choose a reason for hiding this comment

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

引数、lidarとlidar_frameのテストを追加してください。

引数lidarについては、"none", "urg", "lds", or "rplidar" が正しく反映されることを検証してください

Copy link

@ShotaAk ShotaAk left a comment

Choose a reason for hiding this comment

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

diplay.launch.pyが動作すること確認できました。
テストも実行できており、良いと思います。

@ShotaAk ShotaAk merged commit 986f583 into ros2 Sep 22, 2023
2 checks passed
@ShotaAk ShotaAk deleted the feature/add-robot-description-loader branch September 22, 2023 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants