Skip to content

Clean up code - #30

Open
Gold856 wants to merge 4 commits into
PhotonVision:mainfrom
Gold856:main
Open

Clean up code#30
Gold856 wants to merge 4 commits into
PhotonVision:mainfrom
Gold856:main

Conversation

@Gold856

@Gold856 Gold856 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Focuses on compatibility with newer mrcal and OpenCV versions, but also does some optimizations, like being more precise with headers, killing iostream, and removing 2-3 copies of point structures (which is actually a lot of memory to be shuffling around.)

Comment thread src/mrcal_wrapper.cpp
int *c_imagersizes = imagersize;
auto point_min_range = -1.0, point_max_range = -1.0;
mrcal_problem_constants_t problem_constants = {
.point_min_range = point_min_range, .point_max_range = point_max_range};

@Gold856 Gold856 Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you follow the logic for this, you'll see this struct is only accessed by function calls in an #ifdef 0 block, so this is never accessed. Future versions of mrcal empty the struct out entirely.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

penalty_range_normalization is only used in https://github.com/dkogan/mrcal/blob/c311b0acdb29d3f6c1a5abeaf17dc6a7e2ab10d9/mrcal.c#L5212 which has the comment:

TEMPORARY TWEAK: disable range normalization: I will re-add this later

But yeah, main doesn't have it at all. The if 0 was added in dkogan/mrcal@0e72718 , and was fully removed in dkogan/mrcal@e65c1b7 . "This is probably never coming back, so I'm removing the variables I'm not using anymore and the disabled code"

At the API layer there's no guarantee this is totally unused. mrcal.h makes no statement on if it's used or not. I think passing null will quickly cause a crash if he does try to access, so let's do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Harder for me but sure. I had to really go digging for that Python code though. (and there's no comment indicating it was taken from Python, the one that exists is further down.)

@Gold856

Gold856 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

This diff passes the tests in wpical on Windows.

Comment thread CMakeLists.txt
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