ros-perception / image_pipeline

An image processing pipeline for ROS.
Other
762 stars 715 forks source link

[Iron] Improve maintainability of calibration code #975

Open MRo47 opened 2 months ago

MRo47 commented 2 months ago

For #973

mikeferguson commented 2 months ago

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 commented 2 months ago

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.

MRo47 commented 2 months ago

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

arthurlovekin commented 1 month ago

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.

MRo47 commented 3 weeks ago

@arthurlovekin I've just opened a PR #1000

Probably you can add to it?