Skip to content
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

Conversation

wxmerkt
Copy link
Collaborator

@wxmerkt wxmerkt commented Mar 7, 2019

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.

  • Upgrades to AprilTags 3, and in specific a thin CMake overlay of the now-officially-on-GitHub version. This replaces the apriltags2 catkin wrapper which was a fork and not fully working (installation...). AprilTags 3 should be faster by itself.
  • As part of this, some tag families and options are gone (refine_pose, black_border, refine_decode)
  • Renamed the libraries to include the project name as prefix. This avoids clashes with other common libraries.
  • Removed the forced compilation in Debug. I think that is the source of performance issues that many people have highlighted and that can easily trip people up who just clone the repository.

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:

CameronDevine and others added 5 commits March 7, 2019 11:24
… 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
@paulbovbel
Copy link

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)

@wxmerkt
Copy link
Collaborator Author

wxmerkt commented Mar 7, 2019

Thank you @paulbovbel! My plan is to switch over to CMake external project soon, add CI, and then release to the buildfarm.

@paulbovbel
Copy link

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.

@wxmerkt
Copy link
Collaborator Author

wxmerkt commented Mar 7, 2019

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

@rgreid
Copy link
Collaborator

rgreid commented Apr 4, 2019

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 apriltags and package name apriltag?

@dmalyuta Big picture question: given the upgrade to Apriltag 3, this repo/package name is outdated/confusing. Should we rename this repo to apriltags_ros before pushing it to the ROS buildfarm? Seems the right time to do it, while it's the least disruptive (github provides redirects).

@dmalyuta
Copy link
Collaborator

dmalyuta commented Apr 4, 2019

@rgreid I'm fine with changing the repo name to the more general version. Should it be named apriltags_ros or apriltag_ros?

@rgreid
Copy link
Collaborator

rgreid commented Apr 5, 2019

@rgreid I'm fine with changing the repo name to the more general version. Should it be named apriltags_ros or apriltag_ros?

I've branched this discussion to #45

@wxmerkt
Copy link
Collaborator Author

wxmerkt commented Apr 17, 2019

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 apriltag_ros (resolves #45). I am planning to tag this as version 2.0.0 once merged (with 1.0.0 being the last version using the current master).

Can you please test it and give feedback whether that's good to go? @dmalyuta @rgreid @mkrogius. Thank you very much :)

@rgreid
Copy link
Collaborator

rgreid commented Apr 20, 2019

Suggestion: Can we tag master from a month ago with apriltags2_ros and after the package rename to version 2.0.0. And once your Apriltag 3 branch has been merged 3.0.0? I.e. track the upstream version numbers (Sorry if I have misunderstood, can't browse the commits atm).

I'll get someone to test next week. Thanks.

@wxmerkt
Copy link
Collaborator Author

wxmerkt commented Apr 21, 2019

Thanks for volunteering to test this @rgreid. The current master (apriltags2_ros) is already tagged as 1.0.0. I am intending to tag it as 2.0.0 after this has been merged (as apriltag_ros).

Edit: Happy to change to track upstream version numbers - good call if everyone agrees.

@wxmerkt
Copy link
Collaborator Author

wxmerkt commented May 9, 2019

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.

@rgreid
Copy link
Collaborator

rgreid commented May 13, 2019

@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

@rgreid
Copy link
Collaborator

rgreid commented May 13, 2019

LGTM! Builds and runs fine. I wonder if bloom ignores Git submodules? If so, then perhaps we should either 1) add the upstream apriltag repo as a submodule here or 2) add something in README.md about the apriltag prerequisite?

@wxmerkt
Copy link
Collaborator Author

wxmerkt commented May 13, 2019

Thank you very much for testing!

I wonder if bloom ignores Git submodules? If so, then perhaps we should either 1) add the upstream apriltag repo as a submodule here or 2) add something in README.md about the apriltag prerequisite?

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.

@wxmerkt wxmerkt merged commit 92429aa into AprilRobotics:master May 13, 2019
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.

Fork/rename package to apriltag_ros to align with upstream Apriltag library Extremely High CPU Usage
5 participants