-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Odd and fragile code for Transformations #164
Comments
I agree with your analysis. I would assume that the apply concept was wrongly transferred from the However, I would fix it differently because if only chains of size 1 can be applied there is no need for chains. Which way to go for the library is @pomerlef to decide. Currently this might take some days AFAIK. |
I do agree that the API itself needs modification. Chains here make no sense. |
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 The way I would see the schedule:
|
OK, here's a pull request for the simple check. #165 |
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).
The text was updated successfully, but these errors were encountered: