dkogan / mrcal

Next-generation camera-modeling toolkit
http://mrcal.secretsauce.net
Apache License 2.0
196 stars 16 forks source link

Pinhole model support in C API for mrcal_rectified_system #30

Open akonneker opened 1 month ago

akonneker commented 1 month ago

See here

I just stumbled over this while embedding mrcal rectification map calculation into some C++ code. I had previously used rectified_system with a pinhole model in the python api.

Do you have a timeline for when you might add this support? Or perhaps you have some advice about an easier workaround than just using the python API?

dkogan commented 1 month ago

Hello. This is very low priority for me since pinhole rectification is strictly worse than latlon rectification in every way: https://mrcal.secretsauce.net/stereo.html

If you need this, you should implement it! It wouldn't be that difficult, and I would appreciate the contribution. Port the relevant logic in mrcal._rectified_system_python() to the C mrcal_rectified_system() function. There already exists a test that makes sure that the not-yet-written port is correct: test-rectification-maps.py.

Thanks!

akonneker commented 1 month ago

I'm happy to add the functionality, but can you help me understand how that test works to call the c function? As-is it happily passes, when I would expect it to fail if it actually called the c version of the function.

akonneker commented 1 month ago

Ah, I suppose I need to undo this behavior in stereo.py: https://github.com/dkogan/mrcal/blob/71c89c4e9f268a0f4fb950325e7d551986a281ec/mrcal/stereo.py#L511

dkogan commented 1 month ago

Yep. The python function mrcal.rectified_system() is the outer logic, and it makes sure to call the Python code when using pinhole models: https://github.com/dkogan/mrcal/blob/71c89c4e9f268a0f4fb950325e7d551986a281ec/mrcal/stereo.py#L422

So you're remove that if statement entirely. Right after that, it calls mrcal._mrcal._rectified_system(), which routes to the C code.

akonneker commented 1 month ago

So I have it implemented, and test-stereo.py and test-rectification-maps.py are now passing.

The primary issue is how to replace the root-finding provided by numpy with something in c. I found this as a possible solution: https://github.com/yairchu/quartic. It's BSD licensed and only two files (for the c implementation), so it seems to me to be a reasonable solution to include the files directly in mrcal, but that's up to you to decide.

I found another issue when working on this: there are missing docstring files that are referenced in mrcal-pywrap.c that don't exist in the repo: measurement_index_points_triangulated num_measurements_points_triangulated decode_observation_indices_points_triangulated

I just commented those out to move forward, but I thought you should know.

dkogan commented 1 month ago

akonneker @.***> writes:

So I have it implemented, and test-stereo.py and test-rectification-maps.py are now passing.

Cool!

The primary issue is how to replace the root-finding provided by numpy with something in c. I found this as a possible solution: . It's BSD licensed and only two files (for the c implementation), so it seems to me to be a reasonable solution to include the files directly in mrcal, but that's up to you to decide.

Solving a quartic should be a standard-enough thing that we shouldn't need to pull in any weird libraries. My suggestion would be to

I found another issue when working on this: there are missing docstring files that are referenced in mrcal-pywrap.c that don't exist in the repo: measurement_index_points_triangulated num_measurements_points_triangulated decode_observation_indices_points_triangulated

Yeah. This is one of the many things that will eventually end up in mrcal 3.0. It's nowhere near done, and the missing docstrings are a part of that.

I just commented those out to move forward, but I thought you should know.

That's ok for now. Once you have a workable patch we'll need to figure out what to do. That could be you using the current mrcal git (which would include your code), or maybe I should release mrcal 2.5 at some point that will include the stuff that's done.

dkogan commented 2 weeks ago

Hi. Are you still trying to do this? Do you need help?

akonneker commented 1 week ago

Sorry for the delayed response. My quick hack satisfied my initial need and I've been pretty busy at work since then. I'll come back to this in another week or so when things slow down.