I'm working on a vision sensor that provides depth data for each pixel through its API. The buffer is ordered row by row, left to right, and bottom to top, see below. The image has a resolution of width by height.
imgae
I need to reorder the depth data list for transformation purposes, taking into account the resolution. For example, if bottomLeftData = 1 2 3 4 5 6
, I need to iterate in reverse to get topLeftData = 5 6 3 4 1 2
. Note, this is not merely reversing per se. It involves two operations. For example, for width=2, I iterate bottomLeftData
in reverse to extract 6 5
and reverse this order to get 5 6
. I will continue all over bottomLeftData
like this.
My solution is
#include <iostream>
#include <vector>
#include <algorithm>
using namespace std;
int main()
{
vector<double> a = {1,2,3,4,5,6};
int w(2),h(3);
for (int i=0; i < a.size(); ++i)
cout << a[i] << ' ';
cout << endl << endl;
k=a.size();
vector<double> c;
for(int i=0; i < h; ++i){
vector<double> b;
for(int j=0; j < w; ++j)
b.push_back(a[--k]);
std::reverse(b.begin(), b.end());
for (int h=0; h < b.size(); ++h)
c.push_back(b[h]);
}
for (int i=0; i < c.size(); ++i)
cout << c[i] << ' ';
cout << endl << endl;
return 0;
}
Are there better methods to accomplish this task? Note that the resolution can be large.
-
\$\begingroup\$ Are height 4 and width 3 examples? What are the actual dimensions of the problem? \$\endgroup\$Davislor– Davislor2025年09月07日 04:06:35 +00:00Commented Sep 7 at 4:06
-
\$\begingroup\$ Your problem description is unclear. Do you actually need another vector with all the elements in reverse order? Or do you just want to iterate over the elements of an existing vector in reverse order? This is Code Review, not Stack Overflow; we need your actual code to give proper reviews, not a "minimum viable example". \$\endgroup\$indi– indi2025年09月07日 13:35:02 +00:00Commented Sep 7 at 13:35
-
1\$\begingroup\$ Presumably the actual problem here involves more than one pixel. You only specify a fixed-sized toy problem, for which views of a vector are overkill. What are the dimensions of the actual problem? \$\endgroup\$Davislor– Davislor2025年09月07日 16:10:03 +00:00Commented Sep 7 at 16:10
-
1\$\begingroup\$ @CroCo Rotation and translation are both affine, and vertical reversal is also affine, so: don't do any of this. Write a single affine matrix that includes the flip. \$\endgroup\$Reinderien– Reinderien2025年09月08日 00:17:27 +00:00Commented Sep 8 at 0:17
-
1\$\begingroup\$ Your edit invalidated an answer, which breaks the question-and-answer nature of this site. See What should I do when someone answers my question?. I've reverted the change for you so that the answer makes sense again. \$\endgroup\$Toby Speight– Toby Speight2025年09月08日 07:01:44 +00:00Commented Sep 8 at 7:01
4 Answers 4
Actual problems with your code
Your code does not compile, as
k
is not declared.std::size_t
is probably a better type for array dimensions thanint
. This will also help you with a number of warnings. Make sure you're compiling with warnings turned on/up. To take this a step further, compile with warnings treated as errors, to force yourself to fix them.
Opportunities for improvement
Variables are often best declared on individual lines. There's very little room for confusion in your specific program, but when we combine this with pointers, the syntax can be misleading. E.g.
int* a, b;
is equivalent toint *a; int b;
rather thanint *a; int *b;
using namespace std;
is strongly discouraged.Your first loop:
for (int i=0; i < a.size(); ++i) std::cout << a[i] << ' ';
is better written as a range-based for loop.for (const auto &x : a) std::cout << x << ' ';
Your code would be improved by utilizing braces consistently. This can prevent common sources of confusion. Your use of indentation is good, though!
On the subject of consistency, you should use whitespace consistently around operators. You may wish to employ an automatic formatting tool.
std::endl
does rather more than simply outputting a newline. It also flushes the output stream. Maybe you want to do this to prevent out of sync I/O, but that doesn't look like an issue. You would be better off simply pushing'\n'
or"\n\n"
to the output stream.main
is a special case and returns0
automatically on ending, making an explicitreturn 0;
a bit extraneous.Your variable names aren't very meaningful.
w
is probably better aswidth
, for instance.Use
const
orconstexpr
when you know variable will not change. This will let the compiler help you by causing errors if you make a mistake and change them.
A first revision
Taking some of these thoughts into consideration, we might have a program that looks like the following, and compiles without warnings.
#include <iostream>
#include <vector>
#include <algorithm>
int main()
{
const std::vector<double> a {1,2,3,4,5,6};
constexpr std::size_t w {2};
constexpr std::size_t h {3};
for (const auto &x : a) {
std::cout << x << ' ';
}
std::cout << "\n\n";
std::size_t k {a.size()};
std::vector<double> c;
for (std::size_t i {0}; i < h; ++i) {
std::vector<double> b;
for (std::size_t j {0}; j < w; ++j) {
b.push_back(a[--k]);
}
std::reverse(b.begin(), b.end());
for (const auto x : b) {
c.push_back(x);
}
}
for (const auto &x : c) {
std::cout << x << ' ';
}
std::cout << "\n\n";
}
A second revision
But really we can streamline this with some math and a pair of loops which populate a new vector b
. The intermediate vectors are gone, so there's no need for a vector c
.
#include <iostream>
#include <vector>
int main()
{
const std::vector<double> a {1,2,3,4,5,6};
constexpr std::size_t w {2};
constexpr std::size_t h {3};
for (const auto &x : a) {
std::cout << x << ' ';
}
std::cout << "\n\n";
std::vector<double> b;
for (std::size_t i {h}; i > 0; --i) {
const std::size_t offset {(i-1) * w};
for (std::size_t j {0}; j < w; ++j) {
b.push_back(a[offset + j]);
}
}
for (const auto &x : b) {
std::cout << x << ' ';
}
std::cout << '\n';
}
The output:
1 2 3 4 5 6
5 6 3 4 1 2
Don't repeat yourself
You may also wish to factor out the printing of the vectors to avoid pointless repetition and make it possible to change your printing in one place.
#include <iostream>
#include <vector>
template <typename T>
void print_vector(const std::vector<T> &v) {
for (const auto &x : v) {
std::cout << x << ' ';
}
std::cout << '\n';
}
int main()
{
const std::vector<double> a {1,2,3,4,5,6};
constexpr std::size_t w {2};
constexpr std::size_t h {3};
print_vector(a);
std::vector<double> b;
for (std::size_t i {h}; i > 0; --i) {
const std::size_t offset {(i-1) * w};
for (std::size_t j {0}; j < w; ++j) {
b.push_back(a[offset + j]);
}
}
print_vector(b);
}
-
2\$\begingroup\$ Why write ugly, complicated (and slow!) nested loops? Why not just use
std::ranges::reverse_copy()
orstd::views::reverse | std::ranges::to<std::vector>()
? \$\endgroup\$indi– indi2025年09月07日 13:32:38 +00:00Commented Sep 7 at 13:32 -
1\$\begingroup\$ That's a great idea too. I wanted to demonstrate iterative improvements to the OP's code. \$\endgroup\$Chris– Chris2025年09月07日 14:32:56 +00:00Commented Sep 7 at 14:32
-
\$\begingroup\$ you are right about
int k=0;
. I just added it. \$\endgroup\$CroCo– CroCo2025年09月07日 17:47:33 +00:00Commented Sep 7 at 17:47 -
2\$\begingroup\$ @CroCo: Why
k=0
right before you assign it a different value? The normal thing would besize_t apos = a.size();
to initialize it with the value you want it to have. (And give it a meaningful name, and give it the standard type for an index into a std::vector, the same type you'd get fromauto apos = a.size();
. Hopefully you're trying to not invalidate the answers by using their feedback when updating the question, so using int like other vars... Normally it's discouraged to update the question after answers are posted, but making it actually compile when it didn't is probably fine.) \$\endgroup\$Peter Cordes– Peter Cordes2025年09月08日 06:48:21 +00:00Commented Sep 8 at 6:48 -
\$\begingroup\$ You should reserve space in those vectors to avoid reallocating repeatedly with
push_back()
. \$\endgroup\$Cris Luengo– Cris Luengo2025年09月12日 00:17:20 +00:00Commented Sep 12 at 0:17
Is This an XY-Problem?
Frame challenge: You describe needing to iterate through the rows in reverse order, while keeping the columns of each row in the same order, as input to a transformation algorithm.
Can You Access the Data in the Correct Order?
If you have control over the transformation algorithm, you can instead access the elements in a different order, such as:
// Width w and total size n are already computed, w <= INT_MAX, and n <= PTRDIFF_MAX.
for (ptrdiff_t i = n - w; i >= 0; i -= w) { // i must be SIGNED and wide enough!
for (int j = 0; j < w; ++j) { // j should have the same signedness as i.
const auto pixel_ij = *(buffer + i + j);
/* ... */
You could instead slice the array into rows with std::span
or std::ranges::subrange
as your slice type, and pass those slices to algorithms that use the range interface.
An alternative using ranges and adapters is to reverse
a chunk
view, as in Toby Speight’s excellent answer, but instead of collecting the data to
a vector
, flatten the view.
In a more complicated convolution, you might separately calculate the addresses of two input rows, counting down here and up elsewhere, possibly as array slices or std::span
, then iterate over the elements of the two rows in parallel.
Can You Permute in Place?
You can apply the algorithm for reversing an array in place to an array of rows. That is, you can find the addresses of the rows starting from the top and counting down, and the addresses of the rows starting from the bottom and counting up, swap the two rows element-wise, and stop when the addresses meet in the middle.
Feedback on the Code as Written
Make The Permute Operation a Function
Not only is this better organization that putting everything in main
, it lets you chain the returned vector, such as transform(permutePixelRows(getRawImage(camera)))
.
You might accept the input as a std::vector<double>&&
that can be permuted in place, then return the permuted input vector (by move). Alternatively, it could accept an input range and an output range, and return the output range.
Optimizing with Vector
First, you want to avoid allocating a std::vector
off the heap within each iteration of the loop, as in:
for(int i=0; i < h; ++i){
vector<double> b;
This slows the loop down immensely and inhibits vectorization. Since each row has the same width, if you need a temporary buffer to hold a row, you can allocate it once before the loop, and overwrite it on each iteration.
Consider taking the output buffer as a parameter (possibly std::span<double>
, which can alias any array-like contiguous data structure, or you could make it a template
for any output range that supports std::begin(dest)
and std::end(dest)
). This lets the caller fill any range of memory it owns.
If you stick with creating your own std::vector
to hold the output data, it is faster than push_back
to resize
to the known final size, obtaining an initialized output buffer, and copy or move the input data to the correct offsets within the output buffer.
In 2025, inserting one element at a time at the end causes compilers to actually call the non-trivial push_back
code each time, and copying to a pre-allocated output buffer does not. If you must build your output as you go, at least copy a row of data at a time to minimize the number of bounds checks. This is explicitly what Toby Speight’s excellent solution (which is very easy on the eyes) does.
However, you could also use the access pattern in the first code sample to fill an empty std::vector
in order, which could give you simple code.
However you do, you probably want to call shrink_to_fit
before you return, since you almost certainly will not be appending any more elements to the buffer.
Everything that Chris said, plus:
Reserve vectors when we have a good idea how big they'll become:
c.reserve(w * h);
Know your standard algorithms and views:
auto c = a | std::views::chunk(w) | std::views::reverse | std::views::join | std::ranges::to<std::vector>();
-
3\$\begingroup\$ Eh, I wouldn’t use
join
withto()
for this.join
loses the size information, so the vector cannot be constructed with a single allocation. It would be better to create a vector first, reserve the final size, thenassign_range()
into it, like so: godbolt.org/z/dPa9Ghjb3 \$\endgroup\$indi– indi2025年09月07日 23:03:00 +00:00Commented Sep 7 at 23:03 -
\$\begingroup\$ Actually, on second thought, it might even be better to construct the vector pre-sized to the correct size, and then use
std::ranges::copy()
to copy the actual data over it all. There would be a specious zeroing of the vector contents—sadlystd::vector
does not have a "resize-for-overwrite" function—but it might still be a faster solution. This is the point where only profiling will help. \$\endgroup\$indi– indi2025年09月08日 20:17:15 +00:00Commented Sep 8 at 20:17
Let me reframe how you index into your image, and this whole problem will go away.
You define the image as:
vector<double> data = {1,2,3,4,5,6};
int width = 2;
int height = 3;
I suggest you also need the strides, how to increase or decrease the index into data
to go to the next pixel along each dimension:
int h_step = 1;
int v_step = width;
And also an origin, the index of the pixel at (0,0):
int origin = 0;
Your loop over the pixels now is:
for (int i = 0; i < height; ++i) {
int index = origin + i * v_step;
for (int j = 0; j < width; ++j) {
std::cout << data[index] << ' ';
index += h_step;
}
std::cout << '\n';
}
std::cout << '\n';
[Note that I'd prefer to use std::size_t
and/or std::ptrdiff_t
over int
, but I left the int
in to keep changes from your code minimal.]
The double loop above prints the pixel values in address order, starting with the first one, which is considered to be at the bottom left of the image (we print downwards, so it shows upside-down here, ignore that):
1 2
3 4
5 6
You now want to flip the image, so that the pixel at index (0,0) is the top-left pixel. This requires updating the origin
to point to the top-left pixel, and the v_step
to move downwards:
origin = (height - 1) * v_step
v_step = -v_step;
The exact same double-loop above now outputs the data in the new order:
5 6
3 4
1 2
You didn't need to copy any data over, you didn't need to reorder or reorganize your pixels, all you needed to do was to specify a new origin and strides. This trick allows you to rotate the image over multiples of 90 degrees, mirror and flip the image as you please.
[Imaging libraries usually have origin
be a pointer rather than an index, which avoids one addition per pixel accessed, but this is a minor difference, as memory access generally takes more time than an addition.]
That said, I highly recommend that you use an existing image processing library to do all this stuff, so you don't need to reinvent the wheel.
-
\$\begingroup\$ This is really bad advice. Writing raw loops would be bad enough, but one could at least assume you’ve verified that
width * height <= data.size()
(I mean, you never actually check or even mention checking this but... we can at least assume). However, within the (nested!) loops, you then do a serious of rather complicated calculations on an index... including one assignment hidden in the loop control (!!!)... and at no point verify that you haven’t over/underflowed anything, or gone out of bounds. This is how nightmare bugs are made. \$\endgroup\$indi– indi2025年09月12日 23:51:20 +00:00Commented Sep 12 at 23:51 -
\$\begingroup\$ There is a reason the modern recommendation is to use algorithms or range views. THEY ARE SAFE. They are often "correct by construction". Take @TobySpeight’s code. So long as
w
is not<= 0
... which is trivial to check for before starting anything, that code will produce (basically) the same low-level operations as your manual loops, and is obviously, even at a glance, correct and safe. You want to handle different strides? Trivial. Arbitrary origin? Also trivial: use a span as a sub view. Or there’smdspan
of course. \$\endgroup\$indi– indi2025年09月12日 23:52:53 +00:00Commented Sep 12 at 23:52 -
\$\begingroup\$ Writing raw loops like this—with complex indexing calculations and *no checking at all!*—is not acceptable in 2025. \$\endgroup\$indi– indi2025年09月12日 23:53:19 +00:00Commented Sep 12 at 23:53
-
\$\begingroup\$ @indi Obviously one assumes that the inputs are checked. I just reworked OP’s loop. Sure the range-based loops are nice, and make it harder to make mistakes, but calling any of this indexing complex is a bit of a reach, I assume a rhetorical construct to get your point across. \$\endgroup\$Cris Luengo– Cris Luengo2025年09月13日 13:18:42 +00:00Commented Sep 13 at 13:18
-
\$\begingroup\$ @indi The "assignment hidden in the loop control" is an increment. We’re incrementing two variables in this loop, the loop counter and the index. Maybe it’s better to do that second increment in the loop body, but I find it reduces my cognitive load to do it in the control, because then the loop body is just the code that handles the one pixel. I’ll move it over. \$\endgroup\$Cris Luengo– Cris Luengo2025年09月13日 13:24:44 +00:00Commented Sep 13 at 13:24