autowarefoundation / autoware.universe

https://autowarefoundation.github.io/autoware.universe/
Apache License 2.0
937 stars 614 forks source link

Casting between unsigned char * and const float * in perception #6979

Closed veqcc closed 3 months ago

veqcc commented 3 months ago

Checklist

Description

In perception/image_projection_based_fusion/src/pointpainting_fusion/node.cpp, there are several castings from unsigned char to float:

As the CppCheck warnning shows, this conversion may not be correct. For example, even if unsigned char c = 10, the casted value is not 10.0f.

Here is my simple test program:

int main() {
    unsigned char i = 10;
    {
        unsigned char * ptr_i = &i;
        float f = *reinterpret_cast<const float *>(ptr_i);
        printf("%f\n", f);
    }
    {
        float f = (float)i;
        printf("%f\n", f);
    }
}

Output:

0.000000
10.000000

This is because the casted float value is really small.

10 as unsigned char has 0x00001010 bit expression. This is casted to 32 bit flaot value 0x 0 00000000 00000000000000000001010. And it is interpreted as the following: image

Question

I have one question: is it a expected behavior? I know that sometimes the bit expressions are more important than the actual values.

Expected behavior

See Description.

Actual behavior

See Description.

Steps to reproduce

See Description.

Versions

No response

Possible causes

No response

Additional context

No response

soblin commented 3 months ago

As described here http://docs.ros.org/en/melodic/api/sensor_msgs/html/msg/PointCloud2.html, the point cloud.data field is the array of 8bits and a specific chunk of the part of array represents float values of XYZ, namely [stride i, stride i + offset_x + offset_y + offset_z] bytes.

https://autowarefoundation.github.io/autoware-documentation/main/design/autoware-architecture/sensing/data-types/point-cloud/

I think your example is not correct because the test data is a single uchar.

veqcc commented 3 months ago

@soblin Thank you for your quick review!

As described here http://docs.ros.org/en/melodic/api/sensor_msgs/html/msg/PointCloud2.html, the point cloud.data field is the array of 8bits and a specific chunk of the part of array represents float values of XYZ, namely [stride i, stride i + offset_x + offset_y + offset_z] bytes.

I'm sorry I missed the document. I understand your point and it makes sense.

soblin commented 3 months ago

@veqcc I think we should explicitly write something like clang-format off in that part to suppress error reports from sanitizers tools.

veqcc commented 3 months ago

Agreed 👍