Skip to content

Conversation

@giragon6
Copy link

Add config to select which apriltags are valid

@giragon6 giragon6 requested a review from kjaget November 18, 2024 22:52
constexpr int LEGAL_TAGS[16] = {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16};

// Load config for legal tags
std::vector<int> LEGAL_TAGS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this PR was just extending the previous code which worked with a vector. But there's a potentially better way to do this.

C++ has a std::set data structure which is a perfect fit for this. It's a structure which simply tracks members of a set. In this case, you'd set it up with the tags IDs that are valid, and then in the detection loop just check that .count() is non-zero (it's going to be either 0 or 1, the .count() function is used by the container to be compatible with other data structures where count can be any value).

This set will take a slight bit of extra work to set up (read the param into a vector, then loop to .emplace() each entry from that vector into the set) but the lookup will be quicker and easier to read.

Speaking of which, it looks like the code currently re-reads the param each time an image is processed. Instead, this param read should be made part of the ApriltagDetectorNode constructor and the set data structure stored as a class member. No need to re-read the config value every image when it won't change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the suggested changes to apriltag_detector_node -- it seems to be working based on my testing -- and currently working on the GPU port. Thanks so much for your help!

@kjaget
Copy link
Member

kjaget commented Nov 20, 2024

One more idea, once this is working for the CPU apriltag code, port it over to the code in gpu_apriltag/src/gpu_apriltag_nodelet.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants