search

ros-perception / ar_track_alvar

AR tag tracking library for ROS
www.ros.org/wiki/ar_track_alvar
141 stars 129 forks source link

Noetic devel #89

Closed pmusau17 closed 1 year ago

pmusau17 commented 3 years ago

Noetic devel port from @machinekoder

pmusau17 commented 3 years ago

friendly ping for review @130s

kscottz commented 3 years ago

@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 commented 2 years ago

Can you guys merge these changes with the main repo?

RobertBlakeAnderson commented 2 years ago

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

mysablehats commented 2 years ago

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

# Noetic

use branch noetic-devel
kscottz commented 2 years ago

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

mjcarroll commented 2 years ago

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 commented 2 years ago

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

Khush-dev commented 2 years ago

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 commented 1 year ago

Hi! Could it be possible to merge this to noetic? :pray: 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 commented 1 year ago

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

ana-GT commented 1 year ago

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

RobinHeitz commented 1 year ago

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

ana-GT commented 1 year ago

Thank you very much! :tada: