Only recreate target object when parameters change#86
Conversation
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
==========================================
- Coverage 79.16% 78.38% -0.78%
==========================================
Files 9 9
Lines 499 504 +5
==========================================
Hits 395 395
- Misses 104 109 +5
Continue to review full report at Codecov.
|
auman66
left a comment
auman66
left a comment
There was a problem hiding this comment.
Are there any other cases you might have missed where you would wan't to clear/recreate the target?
| case moveit_handeye_calibration::HandEyeTargetBase::Parameter::ParameterType::Float: { | ||
| QLineEdit* line_edit = new QLineEdit(); | ||
| connect(line_edit, SIGNAL(textChanged(QString)), this, SLOT(targetParameterChanged(QString))); | ||
| target_param_inputs_.insert(std::make_pair(param.name_, line_edit)); | ||
| target_param_layout_->addRow(param.name_.c_str(), target_param_inputs_[param.name_]); | ||
| static_cast<QLineEdit*>(target_param_inputs_[param.name_])->setText(std::to_string(param.value_.f).c_str()); | ||
| break; |
There was a problem hiding this comment.
This case looks the exact same as above. Can you combine?
There was a problem hiding this comment.
I went to do this and realized that there's a one character difference, in lines 313 and 321: one uses param.value_.i and the other param.value_.f. I'm not thinking of a more elegant way to combine them . . .
There was a problem hiding this comment.
You could make it a lambda function that lives inside loadInputWidgetsForTargetType function and have that one parameter be the input value. And make that parameter const std::string& to avoid having to template it for different value types.
There was a problem hiding this comment.
I don't quite follow your suggestion, @jliukkonen, but I did realize that the parameter could know how to make a string of itself, and so that's what I did and used it here to combine the int and float cases. It also led me to some other situations where things could be simplified related to parameter types.
jliukkonen
left a comment
jliukkonen
left a comment
There was a problem hiding this comment.
I would prefer std::atomic<bool> if possible simply because it can be used the same way as your ordinary bool, which makes it easier to understand.
| case moveit_handeye_calibration::HandEyeTargetBase::Parameter::ParameterType::Float: { | ||
| QLineEdit* line_edit = new QLineEdit(); | ||
| connect(line_edit, SIGNAL(textChanged(QString)), this, SLOT(targetParameterChanged(QString))); | ||
| target_param_inputs_.insert(std::make_pair(param.name_, line_edit)); | ||
| target_param_layout_->addRow(param.name_.c_str(), target_param_inputs_[param.name_]); | ||
| static_cast<QLineEdit*>(target_param_inputs_[param.name_])->setText(std::to_string(param.value_.f).c_str()); | ||
| break; |
There was a problem hiding this comment.
You could make it a lambda function that lives inside loadInputWidgetsForTargetType function and have that one parameter be the input value. And make that parameter const std::string& to avoid having to template it for different value types.
3 participants
Fixes #14: only re-initializes target object when the parameters change.