Odd and fragile code for Transformations

[oleg-alexandrov]

The code for applying to a transformation to a cloud is the following: (https://github.com/ethz-asl/libpointmatcher/blob/master/pointmatcher/Transformation.cpp#L59)

//! Apply this chain to cloud, using parameters, mutates cloud
template
void PointMatcher::Transformations::apply(DataPoints& cloud, const TransformationParameters& parameters) const
{
DataPoints transformedCloud;
for (TransformationsConstIt it = this->begin(); it != this->end(); ++it)
{
transformedCloud = (*it)->compute(cloud, parameters);
swapDataPoints(cloud, transformedCloud);
}
}

Please correct me if I am wrong. The idea here is that the iterator it point to the desired type of transform, be it similarity transform, or orthogonal transform, etc. Then, the transform itself, given by the "parameters" variable that is passed as input will be applied to the cloud.

There is a problem though. Nobody checks that there is exactly one iterator between this->begin() and this->end(). If there is more than one, the transform given in "parameters" will be applied to the cloud more than once, which is just wrong. I ran into this issue actually when in my case the set of iterators it was empty, and no transform was applied. That was hugely confusing.

In short, there must be a check here I think that there must be exactly one iterator between this->begin() and this->end().

I wonder if my interpretation is correct. If you agree with me, I will try to study this in more depth and put in a pull request (the check itself).

[HannesSommer]

I agree with your analysis.

I would assume that the apply concept was wrongly transferred from the DataPointsFilters, to the Transformations and never properly tested.

However, I would fix it differently because if only chains of size 1 can be applied there is no need for chains.
So one fix would be to remove the Transformations class entirely. Another would be to require as many TransformationParameter objects as there are transformations in the chain as arguments for compute, but it doesn't seem obvious how to do that nicely and intuitively with c++. I guess the most intuitive way here would be to change the Transformations class such that it actually stores transformation parameters for each transformation - maybe mutable from outside. The apply method would then not get any extra parameters in addition to the mutable cloud. This way the overall behaviour would be much more in line with the DataPointsFilters class. One could leave a function taking an additional TransformationParameter object and checking the chain length to be one, as you suggested, for backward compatibility over some transition phase.

Which way to go for the library is @pomerlef to decide. Currently this might take some days AFAIK.

[oleg-alexandrov]

I do agree that the API itself needs modification. Chains here make no sense.

[pomerlef]

Alright, I'm trying to slowly catching up with the maintenance...

Thanks @oleg-alexandrov to point that out. The idea was to offer the possibility to the user to build a list of transformations and to apply all of them with one call. Not much thoughts were put into that as we dealt with only one Transformations class for many years. Now that I'm thinking about it, it was most probably an over optimisation as I'm not sure if it make sense at all. There is a tight connection between the Transformations class used and the ErrorMinimizer used. Since only one ErrorMinimizer is allowed, it should be the same for the Transformations.

The way I would see the schedule:

1. Quickly fix the fragile part of the issue by adding a check there for only one Transformation
2. It make sense to have a major release this year with some API modifications to fix the odd part of the issue. Rethinking how the transformations are setup would be useful. Does it even make sense to have it setup by the user as the ErrorMinimizer would dictate what TransformationParameter are used and thus which Transformation needs to be applied.

[oleg-alexandrov]

OK, here's a pull request for the simple check. #165