search

UbiquityRobotics / fiducials

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

Parameters not namespaced with ~ since initialized before init_node #243

Closed dorkamotorka closed 3 years ago

JanezCim commented 3 years ago

Seems ok to me visually, but i have never used aruco_gazebo package, so can you please tell me how to test this and how is this package different from aruco_detect?

MoffKalast commented 3 years ago

It's generally supposed to be a drop in replacement for aruco_detect, but instead it uses gazebo data to emulate the tf positions and FiducialArray msgs. Should be enough to just launch it instead of aruco, but it hasn't been confirmed to work too reliably so far.

dorkamotorka commented 3 years ago

It has worked reliably for me, since the whole breadcrumb Gazebo is based upon it.

dorkamotorka commented 3 years ago

sim_ground_fiducials.launch actually uses it.

MoffKalast commented 3 years ago

Ah that's great to hear, after I initially wrote it Rohan or whoever tested it said that something wasn't quite right, but I suppose you must've solved the issue somewhere along the way 👍

dorkamotorka commented 3 years ago

Ah that's great to hear, after I initially wrote it Rohan or whoever tested it said that something wasn't quite right, but I suppose you must've solved the issue somewhere along the way

Not that I would know ;)

dorkamotorka commented 3 years ago

I always placed the fiducials by hand. But I think Nikola is placing them(programatically) in Gazebo with Constraint Random Verification.

JanezCim commented 3 years ago

Can you then please tell me how you place them by hand? I cant satisfy the parsing algorithm in the script.

dorkamotorka commented 3 years ago

I have it on the sidebar in Gazebo. Click on them and place it. image

JanezCim commented 3 years ago

Okay I looked at this again and there seems to be some unconventional stuff happening in the code. Maybe I dont understand it enough.

1.) after inserting the markers from here: https://github.com/UbiquityRobotics/breadcrumb/tree/indigo-devel/breadcrumb_gazebo/models into gazebo, and launching roslaunch aruco_gazebo aruco_gazebo.launch im still getting output:

...
[INFO] [1615466006.379645, 729.348000]: Detected 0 markers
[INFO] [1615466006.500124, 729.441000]: Got data 4913
[INFO] [1615466006.523358, 729.456000]: Detected 0 markers
[INFO] [1615466006.610347, 729.532000]: Got data 4914
[INFO] [1615466006.657213, 729.570000]: Detected 0 markers
[INFO] [1615466006.744403, 729.630000]: Got data 4915
[INFO] [1615466006.760245, 729.632000]: Detected 0 markers
[INFO] [1615466006.891804, 729.730000]: Got data 4916
[INFO] [1615466006.923225, 729.750000]: Detected 0 markers

Seems like the parser here https://github.com/UbiquityRobotics/fiducials/blob/20b09e6e938f2c02095a1d237861df12b8eba3d1/aruco_gazebo/scripts/aruco.py#L105 does not detect the markers inside the /gazebo/model_states topic

am i doing something wrong?

2.) why are you subscribing to /gazebo/model_states inside a subscriber callback? https://github.com/UbiquityRobotics/fiducials/blob/20b09e6e938f2c02095a1d237861df12b8eba3d1/aruco_gazebo/scripts/aruco.py#L98

3.) This line seems unnecessary to me since you don't have an image subscriber in the code https://github.com/UbiquityRobotics/fiducials/blob/20b09e6e938f2c02095a1d237861df12b8eba3d1/aruco_gazebo/launch/aruco_gazebo.launch#L18

4.) hardcoded line 152

MoffKalast commented 3 years ago

the parser here

Yeah it's probably too specific, might be better to just extract any number contained in the string.

why are you subscribing to /gazebo/model_states inside a subscriber callback?

Well it's not just any callback, it's /camera_info which will only publish once. That will then start the subscriber once we have the required camera info to proceed, and that callback is where we do the actual pose republishing. It's quite an elegant way to wait before starting imo.

This line seems unnecessary to me since you don't have an image subscriber in the code

It was copied over from aruco_detect since it was meant to be as a drop-in replacement, but yeah has no actual functionality in the current implementation.

4.) hardcoded line 152

Yep, that does need to be fixed.

dorkamotorka commented 3 years ago

I will address a rewrite of aruco_gazebo in a new issue in breadcrumb, since we will also have to create meshes for stag markers.

JanezCim commented 3 years ago

Well it's not just any callback, it's /camera_info which will only publish once. That will then start the subscriber once we have the required camera info to proceed, and that callback is where we do the actual pose republishing. It's quite an elegant way to wait before starting imo.

Ok, that is true, but then what happenes if /camera_info gets published more times (eg. camera driver gets restarted)?

MoffKalast commented 3 years ago

Ok, that is true, but then what happenes if /camera_info gets published more times (eg. camera driver gets restarted)?

Yeah that would cause some issues, but it would also require a restart of gazebo, which usually also restarts the roscore and requires the rest of the nodes to be relaunched anyway. So unlikely to actually ever cause any issues in practice.

But it could be fixed with a simple "if subscriber is registered, unregister, then register again" I think.

JanezCim commented 3 years ago

Sure. What I believe is the standard is to just have a simple global variable eg. camera_info_callback_called that is set to true on the info callback and then you just dont continue with /gazebo/model_states callback if its not true. But i may be wrong

MoffKalast commented 3 years ago

Imo either is perfectly valid. There's more than one way to skin a cat, as they say.

JanezCim commented 3 years ago

Ok, so whats the plan for fixes here? we merge this and someone creates a bugfix branch, or will someone fix here and then we can test again?

dorkamotorka commented 3 years ago

I would merge this and address the problems in the new issue I opened.