image_pipeline icon indicating copy to clipboard operation
image_pipeline copied to clipboard

[Iron] Improve maintainability of calibration code

Open MRo47 opened this issue 9 months ago • 5 comments

For #973

  • calibrator.py file is almost 1.5K lines long and difficult to navigate.
  • This could break dependencies? I'd like to know more if this is true.
  • Ive fixed the imports in the remaining files.

MRo47 avatar Apr 29 '24 16:04 MRo47

I see there is some discussion of this in a ticket as well - but I just want to note that a PR of this size/scale should really target Rolling and then be backported as needed to earlier releases.

mikeferguson avatar Apr 30 '24 02:04 mikeferguson

Another thought: it appears that a number of the changes are basically just linting - I would actually suggest to create a standalone PR that is just linting changes (because that will be much easier to review) and then put the structural changes in a follow up PR.

mikeferguson avatar Apr 30 '24 02:04 mikeferguson

Understood. Will do the linting first, think my auto-formatter did it > PR for rolling

MRo47 avatar Apr 30 '24 11:04 MRo47

As it happens, I recently rewrote most of the code in the image_pipeline in order to make it cleaner for internal use at my company and also incorporate some features for thermal cameras. I chose to rewrite the code because there were several large structural changes that made it very hard to work with:

  1. The ROS interface is heavily interwoven with OpenCV. For example, all of the OpenCV GUI functionality is embedded in a OpenCVCalibrationNode, making it hard to add additional GUI functionality. Ideally, this package should be a ROS package that provides an interface for a completely independent python calibration library, which itself is just a wrapper for OpenCV (plus a few helper functions to, for instance, ensure good sample-point distribution and provide a GUI).
  2. The class structure does not capture the structure of the calibration process well, so there is a lot of duplication between classes. This is most notable in the StereoCalibrator and MonoCalibrator classes.
  3. The file names and general file structure could be a lot better. One glaring example is that there is both cameracalibrator.py and also camera_calibrator.py. However, most of the files could be organized better according to their functionality (but first should make changes 1 and 2).
  4. Lots of small readability issues, like passing around the sample distribution as a list(zip(self._param_names, min_params, max_params, progress)) as opposed to using a named dictionary or other object. 5.I think there is an opportunity here to also address a number of other Issues/enhancements that have been posted, like #785.

Unfortunately due to export restrictions I cannot simply share my code, and at this point I would actually have to request access to see it again. However, if someone is making large structural changes to this package I would be interested in helping. I have some time right now so I can help with both broad discussion of things I found to be most broken, and also writing code if someone else (@MRo47 or @mikeferguson) can help.

arthurlovekin avatar May 30 '24 18:05 arthurlovekin

@arthurlovekin I've just opened a PR #1000

Probably you can add to it?

MRo47 avatar Jun 12 '24 18:06 MRo47

Closing this one, PR in rolling in merged https://github.com/ros-perception/image_pipeline/pull/1000

ahcorde avatar Aug 20 '24 08:08 ahcorde