Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Limelight megatag2 #73
Limelight megatag2 #73
Changes from 10 commits
9e563a1
20d9600
e8a613b
997ab27
f27ed96
ea164bb
4fa30b2
bd3a169
3ad4780
514a683
a51458e
7d21c52
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it clearer, instead of writing
4
useRED_SPEAKER_CENTER_TAG_ID
and similarly for7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it turns out that megatags work just fine, I highly recommend getting rid of old methods to reduce clutter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do after testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is within the context of this PR, but I'm pretty sure this only calculates the distance between the note and camera in the y direction relative to the robot, it doesn't account for the tx measurement from Limelight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly for
getDistanceToSpeakerMeters
. However if it turns out Megatags work just fine, you can probably just get rid of these methods except for the note one (you could make the note method use Megatags)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this falls under the scope of this PR, but something's weird with this math.
If this is something that would be better addressed in a different PR, feel free to ignore this comment, and I can just open an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, seems like the second involvement of meters should be outside a normal tangent, not an arctan?
Something like that, I second this possible issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if megatags work though, you don't have to do all this math. I'd say we see if megatags work and if so, we can probably scrap the math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double check that the signs are correct (to be clear, idk if they are correct, but people not understanding the coordinate system has caused bugs in the past)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what signs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the robot, left and right and forward and backward are sometimes swapped and rotated. Not that I remember which way the signs are, off the top of my head. Ask Big Brettle for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll flip signs if necessary after testing 🤷