Skip to content

Expose optical frame in CameraSensor to be used by DepthCameraSensor#362

Merged
iche033 merged 1 commit intogazebosim:gz-sensors7from
Levi-Armstrong:fix/DepthCameraSensor
Jul 25, 2023
Merged

Expose optical frame in CameraSensor to be used by DepthCameraSensor#362
iche033 merged 1 commit intogazebosim:gz-sensors7from
Levi-Armstrong:fix/DepthCameraSensor

Conversation

@Levi-Armstrong
Copy link
Contributor

Summary

I was using DepthCameraSensor along with the ros_gz_bridge and noticed that the point cloud could not be visualized in rviz because the optical frame was not being assigned to the message being produced by this package.

@Levi-Armstrong Levi-Armstrong requested a review from iche033 as a code owner July 21, 2023 14:25
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #362 (c9b8262) into gz-sensors7 (44c67b0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head c9b8262 differs from pull request most recent head 488fcff. Consider uploading reports for the commit 488fcff to get more accurate results

@@               Coverage Diff               @@
##           gz-sensors7     #362      +/-   ##
===============================================
+ Coverage        69.71%   69.76%   +0.04%     
===============================================
  Files               36       36              
  Lines             3959     3965       +6     
===============================================
+ Hits              2760     2766       +6     
  Misses            1199     1199              
Impacted Files Coverage Δ
src/CameraSensor.cc 76.09% <100.00%> (+0.11%) ⬆️
src/DepthCameraSensor.cc 74.80% <100.00%> (+0.39%) ⬆️

@Levi-Armstrong Levi-Armstrong force-pushed the fix/DepthCameraSensor branch from 2f2b073 to 327531a Compare July 21, 2023 19:54
@Levi-Armstrong
Copy link
Contributor Author

This looks good now as far as the message containing the correct frame_id if set optical frame.


/// \brief Get the camera optical frame
/// \return The camera optical frame
public: std::string OpticalFrameId() const;
Copy link

Choose a reason for hiding this comment

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

Suggested change
public: std::string OpticalFrameId() const;
public: const std::string &OpticalFrameId() const;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally did this but the rest of the API returns string by value like FrameId().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@azeey Let me know if I should still make the change.

Copy link

Choose a reason for hiding this comment

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

Not sure if the extra copy makes sense especially since this will be called every time a message is published. I'd vote for const ref. Maybe @iche033 can weigh in.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah unfortunately it breaks API consistency but let's go with const & since as it makes sense here, and we'll start doing this more in new APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to return const&

@Levi-Armstrong Levi-Armstrong force-pushed the fix/DepthCameraSensor branch from a6f664f to 08ce9c4 Compare July 24, 2023 19:23
…ensor

Signed-off-by: Levi Armstrong <levi.armstrong@gmail.com>
@Levi-Armstrong Levi-Armstrong force-pushed the fix/DepthCameraSensor branch from 08ce9c4 to 488fcff Compare July 24, 2023 19:24
Copy link

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM!

@azeey
Copy link

azeey commented Jul 24, 2023

Rerunning the Ubuntu CI. I think it had an infra issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🌱 garden Ignition Garden

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants