Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roi for tags #7645

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Roi for tags #7645

wants to merge 5 commits into from

Conversation

GGreenix
Copy link

@GGreenix GGreenix commented Jan 6, 2025

This is a ROI implementation for the Apriltags detector to lower processing time

@GGreenix GGreenix requested a review from a team as a code owner January 6, 2025 16:06
@github-actions github-actions bot added the component: apriltag AprilTag library label Jan 6, 2025
Copy link
Member

Choose a reason for hiding this comment

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

Remove this file.

Copy link
Member

Choose a reason for hiding this comment

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

Remove this file.

@@ -86,6 +95,7 @@ void AprilTagDetector::SetQuadThresholdParameters(
qtp.max_line_fit_mse = params.maxLineFitMSE;
qtp.min_white_black_diff = params.minWhiteBlackDiff;
qtp.deglitch = params.deglitch;

Copy link
Member

Choose a reason for hiding this comment

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

Remove the spurious newlines added in this PR.

for (int y = 0; y < orig->height; y++) {
for (int x = 0; x < orig->width; x++) {

//TO-DO add check of roiX and roiY
Copy link
Member

Choose a reason for hiding this comment

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

Are these going to be done in this PR? If so, this PR should be marked as a draft.

Also, IDEs typically highlight TODO instead of TO-DO.

A space should be included between // and its content.

Copy link
Author

Choose a reason for hiding this comment

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

Excuse me I didn't quite understand what you want with this one, I just marked it for me to remember, I can delete it if that's a problem but I intent on making it part of the PR

Copy link
Member

Choose a reason for hiding this comment

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

Since you still have more to add, I'll convert this PR to a draft.

@calcmogul
Copy link
Member

What does ROI mean in this context?

@KangarooKoala
Copy link
Contributor

What does ROI mean in this context?

Based on #7583 (which was opened by the same person), I would guess Region Of Interest? I'd still want to hear confirmation from the PR author though.

@GGreenix
Copy link
Author

GGreenix commented Jan 6, 2025

What does ROI mean in this context?

Yes hi, I want to implement a ROI for the tasks that process the image at C level, x,y,w,h which will shorten processing time

Copy link
Contributor

Choose a reason for hiding this comment

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

You should combine patches 9 through 12 into one commit. (Follow https://github.com/wpilibsuite/allwpilib/blob/main/upstream_utils/README.md#adding-patch-to-thirdparty-library, but instead of making a commit, run git rebase -i 3806edf38ac4400153677e510c9f9dcb81f472c8 and replace "pick" with "fixup" at the start of the lines with "fixed names for roi variables", "change names of roi variables", and "test".

@calcmogul calcmogul marked this pull request as draft January 6, 2025 21:17
…that were detected outside of the wanted region

Need to polish it a bit but the main thing is that I only filter quads after they detected because I tried to make changes into the code itself with starting the iterators and until from given ROI variables
@GGreenix
Copy link
Author

GGreenix commented Jan 7, 2025

I will address your comments but I want to finish the code first before managing all the things around it, there are still ways to save on processing time if anyone could help It would be gladly appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: apriltag AprilTag library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants