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

Fix positioning of multitarget struct in pipelineresult unpack #1181

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

theVerySharpFlat
Copy link
Contributor

@theVerySharpFlat theVerySharpFlat commented Jan 21, 2024

In the c++ library, the Multitarget structure is unpacked right after the latency double, which is out of place. Currently, it's supposed to be the last struct in the bytes. This incorrect unpacking order essentially makes it such that the rest of the data (targets, etc) are "corrupted".

I think the order in the java library was changed in #1058 and the c++ library wasn't updated.

TLDR; I fixed the unpacking order to match the current pipelineresult data layout.

@theVerySharpFlat theVerySharpFlat requested a review from a team as a code owner January 21, 2024 05:45
@mcm001
Copy link
Contributor

mcm001 commented Jan 21, 2024

So the order changed between Java and C++, meaning our unit tests didn't catch it? Sounds like our tests are pretty lame. If you have bandwidth it would be awesome to fix that as well

@theVerySharpFlat
Copy link
Contributor Author

So the order changed between Java and C++, meaning our unit tests didn't catch it? Sounds like our tests are pretty lame. If you have bandwidth it would be awesome to fix that as well

Sure! That would certainly fix future errors

@mcm001
Copy link
Contributor

mcm001 commented Jan 21, 2024

I would try to mirror python where we have a list of bytes we decode and then assert things on, maybe? But I'm happy to add that later and merge this now.

@theVerySharpFlat
Copy link
Contributor Author

I would try to mirror python where we have a list of bytes we decode and then assert things on, maybe? But I'm happy to add that later and merge this now.

Ok. Sounds good. I need to add another commit that fixes the order in the encode function

@mcm001 mcm001 merged commit 939283d into PhotonVision:master Jan 22, 2024
23 checks passed
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