Physics system: update link poses if the canonical link pose has been updated #876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ashton Larkin <[email protected]>
ahcorde
requested changes
Jun 23, 2021
|
FYI, gazebosim/gz-physics#264 should be partially fixed by gazebosim/gz-physics#268 |
Signed-off-by: Ashton Larkin <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-gazebo4 #876 +/- ##
===============================================
+ Coverage 65.97% 66.00% +0.02%
===============================================
Files 239 239
Lines 17657 17681 +24
===============================================
+ Hits 11649 11670 +21
- Misses 6008 6011 +3
Continue to review full report at Codecov.
|
Signed-off-by: Ashton Larkin <[email protected]>
adlarkin
force-pushed
the
adlarkin/fix_pose_updates
branch
from
78a8297 to
db748c0
Compare
Signed-off-by: Ashton Larkin <[email protected]>
adlarkin
changed the title
adlarkin
changed the title
|
@azeey, this is ready for review now. I also left a note in the PR description about not being able to test nested models for this in Dome, as we discussed offline. |
diegoferigo
reviewed
Jun 29, 2021
azeey
reviewed
Jun 29, 2021
Signed-off-by: Ashton Larkin <[email protected]>
azeey
approved these changes
Jul 8, 2021
This was referenced Jul 13, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Ashton Larkin [email protected]
🦟 Bug fix
Summary
As pointed out in gazebosim/gz-physics#264 (comment), if the pose of a model's canonical link changes, then the poses for all other links in this model must be updated (even if these links haven't moved) since these other link poses are given w.r.t. the model whose pose was just updated. I appeared to have removed this functionality in #669 (on accident, of course 🙂) when working on physics system performance updates, so I am adding it back in this PR. I've also added logic (and test coverage) to support pose updates for canonical links that are not the first link of a model (this has already been fixed in Edifice, but wasn't fixed for Dome yet).
A note about nested models: since nested model support is limited in Dome, I wasn't able to write tests that check for pose updates of nested models in this PR. I have left a TODO note for myself to add this test coverage when we forward port these changes to Edifice. Since we don't have test coverage for nested models yet, we should probably leave #871 open until this PR is forward ported to Edifice and nested model test coverage is included.
It's also worth noting that the physics system in Edifice/Dome has a few differences that were made for performance. This means that we may have some conflicts to resolve when porting this forward - I'm expecting that the implementation for this bug fix in Edifice/Fortress will differ somewhat from the Dome implementation (I can handle merging this bug fix forward once this PR is merged).
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge