search

erget / StereoVision

Library and utilities for 3d reconstruction from stereo cameras.
http://erget.github.io/StereoVision
GNU General Public License v3.0
648 stars 268 forks source link

Enforcing 5 seconds delay, Adding timer overlay. #39

Closed sebinjude closed 4 years ago

sebinjude commented 4 years ago

The capture_chessboards utility is really wonderful. I would like to suggest a few additions to make it a bit more robust and appealing. The changes are as follows:

  1. Enforced the 5 second delay, now independent of the executing machine the delay would be approximately 5 seconds.

  2. Added a timer overlay on the cam stream. This really helps to properly re-orient the chess_board. Furthermore it helps the user to avoid motion blur.

  3. Two more windows (selected windows) were added to show the snapshot of the last screen captured (both left and right). I believe this will help in keeping a mental map of the chess_board orientations. The snapshot images are marked with the corners (drawChessboardCorners), to help in gaining a fair approximation as to whether the corners are being detected properly. This will also provide a better visual appeal.

  4. Added a function to provide better starting positions to the windows. The position is not enforced.

I hope these additions would be beneficial. Please do share your thoughts on same.

erget commented 4 years ago

Thanks for this, it looks useful!

Have you checked the new code with respect to the changed API in stereo_cameras.ChessboardFinder.get_chessboard? I don't have an IDE set up where I'm situated right now so I can't check to ensure that no references are broken. It might be that ChessboardFinders are used elsewhere in the package.

Also, could you add docstrings to the new methods? I see that you've got one on capture_chessboard.enforce_delay but I'm missing one on the others. I know they're fairly short and one could consider it a pretty obvious no-brainer, but I've got the consistency / documentation disease and have a deep love for docstrings ;)

Other than that I'd be interested if you've tested this on a live system (I don't have a rig to hand).

sebinjude commented 4 years ago

Thanks for this, it looks useful!

Have you checked the new code with respect to the changed API in stereo_cameras.ChessboardFinder.get_chessboard? I don't have an IDE set up where I'm situated right now so I can't check to ensure that no references are broken. It might be that ChessboardFinders are used elsewhere in the package.

Also, could you add docstrings to the new methods? I see that you've got one on capture_chessboard.enforce_delay but I'm missing one on the others. I know they're fairly short and one could consider it a pretty obvious no-brainer, but I've got the consistency / documentation disease and have a deep love for docstrings ;)

Other than that I'd be interested if you've tested this on a live system (I don't have a rig to hand).

Thank you for reviewing the changes.

I have re-verified that ChessboardFinder.get_chessboard method is only being invoked from capture_chessboards.main method. Hence the references are intact.

As suggested I have made sure that all methods have doc strings.

Tested the capture_chessboards module with a stereo rig. I was able to capture and save the calibration images. However the calibrate_cameras module encountered a few OpenCV related errors. So as a work-around I made a quick python3 with OpenCV 4 translation of the calibrate_cameras module. As a consequence end to end testing (ie. capture and calibrate) was not performed. Yet I believe it is safe to merge these changes as the capture_chessboards module and calibrate_cameras are decoupled.

Finally adding a helper function to approximate the scale factor of the timer text.

Please share your thoughts on the same.

Once again Thank you very much for the quick response. :-)

erget commented 4 years ago

This looks good, I guess porting to Python3 with OpenCV 4 is a bigger job ;)