add support for lyrical#196
Conversation
christian-rauch
force-pushed
the
lyrical
branch
2 times, most recently
from
42ecd92 to
aeb1670
Compare
christian-rauch
changed the title
|
@ob-yalian Can you have a look at this PR to get the |
christian-rauch
force-pushed
the
lyrical
branch
2 times, most recently
from
d632b52 to
2d32b93
Compare
|
@ob-yalian @jjiszjj @winf Can you comment on this PR or let me otherwise know, who is maintaining this repo and who I should contact? |
|
@christian-rauch Sorry for the late reply. Thanks for the reminder and for all the work you've put into this PR. I'll review the changes as soon as possible and get back to you with feedback. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 292275b83b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| diagnostic_updater::diagnostic_updater | ||
| Eigen3::Eigen | ||
| image_transport::image_transport | ||
| message_filters::message_filters |
There was a problem hiding this comment.
This target is linked without ever finding the message_filters package: the dependencies list above does not include it, so the foreach(find_package...) loop never creates message_filters::message_filters. In a normal ROS build CMake will stop at configure time with an imported-target-not-found error before orbbec_camera can build; add message_filters to the CMake/package dependencies before linking its target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This PR does not add the dependency on message_filters. It only changes the way it is used. For example, previously the header message_filters/subscriber.h was included. The PR changes this to header message_filters/subscriber.hpp from the same package. Review the full diff. There are no headers from new package dependencies.
| node_, "color/image_raw", rgb_qos); | ||
| node_, "color/image_raw", | ||
| #ifdef message_filters_QoS | ||
| rclcpp::QoS{rclcpp::QoSInitialization::from_rmw(rgb_qos)} |
There was a problem hiding this comment.
On the new message_filters_QoS path, this constructs rclcpp::QoS from only QoSInitialization::from_rmw(rgb_qos), which preserves history/depth but falls back to the default rmw profile for reliability/durability. When enable_d2c_viewer is used with color_qos or depth_qos set to SENSOR_DATA, the viewer subscriptions become reliable instead of best-effort and will not match the best-effort image publishers the user requested; pass the original rmw profile as the second rclcpp::QoS constructor argument as the rest of the codebase does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I am not sure exactly what you mean. The same QoS, rgb_qos, was used before. Please suggest an alternative, in your opinion correct way, how to construct a rclcpp::QoS from rgb_qos.
|
@ob-yalian @obshenlanzhou Can you check the codex review and my responses? I am not sure how much sense they make. Can you comment on your own what the issues with the proposed changes are? |
3 participants
This adds support for ROS 2
lyricalby mainly replacingament_target_dependencieswith the targets fortarget_link_libraries.