image_pipeline
image_pipeline copied to clipboard
[Iron] Improve maintainability of calibration code
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.
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.
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.
Understood. Will do the linting first, think my auto-formatter did it > PR for rolling
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:
- 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).
- 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.
- 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).
- 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 I've just opened a PR #1000
Probably you can add to it?
Closing this one, PR in rolling
in merged https://github.com/ros-perception/image_pipeline/pull/1000