Move the init of static variables to library load time

[kunaltyagi]

Possibly fixes #1593 (race condition in OMP implementation of SHOT)

[kunaltyagi]

Wow, seems like apple-clang doesn't keep the lambdas unique

[vseib]

Dear all,
this looks like a valid solution to #1593, thanks @kunaltyagi
I am not sure if I am allowed to approve a pull request, therefore I just create a review with a general comment.
Best,
Viktor

[mvieth]

@kunaltyagi What do you think about moving the RGB2LAB functionality to the common module? There is a function in gicp6d that basically does the same (without LUT).

[kunaltyagi]

Sounds like a good idea. I'll take a deeper look either tomorrow or after the weekend.

[kunaltyagi]

@vseib @mvieth PTAL This should still remove the race condition 🤞🏾 (static vars are specially protected by the standard), while allowing code reuse.

Added a bunch of references as well which I hunted down while making sure I wasn't messing up anything major

[kunaltyagi]

@larshg Need help on windows 😄

[larshg]

It seems MSVC has a bug with constexpr and lambda - see here.

Setting them to just static instead of constexpr, then it works - not sure if that is sufficient?

Alternatively, you could add an additional definition of constexpr const std::size_t size = 1 << bits; within the lambda.

[kunaltyagi]

Thanks @larshg. That worked wonders

[larshg]

Thanks @larshg. That worked wonders

Great😅

[kunaltyagi]

I'll have to check this error on Windows before merging:

test\registration\test_registration.cpp(619): error: Expected: (reg.getFitnessScore ()) < (0.003), actual: 0.00387298 vs 0.003

[kunaltyagi]

I'm unable to test why Windows is having such a large error in registration. On linux machines, the error is approx 4e-5, much lower than on Windows (reducing the LUT size used in GICP6D increases error, but going to 1024 increases it to only 4.5e-5)

Any ideas? Is this due to difference in c math lib?

[mvieth]

I'm unable to test why Windows is having such a large error in registration. On linux machines, the error is approx 4e-5, much lower than on Windows (reducing the LUT size used in GICP6D increases error, but going to 1024 increases it to only 4.5e-5)

Any ideas? Is this due to difference in c math lib?

Perhaps because the second LUT is of float type, not of double? Not really a good explanation, but something to test out. Else you could try to replace the second LUT with a normal computation as it was before to check if that is the problem.
BTW I originally had something different in mind, maybe something like a simple class that creates the LUTs in the constructor and has a RGB2LAB function. SHOT and GICP6D could each have such an object as a class member, and use the conversion function when needed. I think with the current solution, in RGB2Lab in GICP6D, the LUTs are computed every time the function is called, aren't they? I think that would ultimately be worse than without LUTs.
But if you don't want to implement such a class and only focus on fixing the race condition in this PR, that would be fine, too.

[kunaltyagi]

the LUTs are computed every time the function is called, aren't they?

This shouldn't be the case. The LUT are static variables inside the function. They are created on the first function call, and in a thread-safe manner (guarantee of language). Once init, they are returned safely.

Re: adding a class for easy conversion
IMO, the class for converting between colors should be left for another time, since colors aren't something that PCL specializes in either.

Re: float vs double
I'll push an update with double LUT since it's a plausible explanation. :) Let's see.

[mvieth]

Ok, from my side, this is good to merge 👍