Add export functionality for 3D points and a visualization tool#55
Add export functionality for 3D points and a visualization tool#55abrock wants to merge 2 commits into
Conversation
…he 3D points of all calibration markers
…rojection errors computed by other software
|
Thank you very much for the PR. I will have a look at it when I have time. |
puzzlepaint
left a comment
puzzlepaint
left a comment
There was a problem hiding this comment.
Thanks again for this PR and sorry that it took me so long to review it. I like the added functionality, but there are a few things that I would prefer to have changed before merging this. In case you don't want to do the changes (judging by myself, I don't like revisiting and changing old PRs ...) I might do it myself at some point.
| // Save the optimization state | ||
| if (state_output_path) { | ||
| SaveBAState(state_output_path, *state); | ||
| SaveDatasetAndState(state_output_path, *dataset, *state); |
There was a problem hiding this comment.
Saving the dataset to the state_output_path each time that the state is saved would be a behavior change. I think that such a behavior should either be behind a specific flag (e.g., save_dataset_with_state) or the state_output_path should be renamed to fit its changed semantic.
|
|
||
| if (dataset_output_path) { | ||
| SaveDataset(dataset_output_path, *dataset); | ||
| SaveDatasetAndState(dataset_output_path, *dataset, *state); |
There was a problem hiding this comment.
Here, a similar comment applies as above, since this would now always save the state with the dataset. Also for the two instances below.
| return true; | ||
| } | ||
|
|
||
| bool CreateCalibrationReportForData(const char* base_path, |
There was a problem hiding this comment.
This seems to somewhat duplicate the code of CreateCalibrationReportForCamera(). I believe that, as far as possible, it would be much preferable to use only a single code path for both.
| return result; | ||
| } | ||
|
|
||
| bool SaveDatasetAndState(const char* path, const Dataset& dataset, const BAState& state) { |
There was a problem hiding this comment.
This seems to duplicate all of the code from SaveDataset(), with some additional things interleaved. I think that it would be much preferable to avoid code duplication, for example by making the additional parts optional behind a flag.
Also, unless I am overlooking something, this does not actually save the state, so the name "SaveDatasetAndState()" seems to be misleading.
| } | ||
|
|
||
|
|
||
| unordered_map<int, Vec2i>* feature_id_to_coord; |
There was a problem hiding this comment.
This seems to be unused and the comment below seems to be left over.
| & feature_id_to_coord); | ||
| //*/ | ||
|
|
||
| for (const auto& item : calibration.feature_id_to_points_index) { |
There was a problem hiding this comment.
This looks like a copy of a few lines above? So this seems like it saves the same data to the file twice after each other.
| // Save the optimization state after each iteration | ||
| SaveBAState(model_output_directory.c_str(), state); | ||
|
|
||
| SaveDatasetAndState(model_output_directory.c_str(), dataset, state); |
There was a problem hiding this comment.
The comments above about saving state and dataset together also apply here.
| return io->Write(image_file_name, *this); | ||
| } | ||
|
|
||
| template<> |
There was a problem hiding this comment.
I'd suggest to move this functionality to specialized functions that say explicitly which format they are saving, for example, "WriteAsGnuPlot()".
2 participants
I implemented #43 (first commit).
I think the visualization of error directions and magnitude is great and I wanted to use it for visualizing results from other calibration methods. I also wanted to use different color scales for the visualization so I added methods to export Image to text files I can process with gnuplot, and Image to optical flow files in the Middlebury format (https://vision.middlebury.edu/flow/data/).