Refactored output parameter.

[lewie-donckers]

Refactored TrackingPidLocalPlanner::computeVelocityCommands() to use a return value instead of an output parameter.

The function never read from the output parameter (it was pure output and not in-/output), and the results were one of the following:

• false and the output parameter was not set
• true and the output parameter was set

This maps perfectly to std::optional. Either the function is successful and it returns the velocity command or it is not successful and it does not return a velocity command.

[lewie-donckers]

I'm a bit puzzled. I created what I think was a very innocuous PR which is in-line with a best practice which is widely supported within the C++ community.

Yet, for such a small change, I get quite a bit of pushback from both @Rayman and @Timple . So I was hoping you two would explain to me what you don't like about this. So I can perhaps address your concerns or refrain from these kind of changes in the future.

Is it the principle of no output parameters itself, the usage of std::optional, something else?

[Timple]

PR comments often sound a bit harsh as good stuff is rarely complemented :slightly_smiling_face: . I actually liked to learn about the std::optional.

My comment was that you are refactoring a function which doesn't describe it's actual use. But these kind of comments can simply be split-off to issues and the PR can continue: https://github.com/nobleo/path_tracking_pid/issues/120

[Rayman]

So I was hoping you two would explain to me what you don't like about this.

I think we were hoping and suggesting some bigger changes to the error handling that are needed, while you made a small improvement to existing code. I sometimes use a review to comment on code that surrounds the block of code because that's the first time I'm really looking at it. I shouldn't do that and focus more on the current review.