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

[ROS2] Raycast Results Refactor #751

Merged
merged 3 commits into from
Aug 29, 2024
Merged

[ROS2] Raycast Results Refactor #751

merged 3 commits into from
Aug 29, 2024

Conversation

alek-kam-robotec-ai
Copy link
Contributor

What does this PR do?

This pull request features:

  • Refactor of the Raycast Request class:
    • results now are guaranteed to be the same size
    • introducing new fields is now easier
  • New ROS 2 pc message builder class

This PR aims to resolve #736.

How was this PR tested?

Ran in multiple configurations (also with RGL gem) on a simple project.

@alek-kam-robotec-ai alek-kam-robotec-ai mentioned this pull request Aug 22, 2024
Merged
michalpelka
michalpelka reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@michalpelka michalpelka left a comment

Choose a reason for hiding this comment

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

I did not test those changes, but added some questions and suggestions.

Gems/ROS2/Code/Include/ROS2/Lidar/LidarRaycasterBus.h Show resolved Hide resolved
Gems/ROS2/Code/Include/ROS2/Lidar/LidarRaycasterBus.h Show resolved Hide resolved
Gems/ROS2/Code/Source/Lidar/ROS2LidarSensorComponent.cpp Outdated Show resolved Hide resolved
@alek-kam-robotec-ai
Refactor Raycast Results
b2ec5b6 
Signed-off-by: Aleksander Kamiński <[email protected]>
@alek-kam-robotec-ai
Review changes
929629f 
Signed-off-by: Aleksander Kamiński <[email protected]>
PawelLiberadzki
PawelLiberadzki reviewed Aug 23, 2024
View reviewed changes
Copy link
Contributor

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

Nice code! I did not run it so far, but I will definitely check the runtime before approving.

Gems/ROS2/Code/Include/ROS2/Lidar/RaycastResults.h Show resolved Hide resolved
Gems/ROS2/Code/Include/ROS2/Lidar/RaycastResults.h Outdated Show resolved Hide resolved
Gems/ROS2/Code/Include/ROS2/Lidar/RaycastResults.h Outdated Show resolved Hide resolved
Gems/ROS2/Code/Include/ROS2/Lidar/RaycastResults.h Show resolved Hide resolved
Gems/ROS2/Code/Source/Lidar/LidarCore.cpp Outdated Show resolved Hide resolved
Gems/ROS2/Code/Source/Lidar/LidarCore.cpp Show resolved Hide resolved
Gems/ROS2/Code/Source/Lidar/PointCloudMessageBuilder.h Outdated Show resolved Hide resolved
Gems/ROS2/Code/Source/Lidar/ROS2Lidar2DSensorComponent.cpp Show resolved Hide resolved
Gems/ROS2/Code/Source/Lidar/RaycastResults.cpp Outdated Show resolved Hide resolved
@alek-kam-robotec-ai
Review changes
9ee4ac6 
Signed-off-by: Aleksander Kamiński <[email protected]>
PawelLiberadzki
PawelLiberadzki approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

LGTM! I also ran this code (latest stabilization/2409) and it seems to work fine + results are observable in rviz2 as expected.

@byrcolin byrcolin added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 27, 2024
@michalpelka michalpelka added the sig/simulation Categorizes an issue or PR as relevant to SIG Simulation label Aug 28, 2024
@jhanca-robotecai jhanca-robotecai merged commit 2a42422 into o3de:development Aug 29, 2024
2 checks passed
@jhanca-robotecai jhanca-robotecai deleted the refactor/raycast-results branch August 29, 2024 12:43
jhanca-robotecai pushed a commit to RobotecAI/o3de-extras that referenced this pull request Oct 11, 2024
@alek-kam-robotec-ai @jhanca-robotecai
[ROS2] Raycast Results Refactor (o3de#751)
907b5e3 
Signed-off-by: Aleksander Kamiński <[email protected]>
@jhanca-robotecai jhanca-robotecai mentioned this pull request Oct 11, 2024
Merged
jhanca-robotecai pushed a commit to RobotecAI/o3de-extras that referenced this pull request Nov 22, 2024
@alek-kam-robotec-ai @jhanca-robotecai
[ROS2] Raycast Results Refactor (o3de#751)
b250f48 
Signed-off-by: Aleksander Kamiński <[email protected]>
jhanca-robotecai pushed a commit to RobotecAI/o3de-extras that referenced this pull request Nov 25, 2024
@alek-kam-robotec-ai @jhanca-robotecai
[ROS2] Raycast Results Refactor (o3de#751)
c03d27c 
Signed-off-by: Aleksander Kamiński <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spham-amzn spham-amzn spham-amzn approved these changes

@PawelLiberadzki PawelLiberadzki PawelLiberadzki approved these changes

@michalpelka michalpelka michalpelka approved these changes

Assignees
No one assigned
Labels
needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/simulation Categorizes an issue or PR as relevant to SIG Simulation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ROS2] Feature: Improved Lidar Result retrieval and publishing
6 participants
@alek-kam-robotec-ai @michalpelka @PawelLiberadzki @spham-amzn @byrcolin @jhanca-robotecai