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

Plane 260: Fix flipping #51

Merged
merged 9 commits into from
Dec 29, 2024
Merged

Plane 260: Fix flipping #51

merged 9 commits into from
Dec 29, 2024

Conversation

maxspier
Copy link
Member

Fixes the time hog that the other vertical flipping algorithm took. This reverts those changes by using a more simple OpenCV method and adds the ability to flip in two directions to the frontend instead of just one.

Copy link
Member Author

@maxspier maxspier left a comment

Choose a reason for hiding this comment

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

Ltgm on my side after quick readthrough - though I would appreciate if someone could test on Orin before merging.

@maxspier maxspier requested a review from cgpadwick December 24, 2024 23:18
Copy link
Collaborator

@cgpadwick cgpadwick left a comment

Choose a reason for hiding this comment

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

great discovery about cv::flip, I bet this goes a lot faster than the previous method. some small changes requested

@maxspier maxspier requested a review from cgpadwick December 26, 2024 18:55
@maxspier
Copy link
Member Author

@cgpadwick if you wouldn't mind building/testing on Orin before merging - much appreciated!

@maxspier maxspier requested a review from cgpadwick December 27, 2024 06:08
Copy link
Collaborator

@cgpadwick cgpadwick left a comment

Choose a reason for hiding this comment

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

some pretty interesting geometrics here - reflections and rotations are different and a reflection of an apriltag isn't an apriltag, but a double reflection is!

src/ws_server.cu Outdated
flipVertical(&bgr_img);
}
if (flipHorizontal_) {
flipHorizontal(&bgr_img);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some experiments last night to figure out what was going on. The results are in this truth table:

tag detected vertical flip horizontal flip
true false false
false true false
false false true
true true true

If no checkboxes are clicked the tag is detected. If one checkbox is clicked and the other one is not, the tag is not detected, but if both of them are clicked the tag is detected.

It took me a few minutes to figure out why! I had to print out an apriltag and flip it and rotate it in various ways to convince myself of what was happening. Basically a mirror image of an apriltag is not an apriltag pattern. but a mirror image of a mirror image is. So checking both boxes is equivalent to a 180 degree rotation, while only clicking one box is just a reflection.

Think of it this way - when you take an apriltag and flip it vertically with cv::flip, you're actually looking at it upside down from behind. It's as if you took the apriltag and flipped it down on the bottom edge and now you're looking at the back of the apriltag. It;'s upside down and backwards from the original tag.
And when you apply the horizontal flip, now you're flipping the tag over along the right hand edge. So now you are looking at the front again, and it is upside down from it's original orientation, so its equivalent to a 180 degree rotation.

You might have to print this out and try it!

So the fix here is to have one call to cv::flip which uses the flip code of -1 to do both directions simultaneously. Does this make sense? We can do a video meet and discuss if you like, or I can show you in person at the next robotics session.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes perfect sense. I can sort of visualize it in my head! I just pushed a fix for the -1 option and will look into what P.V. does for their other 90 degree rotations

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is looking good now. what do you want to do about the ui options? I'm concerned that if we leave the two options on there that we will get to a competition and forget to click both of them because we'll be tired and then the vision system won't work and we'll be scrambling. How about just a single option for now and we figure out 90 degree rotation later if we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to leave it as two for now but both are required to be checked for it to work. Then, we can leave it for easy dev later. I'm also going to add a warning in the UI that both must be clicked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also -- best case scenario is we place the cameras correctly so we don't even need to worry about this feature :)

@maxspier maxspier requested a review from cgpadwick December 29, 2024 05:24
Copy link
Collaborator

@cgpadwick cgpadwick left a comment

Choose a reason for hiding this comment

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

approved on the condition that we come back later and fix the dual checkbox issue!

@cgpadwick cgpadwick merged commit 056940a into main Dec 29, 2024
1 check passed
@cgpadwick cgpadwick deleted the PLANE-260 branch December 29, 2024 18:16
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.

2 participants