-
Notifications
You must be signed in to change notification settings - Fork 357
perf(kinbody): skip redundant GetDOFValues recompute in SetDOFValues #1564
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
base: production
Are you sure you want to change the base?
Changes from 8 commits
975ecd6
de84311
b08d9fc
fb0cbbe
5ef43a8
caa1cc9
5b018d9
09b1e1b
b3a7db8
3838d55
49e3d45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2271,27 +2271,44 @@ void KinBody::SetDOFValues(const dReal* pJointValues, int dof, uint32_t checklim | |
| if( dof == 0 || _veclinks.size() == 0) { | ||
| return; | ||
| } | ||
| int expecteddof = dofindices.size() > 0 ? (int)dofindices.size() : GetDOF(); | ||
| OPENRAVE_ASSERT_OP_FORMAT((int)dof,>=,expecteddof, "env=%s, body '%s' not enough values %d<%d", GetEnv()->GetNameId()%GetName()%dof%GetDOF(),ORE_InvalidArguments); | ||
| const int expecteddof = dofindices.size() > 0 ? (int)dofindices.size() : GetDOF(); | ||
| OPENRAVE_ASSERT_OP_FORMAT((int)dof, >=, expecteddof, | ||
| "env=%s, body '%s' not enough values %d<%d", GetEnv()->GetNameId() % GetName() % dof % expecteddof, | ||
| ORE_InvalidArguments); | ||
|
|
||
| GetDOFValues(_vTempJoints); | ||
| if( dofindices.size() > 0 ) { | ||
| // user only set a certain number of indices, so have to fill the temporary array with the full set of values first | ||
| // and then overwrite with the user set values | ||
| GetDOFValues(_vTempJoints); | ||
| for(size_t i = 0; i < dofindices.size(); ++i) { | ||
| if( !std::isnan(pJointValues[i]) ) { | ||
| _vTempJoints.at(dofindices[i]) = pJointValues[i]; | ||
| } | ||
| } | ||
| pJointValues = &_vTempJoints[0]; | ||
| } | ||
| else { | ||
| for(size_t i = 0; i < _vTempJoints.size(); ++i) { | ||
| if( !std::isnan(pJointValues[i]) ) { | ||
| _vTempJoints[i] = pJointValues[i]; | ||
| // When setting values to all joints, the input pJointValues already holds every value so use it directly and | ||
| // skip the expensive GetDOFValues. Only when pJointValues contains NaN (meaning "keep the current value") do we | ||
| // fetch the current values and fill in the rest. | ||
| _vTempJoints.resize(expecteddof); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps only resize if bHasNAN?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it too but |
||
| bool bHasNaN = false; | ||
| for(int i = 0; i < expecteddof; ++i) { | ||
| if( std::isnan(pJointValues[i]) ) { | ||
| bHasNaN = true; | ||
| break; | ||
| } | ||
| } | ||
| if( bHasNaN ) { | ||
| GetDOFValues(_vTempJoints); | ||
| for(int i = 0; i < expecteddof; ++i) { | ||
| if( !std::isnan(pJointValues[i]) ) { | ||
| _vTempJoints[i] = pJointValues[i]; | ||
| } | ||
| } | ||
| pJointValues = &_vTempJoints[0]; | ||
| } | ||
| } | ||
| pJointValues = &_vTempJoints[0]; | ||
|
|
||
| if( checklimits != CLA_Nothing ) { | ||
| dReal* ptempjoints = &_vTempJoints[0]; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Puttichai I wonder if there could be another fast path where dofindices.size() == expecteddof, and dofinidces contains all the indices of the robot (even if they are in a different order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thank you for the suggestion. I made a change to skip
GetDOFValueswhendofindicesalready covers all dofs. Also updated test cases.