[sfm] Reduce code duplication in sequential sfm triangulation #1217
Conversation
|
Unrelated question: I saw a lot of code added with 2-space style indentation as well as 4-space style indentation. Is any of these preferred and new code should be written in that style with the intention that whole codebase will be migrated eventually? |
|
Yes that's the idea, new code should be 4-space style. |
src/aliceVision/sfm/pipeline/sequential/ReconstructionEngine_sequentialSfM.cpp
Outdated
Show resolved
Hide resolved
src/aliceVision/sfm/pipeline/sequential/ReconstructionEngine_sequentialSfM.cpp
Outdated
Show resolved
Hide resolved
src/aliceVision/sfm/pipeline/sequential/ReconstructionEngine_sequentialSfM.cpp
Outdated
Show resolved
Hide resolved
|
What's the status? |
|
@p12tic Could you have a look to Simone's review? |
p12tic
force-pushed
the
sequential-sfm-duplication
branch
from
f2e45e9
to
e59df01
Compare
|
@fabiencastan Sorry for taking so long to address the issues. Everything is fixed now. |
|
|
||
| if (!camPinHole) { | ||
| ALICEVISION_LOG_ERROR("Camera is not pinhole in triangulate_multiViewsLORANSAC"); | ||
| return {nullptr, nullptr, {}, {}, {}, {}}; |
There was a problem hiding this comment.
| return {nullptr, nullptr, {}, {}, {}, {}}; | |
| return {nullptr, {}, {}, {}, {}}; |
as the raw pointer has been removed.
I was also wondering if it'd be worth it to have a method isEmpty(){return cam == nullptr} in ObservationData so that it is easier to test the result. What do you think?
There was a problem hiding this comment.
Yeah, didn't commit everything. I agree about new method.
There was a problem hiding this comment.
Fixed now. I also changed indentation of new code.
p12tic
force-pushed
the
sequential-sfm-duplication
branch
2 times, most recently
from
ee9544b
to
d17f870
Compare
The current code recomputes the same source data for both 2 and N observation cases. Due to the code being structured differently it's hard to see that in both cases the same operations are being performed. By extracting the code into separate function we make this fact explicit.
p12tic
force-pushed
the
sequential-sfm-duplication
branch
from
d17f870
to
6090f3a
Compare
fabiencastan
changed the title
This PR includes a relatively simple and small refactor.
The current code recomputes the same source data for both 2 and N observation cases. Due to the code being structured differently it's hard to see that in both cases a lot of the same operations are being performed. By extracting the code into separate function we make this fact explicit.