-
Notifications
You must be signed in to change notification settings - Fork 338
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.
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
Upgrade to AprilTag 3, fix installation, and performance improvements #43
Upgrade to AprilTag 3, fix installation, and performance improvements #43
Conversation
… apriltags2_ros nodes and launch files can be run when the install space is used. Simplified the directory install command in the apriltags2_ros package CMake file. Added a command to install the nodelet plugsins xml file.
- Replace catkin wrapper which had install issues (cf. AprilRobotics#22, AprilRobotics#37) with a submodule of the latest 0.10.0 version of AprilTag3 with CMake/package.xml support - Remove forced build in Debug - this slowed things down. - Removed tag families and options that are no longer supported in v3.0
- Resolves AprilRobotics#32
- Incorporates and resolves AprilRobotics#37, AprilRobotics#22, AprilRobotics#42, AprilRobotics#36
Looking forward to trying this! Does the ROS buildfarm still have trouble with submodules? Consider using a cmake external project (https://github.com/locusrobotics/apriltag2) |
Thank you @paulbovbel! My plan is to switch over to CMake external project soon, add CI, and then release to the buildfarm. |
Nice. That way you should be able to use the upstream source (https://github.com/AprilRobotics/apriltags) without any kind of vendoring like in (https://github.com/christianrauch/apriltag2). There's actually another approach via bloom (http://wiki.ros.org/bloom/Tutorials/ReleaseThirdParty), which I've used before to bloom vrpn (https://github.com/ros-drivers-gbp/vrpn-release). Let me know if you want a hand with that. |
@paulbovbel Thank you for the offer - very happy if you could assist with the release as a third-party library after @davelkan's 36h9 PR has been merged upstream. |
The CMake scripts and package.xml have been merged to the upstream source https://github.com/AprilRobotics/apriltags, which should make things easier. Seems to build fine. Should we encourage upstream to fix the mismatch between the repo name @dmalyuta Big picture question: given the upgrade to Apriltag 3, this repo/package name is outdated/confusing. Should we rename this repo to |
@rgreid I'm fine with changing the repo name to the more general version. Should it be named |
I've now updated the package to use the apriltag library directly (just clone it into the same workspace until it's released) and renamed it to Can you please test it and give feedback whether that's good to go? @dmalyuta @rgreid @mkrogius. Thank you very much :) |
Suggestion: Can we tag master from a month ago with I'll get someone to test next week. Thanks. |
Thanks for volunteering to test this @rgreid. The current master ( Edit: Happy to change to track upstream version numbers - good call if everyone agrees. |
Do you have any feedback on this PR @rgreid from testing? :-) If not, let's try to bloom-release the library and the wrapper this weekend. |
@wxmerkt I've had a colleague test the PR, however I'd like to test it myself in the next day or two (integrating into a much larger build system). Thanks |
LGTM! Builds and runs fine. I wonder if bloom ignores Git submodules? If so, then perhaps we should either 1) add the upstream |
Thank you very much for testing!
I removed the submodule on purpose - there is a note in the new README that it depends on the upstream AprilTag library, but perhaps it's too subtle at the moment and should be made more clear. I'll do this post-merge. |
This is quite a big pull request and incorporates many other currently open pull requests. I hope to address both installation and performance issues with this one.
common
libraries.Before I merge this, I'd like to get feedback from users and stakeholders that this works for their use cases and that it indeed fixes their issues.
In particular I think this PR will resolve: