Skip to content

Conversation

w-czerski
Copy link
Contributor

@w-czerski w-czerski commented Jan 28, 2025

About:

This PR adds a component with a spline path publisher.
It exposes current entity attached spline as a path available for ros2.

Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Copy link
Collaborator

@jhanca-robotecai jhanca-robotecai 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 run the code, only browsed diagonally. Some nitpicks listed.

Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
@w-czerski
Copy link
Contributor Author

Merge order:

#109 -> #108 -> main

Comment on lines 110 to 111
const ROS2::ROS2FrameComponent* ros2Frame = GetEntity()->FindComponent<ROS2::ROS2FrameComponent>();
if (!ros2Frame)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, the ros2Frame can be assumed to exist because you connect to Tick bus after the check in line 68.

As far as I know, you can't remove/add components when entity is active, therefore we can be sure that nothing strange happen between the ticks (without deactivate/activate and frame re-check). Additionally, you can save the reference and reuse it here if needed - no need to find component in every tick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I've replaced it with AZ_Assert check.
ros2FramePtr now is initialized OnActivate and reused in PublishSplinePath.

Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
@w-czerski
Copy link
Contributor Author

I've added an update frequency option and readme.

@w-czerski w-czerski requested a review from pijaro March 18, 2025 12:21
@w-czerski w-czerski self-assigned this Mar 18, 2025
Copy link
Collaborator

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

Works great, I've tested it against #123

@michalpelka michalpelka merged commit b658ae0 into main May 5, 2025
1 check passed
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.

4 participants