[registration] It might be better to reorganize the inheritance relationship.

[QiuYilin]

The current base class Registration seems to use an ICP algorithm by default. The algorithm is roughly divided into correspondence estimation and transformation estimation. Some rough alignment algorithms do not seem to be suitable for inheritance based on this, because some members of the base class are not used.

like FPCS：

In principle, this inheritance relationship may be more reasonable:

Of course, it can also be distinguished based on the purpose: coarse registration and fine registration, rigid registration and non-rigid registration. I think it is safer to follow the principle relationship. There may be cross-ambiguity based on the purpose.

Currently, ICP can use the strategy mode to switch between the methods in correspondence estimation and optimal transformation estimation, which is very convenient. If there are newly introduced algorithms, they can also be split into two independent algorithms. However, based on my understanding, the ICPwithNormals type can be roughly understood as ICP based on point-to-plane, and ICPNonliear is ICP based on LM point-to-point. There seems to be no clear distinction between these subclasses.

[mvieth]

The current base class Registration seems to use an ICP algorithm by default. The algorithm is roughly divided into correspondence estimation and transformation estimation. Some rough alignment algorithms do not seem to be suitable for inheritance based on this, because some members of the base class are not used.

I don't really understand your logic here: Registration does not perform a correspondence estimation, nor does it perform a transformation estimation. It only has members to store the objects, correspondence_estimation_ and transformation_estimation_. And FPCS for example uses the transformation_estimation_ member inherited from Registration and fills it with TransformationEstimation3Point. The main purpose of the Registration class is that all inheriting classes have to implement computeTransformation(PointCloudSource& output, const Matrix4& guess), and some variables used by multiple inheriting classes are bundled in Registration (like transformation_estimation_, max_iterations_, ...). Yes, not every inheriting classes uses all member variables of Registration, but I don't think this is a major problem.

In principle, this inheritance relationship may be more reasonable:

The main difference would be that FPCS would additionally inherit from RANSAC_IA? Is RANSAC_IA SampleConsensusInitialAlignment? Or a new class? Because SampleConsensusInitialAlignment uses features, which FPCS does not, so I don't see why this relationship would make sense.

I think it is safer to follow the principle relationship. There may be cross-ambiguity based on the purpose.

Sorry, I am not really sure what you mean with this.

However, based on my understanding, the ICPwithNormals type can be roughly understood as ICP based on point-to-plane, and ICPNonliear is ICP based on LM point-to-point. There seems to be no clear distinction between these subclasses.

For IterativeClosestPointNonLinear I completely agree that the same result can be achieved with IterativeClosestPoint and setTransformationEstimation. ICPwithNormals might be mainly a "convenience class" to set these configurations easily. There may also be historical reasons for these classes, i.e. when they were added there may have been a good reason to have them, but after some code restructuring these reason are less clear. But this is just a guess.

Generally speaking, every change to the inheritance relationship of PCL classes has a certain risk that we unintentionally break something for PCL users (something has worked before but doesn't work now). Of course, this must be avoided. So for a change like this, there must be a good reason, a clear advantage, for example after the change users can do something that was not possible before. And honestly, so far I do not see/understand the advantage in a reorganized inheritance.

[QiuYilin]

I don't really understand your logic here: Registration does not perform a correspondence estimation, nor does it perform a transformation estimation. It only has members to store the objects, correspondence_estimation_ and transformation_estimation_.

I mean ICP can be understood as a combination of correspondence_estimation_ and transformation_estimation_, but other registration methods may not be able to do so.

some variables used by multiple inheriting classes are bundled in Registration (like transformation_estimation_, max_iterations_, ...). Yes, not every inheriting classes uses all member variables of Registration, but I don't think this is a major problem.

I think this is actually a problem. Users cannot know whether these parameters need to be set except by looking at the source code and the specific principles. They usually think that the parameters set in the example are not all.

The main difference would be that FPCS would additionally inherit from RANSAC_IA? Is RANSAC_IA SampleConsensusInitialAlignment? Or a new class? Because SampleConsensusInitialAlignment uses features, which FPCS does not, so I don't see why this relationship would make sense.

My idea is that it is a new class besides ICP. Sorry, I don't know enough about coarse registration. Anyway, I want to express that we should remove the members that are not used except ICP in the current registration base class.

This is just an initial suggestion, and I apologize for proposing it without a complete plan. Further classification of the registration module requires extensive knowledge in this area, which is actually very difficult.

[QiuYilin]

For IterativeClosestPointNonLinear I completely agree that the same result can be achieved with IterativeClosestPoint and setTransformationEstimation. ICPwithNormals might be mainly a "convenience class" to set these configurations easily. There may also be historical reasons for these classes, i.e. when they were added there may have been a good reason to have them, but after some code restructuring these reason are less clear. But this is just a guess.

At present, from my personal point of view, ICP can be such a relationship, without considering GICP.