UbiquityRobotics / fiducials

Simultaneous localization and mapping using fiducial markers.
BSD 3-Clause "New" or "Revised" License
265 stars 135 forks source link

Add aspect ratio to FiducialTransforms and add missing check on tf_publish_interval #153

Open jack-digilabs opened 5 years ago

rohbotics commented 5 years ago

Hey @jack-digilabs Thanks for the contribution!

Can you separate this into 2 PRs, one with the tf publish check, and another for the aspect ratio?

The tf publish check should definitely make it in, the aspect ratio idea needs more thought.

jack-digilabs commented 5 years ago

Sure can. Quick clarifying question on the tf_publish_interval vs publish_tf

tf_publish_interval: ROS Wiki, "Specifies an interval at which poses are published, even if no fiducials are observed. This is useful in cases where the fiducial pose is used as a correction to odometry.". This makes me think setting tf_publish_interval==0 should result in tf's only being published when a fiducial is observed however my addition causes the tf never to be published when tf_publish_interval==0. Is this the functionality you want with tf_publish_interval==0 or should I use publish_tf==false to turn off publishing the tf's?

publish_tf: I'd expect this to completely turn off tf publishing and override the tf_publish_interval. Is my interpretation correct and should I add that functionality?

On Sun, Feb 17, 2019 at 12:23 PM Rohan Agrawal notifications@github.com wrote:

Hey @jack-digilabs https://github.com/jack-digilabs Thanks for the contribution!

Can you separate this into 2 PRs, one with the tf publish check, and another for the aspect ratio?

The tf publish check should definitely make it in, the aspect ratio idea needs more thought.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/UbiquityRobotics/fiducials/pull/153#issuecomment-464491017, or mute the thread https://github.com/notifications/unsubscribe-auth/AmLdCifuYQYbntRCDSlEg8W28pmC4TxIks5vOZ4RgaJpZM4a9yG_ .

rohbotics commented 5 years ago

Your interpretation is correct, publish_tf should override the tf_publish_interval.

Looking forward to the other PR

jack-digilabs commented 5 years ago

I submitted PR #154 to add the functionality described above.