Fix https://github.com/3dtof/voxelsdk/issues/93

[philips-thilo]

As reported in issue #93, the vector/array lookup in the getDirection function does a wrong lookup, as row and col were confused in the formula. This can cause wrong projections or segmentation faults.

[larrylisky]

@theten85: I do not see an issue with the original implementation:

   return direction[col * width + row];

'col' will iterate from 0..height-1, resulting in a correct array dimension of width*height.

With your proposed change,

    return directions[row * width + col];

'row' will iterate 0..width-1, will resulting in an array of width*width.

[philips-thilo]

Hi @larrylisky,

col will not iterate from [0..height-1] but [0..width-1] as it is the column or x or u dimension. You might have swapped the meaning of row and col, as your row range is [0..width-1] while it should be [0..height-1]. You can also have a look at PointCloudTransform::depthToPointCloud, here the access is valid.

Below you will find a proof of that error. Assume that you have an image where rows corresponds to number of rows, therefore height of an image cols corresponds to number of columns, therefore width of an image Assume an image where width > height, so the smallest possible width is (height + 1)

Therefore we will have the following column index range: [0.. (height+1)] If you insert this into the old formula for the first row you will get directions[(height + 1)*(height + 1) + 0] = height^2 + 2*height + 1 The number of pixels in the image can be computed as follows in this general case: numPixels = width * height = (height + 1) * height = height^2 + height If you subtract these two formulas, you get the number of pixels the old formula is out of bounds, which will be height + 1

If you now insert the typical values for the OPT 8241, 320x240, trying to convert the 241st pixel (col = 240, row = 0) will try to access index (240 * 320 + 0 = 76800) while the size of the array is 76800. Therefore the access will be out of bounds as it tries to access the first element behind the array.

[larrylisky]

@theten85 I agree PointCloudTransform::depthToPointCloud() access is valid. As to your assertion that 'col' spans the range [0..width-1], I would have to disagree. In PointCloudTransform::_init(), directions[] array is filled width first, so the correct array access would be direction[col*width+row]. I don't quite follow your proof.

Now, you're probably disputing what is passed into PointCloudTransform::getDepth() through the 'col' argument. Please take a look at line 343-343 of PointCloudTransform::imageToWorld(), 'y' is indexed from [0..height-1]; and in PointCloudTransform::calcAperatureAngleRadians(), 'y' is passed into getDepth() through the 'col' argument.

[philips-thilo]

@larrylisky PointCloudTransform::_init() fills the array in the following way: for every row all the columns are filled. Basically you can organize your image in two ways - but from the work you have done I guess you know that already.

Quote from wikipedia (https://en.wikipedia.org/wiki/Row-major_order)

row-major order, consecutive elements of the rows of the array are contiguous in memory column-major order, consecutive elements of the columns are contiguous

Accessing array elements directly

row-major order: pixelLocation = dataLocation + (column * height + row)
column-major order: pixelLocation = dataLocation + (row * width + column)

Apparently the SDK maintainers have chosen column-major order, as is the case with OpenCV. You are addressing a good point with PointCloudTransform::calcAperatureAngleRadians(). But I have to disagree with your statement, as y is used for row, and x is used for column, correct and consistently with the column-major order throughout this class.

Therefore I don't see any problem with the change :)