search

pal-robotics / aruco_ros

Software package and ROS wrappers of the Aruco Augmented Reality marker detector library
MIT License
449 stars 307 forks source link

New executable to publish all visible marker simultaneously #7

Closed jdlangs closed 9 years ago

jdlangs commented 9 years ago

I've found it very nice to just have ROS node subscribe to a single topic and keep track of whatever comes on that topic rather than subscribe to a new topic for every marker that might possibly be visible.

bmagyar commented 9 years ago

Hi, and thanks for the contribution!

I was actually planning to remove the 2 nodes that are there and have a single one that can be configured to one's needs. With your addition there would be 3 nodes that share most of the code and logic, which is harder to maintain and might cause confusion for a newcomer.

On the other hand I would still like to merge parts of your pull request. I will do what I planned then get back to this PR.

In the mean time if you have specific requirements regarding output and/or configuration, you could open an issue for them.

jdlangs commented 9 years ago

The single node plan is definitely a much better idea than maintaining separate nodes with shared code between them. I didn't have to change much to be able to publish all the markers.

What is your thought about introducing specialized aruco message types? It seemed like the most straightforward way to me to combine the id and pose info, but maybe there is a better option.

bmagyar commented 9 years ago

I like the idea a lot, but then I had a second thought... What if we could use the object_recognition_msgs/RecognizedObject

It may be a bit verbose but perhaps it's better to use a message type that is already out there. I don't remember seeing one that is like the marker you put there but let me know if you know of one.

bmagyar commented 9 years ago

Or maybe we could add that message to the object_recognition_msgs.

jdlangs commented 9 years ago

The question I would have is is there existing code that uses the object_recognition pipeline and would make sense to run on aruco markers without changes? Seems like the ObjectType field assumes there is some database present. If it wouldn't be compatible with existing code, I'm not sure it is good practice to have semantically different topics with the same message type going around.

bmagyar commented 9 years ago

That is true. ar_pose also contains it's own message type for markers which I think could be shared... http://docs.ros.org/hydro/api/ar_pose/html/msg/ARMarker.html

Or maybe for now we could go with your message, then consider changing. Could you make a new PR with only the messages and the related update in the build and package config?

jdlangs commented 9 years ago

That AR marker type is definitely compatible. It would be ideal if they moved it to a separate msgs package, which would serve as a light dependency for both augmented reality toolkits. For now, I've changed my previous Marker.msg format to match the fields in the ARMarker message. So if they get unified down the road, it will be a pretty trivial change for anyone using them.

I'll submit the new pull request shortly