Refactored output parameter.

[lewie-donckers]

Refactored the output parameter of Controller::findPositionOnPlan() to return value.

See:

• https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f20-for-out-output-values-prefer-return-values-to-output-parameters
• https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f21-to-return-multiple-out-values-prefer-returning-a-struct-or-tuple

Please note the following in Controller::update() where this function is used:

• instead of two very similar calls to findPositionOnPlan() in different branches of the if statement, we now have 1.
• now we declare path_pose_idx only once we have a value for it, so we can make it const.
• we use an immediately invoked lambda expression (IILE) to initialize current_tf_for_find_position. it has a complex initialization but with the IILE we can do so in one go and make the variable const. see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es28-use-lambdas-for-complex-initialization-especially-of-const-variables

[Timple]

These lambda functions don't really increase readability. But I get the point.

Although current_tf_for_find_position sounds like something that could be a simple request function on the class?

[Timple]

Actually, something is off here.

You're comment suggests a current_tf is requested. But there is also a variable current_tf. We should fix these names first.

Probably one tf points to base_link and the other to the tf that tries to follow the carrot. Right @cesar-lopez-mar ?

[lewie-donckers]

Yes, if I move the lambda to a member function we could add some documentation (that was present in the original implementation).

current_tf is used if config_.track_base_link, else the following is used:

    // find position of current position with projected carrot
    geometry_msgs::Transform current_with_carrot_g;
    tf2::convert(current_with_carrot_, current_with_carrot_g);

[cesar-lopez-mar]

These lambda functions don't really increase readability. But I get the point.

Although current_tf_for_find_position sounds like something that could be a simple request function on the class?

I agree that readability does not improve for all developers that might no be familiar with lambda functions (including myself). I would keep the comments of the original implementation in this function to help understanding the new code.

[cesar-lopez-mar]

Actually, something is off here.

You're comment suggests a current_tf is requested. But there is also a variable current_tf. We should fix these names first.

Probably one tf points to base_link and the other to the tf that tries to follow the carrot. Right @cesar-lopez-mar ?

Indeed current_tf points to base_link, and the other one to the carrot.

[Timple]

Not the carrot. But the ControlPoint.

[rokusottervanger]

I opened #87 for this. Referencing here to put it on the list ;)

[lewie-donckers]

Okay, I'm confused as to what the conclusion of this discussion is/should be. I need some help.

Would replacing the lambda with a member function suffice? If so, I would like some help in naming and documenting that function.

Or, is there some underlying problem that needs to be fixed and which is preventing us from continuing with this PR?

[Timple]

Yes, there is an underlying problem that the variable names are off. I propose we fix that first.

I hope a more clear solution direction will follow from that.

[rokusottervanger]

These variable names are really confusing indeed. current_tf also seems to imply that this is a tf2::Transform, but looking at this code, that cannot be the case (otherwise this wouldn't compile, as current_with_carrot_g is a geometry_msgs::Transform).

BTW, I like this refactor with the IILE

[cesar-lopez-mar]

I would preserve these kind of comments (after fixing the typo ;)), and move it to the new lambda function

[lewie-donckers]

I closed this one in favor of a new PR train to fix #123 .