BuildImageModel: use std::vector instead of numbered arguments #949
Conversation
|
Thanks, looks like a nice simplification. I added a small suggestion. |
|
Oh right of course, thank you for catching this! Looking at this again now, I think that I'll probably change BuildImageModel to just accept two output iterators and pass a nop output iterator for the line data if selection_mode is active. That way we never have to copy anything around at all. This will have to be after my exam later this week though. |
|
This version now seems like the simpler option to me to avoid a copy than doing anything with back_inserter. My usecase is that I'm displaying more information per image using additional lines, but I hope the change makes sense for you even in the standard setting. |
src/ui/model_viewer_widget.cc
Outdated
| LinePainter::Data* line4, LinePainter::Data* line5, | ||
| LinePainter::Data* line6, LinePainter::Data* line7, | ||
| LinePainter::Data* line8) { | ||
| std::vector<TrianglePainter::Data>& triangle_data, |
There was a problem hiding this comment.
output arguments should be pointers (Google C++ coding guideline).
There was a problem hiding this comment.
I'm not too familiar with the Google C++ coding guidelines, but it seems like this section here recommends references. Am I checking in the wrong place?
Or do you mean that I should consider line_data to be an optional output parameter and then remove the show_lines parameter and instead pass nullptr as line_data when not required?
There was a problem hiding this comment.
Apparently they updated their guidelines and I am pretty sure the old convention was to use pointers. In any case, it's the convention across the entire COLMAP code base, so we should follow it here as well. Thanks.
There was a problem hiding this comment.
Okay makes sense. In this case it's probably best to add null-checks and to remove the show_lines parameter. Same here, I'll fix this as soon as I get home.
src/ui/model_viewer_widget.cc
Outdated
| @@ -108,84 +105,76 @@ void BuildImageModel(const Image& image, const Camera& camera, | |||
| Eigen::Vector4f(-image_width, -image_height, focal_length, 1); | |||
|
|
|||
| // Image plane as two triangles. | |||
| triangle_data.reserve(triangle_data.size() + 2); | |||
There was a problem hiding this comment.
Remove this, as it will lead to extremely O(N^2) allocation cost.
There was a problem hiding this comment.
I might be wrong on this but I think it isn't O(N^2). We're reserving the total required size at the beginning of ModelViewerWidget::UploadImageData and ModelViewerWidget::UploadMovieGrabberData, which means that the reserve here is a noop. But I agree that this ends up being unnecessary in this case, if we assume that BuildImageModel will only be called from these places.
There was a problem hiding this comment.
And even if someone ends up using BuildImageModel without having reserved the total required size at the beginning, it probably still makes sense to keep it here because otherwise the allocation would happen during emplace back which isn't any better I guess.
There was a problem hiding this comment.
If we ever use this function somewhere else then this will end up as O(N^2). If the emplace_back() detects that the vector is out of capacity it will just increase the vector by a certain factor.
There was a problem hiding this comment.
Ah I see, you're right of course, I'll fix that tonight when I get home.
src/ui/model_viewer_widget.cc
Outdated
|
|
||
| if (show_lines) { | ||
| // Frame around image plane and connecting lines to projection center. | ||
| line_data.reserve(line_data.size() + 8); |
There was a problem hiding this comment.
Remove the reserve, same issue as above
|
Great, thanks very much and sorry for the delay in getting this merged. |
…p#949) * BuildImageModel: use std::vector instead of numbered arguments * BuildImageModel: avoid copy of line/triangle data * BuildImageModel: implement fixes suggested by ahojnnes
I needed to make this change anyway in my fork and it looks quite upstreamable so I might as well send you this PR. I hope that this is useful and that I'm not missing some obvious reason why the previous version was implemented like that.