apriltag_ros
apriltag_ros copied to clipboard
AprilRobotics
feat: ros2 porting
@wxmerkt Could you create ros2 branch? I will change the base branch when it is created.
What's with the ROS2 node mentioned in https://github.com/AprilRobotics/apriltag_ros/issues/76? Are you missing some functionality? If you are missing features, just send a PR.
Edit: I also don't think it makes sense to maintain this in a separate ros2 feature branch. This will never be merged back into the main (ROS1) branch and I also think there will never be commits that can be cherry-picked between both versions, or pull-requests can be back-ported between versions. It makes more sense to just maintain two repos for this, as they do not share any common code that can be reused.
@christian-rauch
What's with the ROS2 node mentioned in https://github.com/AprilRobotics/apriltag_ros/issues/76? Are you missing some functionality? If you are missing features, just send a PR.
I've not checked the node you wrote yet, but I've ported all the features of the ros1 nodes in this repository.
I also don't think it makes sense to maintain this in a separate ros2 feature branch. This will never be merged back into the main (ROS1) branch and I also think there will never be commits that can be cherry-picked between both versions, or pull-requests can be back-ported between versions. It makes more sense to just maintain two repos for this, as they do not share any common code that can be reused.
In case the most of the features are rewritten in the huge repositories such as rviz2, it is better to maintain the separate repositories for ros1 and ros2, I think. However, apriltag_ros consists of a few features(load image / callback image and do tag detection), so the main part of the code are common and some minor changes are back-portable. Actually the most of ros package repositories which have both ros1 and ros2 implementations coexist in the same repository.
I've not checked the node you wrote yet, but I've ported all the features of the ros1 nodes in this repository.
You reference the issue but you did not look at this prior work? From my point of view, it makes no sense to split the same work amongst multiple nodes doing the same thing. We don't want someone proposing yet another ROS2 AprilTag node in a year. What would be the advantage of that?
However, apriltag_ros consists of a few features(load image / callback image and do tag detection), so the main part of the code are common and some minor changes are back-portable.
In my opinion, common framework-agnostic code should be moved into a library, which is then used by the individual frameworks. If someone wants to use the apriltag library in another framework, say LCM, then they would use the same methods and only implement the framework-specific parts. And these new framework implementations would also live in a separate repo and not as an unrelated branch in the same ROS repo.
You reference the issue but you did not look at this prior work? From my point of view, it makes no sense to split the same work amongst multiple nodes doing the same thing. We don't want someone proposing yet another ROS2 AprilTag node in a year.
I'm sorry about that if it makes you feel uncomfortable.
What would be the advantage of that?
I took a look at you code and found only image callback node. I implemented all the feature (such as single image server/client) same as the ros1 code and followed some ros2 rules.
In my opinion, common framework-agnostic code should be moved into a library, which is then used by the individual frameworks. If someone wants to use the apriltag library in another framework, say LCM, then they would use the same methods and only implement the framework-specific parts. And these new framework implementations would also live in a separate repo and not as an unrelated branch in the same ROS repo.
Coud you telll me which part is the common framework-agnostic code in your implemetation?
@wxmerkt Do you have any thoughts on this? I will follow the decision which the members of apriltag team make.
I took a look at you code and found only image callback node. I implemented all the feature (such as single image server/client) same as the ros1 code and followed some ros2 rules.
That is why I proposed sending a PR if you are missing a feature instead of creating a new node.
Coud you telll me which part is the common framework-agnostic code in your implemetation?
The only common function would be AprilTagNode::getPose, which could probably be replaced by estimate_pose_for_tag_homography.
That is why I proposed sending a PR if you are missing a feature instead of creating a new node.
I see, I'm sorry for my misunderstanding.
The only common function would be AprilTagNode::getPose, which could probably be replaced by estimate_pose_for_tag_homography.
How about creating a library package like apriltag_common including common functions you implemented in a separate repository and referring it from this repository?
How about creating a library package like
apriltag_commonincluding common functions you implemented in a separate repository and referring it from this repository?
I think it would make sense to move this directly into the apriltag repo/package. It would be a separate library (shared object) though so that users can decide to link this functionality or not.
The node uses common_functions.cpp. This contains a lot of ROS1-specific code and some generic code (such as in TagDetector::getRelativeTransform). It would make sense to move that generic code out of this file into the apriltag library.
@wxmerkt Could you create ros2 or rolling branch if possible? I would like to change the base branch after that.
@wxmerkt Thanks for creating the branch. Is it possible to merge this PR and create ros2 release after confirming the issue is resolved?
@wep21 can you please coordinate with @christian-rauch what to do about the two different apriltag_ros / apriltag_msgs versions for ROS2 (ideally in #76) - we can proceed with merging/release based on the outcome
Closing as the suggestion is to use https://github.com/christianrauch/apriltag_ros on ROS2