PointCloudUtils does not compile

[claudiofantacci]

Describe the bug
Calling yarp::sig::utils::depthRgbToPC with T2 class type differentr from either yarp::sig::PixelRgba or yarp::sig::PixelBgra, for example

yarp::sig::DataXYZRGBA pointcloud = yarp::sig::utils::depthRgbToPC<yarp::sig::DataXYZRGBA, yarp::sig::PixelRgb>(img_depth, img_color, camera_intrinsics);

gives the following error

error C2039: 'a': is not a member of 'yarp::sig::PixelRgb'

Expected behavior
Visual Studio compiler is not able to optimize, i.e. remove with branch hints, the following if statement

yarp/src/libYARP_sig/include/yarp/sig/PointCloudUtils-inl.h

Lines 46 to 49 in ec09c81

	if (std::is_same<T2, PixelRgba>::value ||
	std::is_same<T2, PixelBgra>::value) {
	pointCloud(u,v).a = color.pixel(u,v).a;
	}

while other compilers do (gcc, clang).

Configuration (please complete the following information):

• OS: Windows 10
• yarp version: 3.1.100+20181113.21+gitd93264183
• compiler: cl toolchain v141

cc @Nicogene

[traversaro]

@S-Dafarra was describing a simiilar problem during lunch yesterday.

[claudiofantacci]

The only solution for now is to either copy-paste the content of PointCloudUtils-inl.h (that is visible to users) and customize/specialize it for the particular cases Windows does not work properly.
This may also be a possible solution on YARP side.

Unfortunately if constexpr is a C++17 feature, otherwise we would already have a solution.

[S-Dafarra]

I think this is a slightly different issue. What I experienced was that Visual Studio was failing to substitute the following template (https://github.com/S-Dafarra/levi/blob/e5ad65b5a6/include/levi/OperatorsEvaluables.h#L76-L77):

template<typename Scalar_lhs, int lhsRows, int lhsCols, typename Scalar_rhs, int rhsRows, int rhsCols>
struct levi::matrix_product_return<Eigen::Matrix<Scalar_lhs, lhsRows, lhsCols>, Eigen::Matrix<Scalar_rhs, rhsRows, rhsCols>

The problem was that Eigen::Matrix has some additional template parameters which can be defaulted. When trying to call matrix_product_return<Matrix1, Matrix2> Visual Studio was assuming that all the parameters I was not setting should have been all equal (I actually don't know what the standard says in this regard). The fix was to add also those parameters in the template specialization.

Unfortunately if constexpr is a C++17 feature, otherwise we would already have a solution.

Once I saw an answer on stackoverflow that I am not able to find, but it worked basically in this way.

• Define a struct templated with a bool

template<bool value>
struct bool_value{}

• Define a function which takes bool_value as a parameter. By specializing the true and false case you have the same effect of the if

template<bool value>
void copyA(bool_value<value> /*, other parameters*/);

void copyA(bool_value<true> /*, other parameters*/) {
// True case
}

void copyA(bool_value<value> /*, other parameters*/) {
// False case
}

• Use the function

copyA(bool_value</*condition to check*/>(), /*other parameters*/)

[claudiofantacci]

@S-Dafarra I agreee that they are two different problems.

What you suggest is useful, but will lead to two different implementation of a function and in the end we could just implement a template specialization for PixelRgb types (without the alpha channel).

[Nicogene]

We analyzed with godbolt the compilation result of this code snippet:

// Type your code here, or load an example.
int square(int num) {
    int pippo = 2;
    if(false) {
        pippo=5;
    }
    return num * num;
}

And the code inside the if is removed by clang and gcc by default, on the other hand msvc removes it only with the compiler optimization /Ox.

[Nicogene]

I should add a test for this case and see if the compilation in windows is still faling.

[PeterBowman]

I have just stepped into this issue on Linux. Config:

• Ubuntu 18.04.5 LTS (Bionic), kernel 5.4.0-47-generic
• YARP version 3.4.0+28-20200911.5+gitad5857e2e
• g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
• building in Release configuration (-O3 compiler flag)

[drdanz]

A "broken" testcase would be quite useful, in case someone wants to prepare it...

[PeterBowman]

Just changing PixelBgra to PixelBgr in this one would illustrate the problem:

yarp/tests/libYARP_sig/PointCloudTest.cpp

Lines 915 to 933 in 308c8bb

	SECTION("Testing depthToPC")
	{
	ImageOf<PixelFloat> depth;
	size_t width{320};
	size_t height{240};
	depth.resize(width, height);
	IntrinsicParams intp;
	auto pc = utils::depthToPC(depth, intp);
	CHECK(pc.width() == depth.width()); // Checking PC width
	CHECK(pc.height() == depth.height()); // Checking PC height
	ImageOf<PixelBgra> color;
	color.resize(width, height);
	auto pcCol = utils::depthRgbToPC<DataXYZRGBA, PixelBgra>(depth, color, intp);
	CHECK(pcCol.width() == depth.width()); // Checking PC width
	CHECK(pcCol.height() == depth.height()); // Checking PC height
	}

But "broken" here means "don't compile at all", not "make test fail".

[drdanz]

Added the broken testcase, see e264068

It would be nice to use if constexpr, but it requires c++17 which is not yet the enabled, therefore I fixed it using SFINAE, see #2363

[drdanz]

Fixed by #2363