Noetic devel

[pmusau17]

Noetic devel port from @machinekoder

[kscottz]

I went through the source here, since there seems to be a lot of whitespace changes and it is really hard to tell where I need to be actually looking. I did see that there are a ton of code paths that are unused that didn't get updated (i.e. iplImage). We'll want to make sure we exercise the tests heavily to make sure that those never get hit. There's also a ton of code in ALVAR that is now in mainline opencv (or at least an implementation of it) we could flat out replace but that's a slog.

FWIW we might be able to prune the PR2 launch files.

I think in a perfect world we would yank out alvar and re-implement the marker logic in raw opencv. That's a lot of work and I don't understand the reqard. I think right now the best we can do is get this running and then insert a deprecation warning to get people to move over to a mainline marker format.

@mjcarroll I would like to hear your opinion on the above. If that sounds fine we can go ahead and merge this and do a susequent cleanup PR.

[pmusau17]

friendly ping for review @130s

[kscottz]

There is a lot going on here so I want to make it all clear to the other potential reviewers. The version of ALVAR vendored in this package is no longer supported by OpenCV so we're upgrading the vendored code to a version of ALVAR that works with the OpenCV supported by ROS 1 Noetic.

Simultaneously we're upgraded the node to Noetic and along with some much needed linting / refactoring (some really old standards were used, if any). All of this explains why this PR has so many changes in this PR. In retrospect we should have broken this up, but it was also a gnarly upgrade.

I only briefly skimmed the vendor files but I did give all of the ROS nodes a complete read through (I gave most of them a 👍🏻 so they're easy to find). I only saw one issue with a forward declaration that needs to be evaluated. While it would be nice to table the whitespace issues for a second PR I am loathe to make anyone revert to a worse standard, especially when there are no standards given or enforced. The fact that everything would be consistent for a new version of ROS is a big win in my mind.

[kscottz]

@mjcarroll @130s I went and reviewed this, it looks acceptable. Patrick and I went through and resolved my comments, did a full build, and ran the tests / launch files. I would like your sign off before we merge. There is a CMake 2.8.12 issue with google tests that needs to fixed but that can be addressed as a subsequent PR. It should address most of the issues related to this PR. We may still need to address issues related CI.

The PR is sizeable because of linting changes in the vendored packages. We apollogize for the mess but it at least it is slightly more standardized. If you focus on the ROS nodes themselves it really isn't that large. It is worth noting that we also revised a few of the tests to use a smaller local bag file to avoid test flakiness.

[kevt19]

Can you guys merge these changes with the main repo?

[RobertBlakeAnderson]

Yes, please merge. We had to clone this fork build the package.

[mysablehats]

Why isn't this main? Or at least there could be a readme.md on main saying:

# Noetic

use branch noetic-devel

[kscottz]

@mjcarroll @130s if I get your approval I could help move this through. Thanks!

[mjcarroll]

I looked at this a while ago and didn't have any major objections. The size of the diff is quite substantial, so I didn't get a chance to do real line-by-line reviews. It sounds like people are using it and are happy with it, so I think we can merge it and sort out any issues in folllow ups.

[bjsowa]

@kscottz @mjcarroll @130s Could you please move this through? It would really help me if the package was released to Noetic rosdistro.

[Khush-dev]

I get TF_NAN_INPUT #15 issue when using this PR on Noetic, Ubuntu 20.04.. I made a bag file for the same.
Error: TF_NAN_INPUT: Ignoring transform for child_frame_id "ar_marker_1" from authority "unknown_publisher" because of a nan value in the transform (0.645201 -3.742790 8.400771) (-nan -nan -nan -nan) at line 240 in /tmp/binarydeb/ros-noetic-tf2-0.7.5/src/buffer_core.cpp
Error: TF_DENORMALIZED_QUATERNION: Ignoring transform for child_frame_id "ar_marker_1" from authority "unknown_publisher" because of an invalid quaternion in the transform (-nan -nan -nan -nan) at line 255 in /tmp/binarydeb/ros-noetic-tf2-0.7.5/src/buffer_core.cpp

Also, I didn't get any errors(except maybe TF_REPEATED_DATA because of multiple tags of same ID) when using the #86 PR on the same platform/world.

Edit. Sorry, I didn't see that it is already being discussed in issue #82 with a bug fix, but that also gives the same error. Guess, I'll stick with #86

[ana-GT]

Hi! Could it be possible to merge this to noetic? 🙏 If any additional testings would be needed, I would be happy to help. Our team is using this package for a project, and it would be great if we could use the default noetic branch instead of the PR. Thanks!

[kscottz]

@mjcarroll this got left behind. Can we chat about it tomorrow and merge it?

[ana-GT]

Hi, just checking if there are any plans on merging this branch into noetic at this point? Thanks

[RobinHeitz]

Hi, I want to clarify, the package was not released (yet) for noetic, right?

[ana-GT]

Thank you very much! 🎉