-
Notifications
You must be signed in to change notification settings - Fork 53
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
Center of mass visual #345
Conversation
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #345 +/- ##
==========================================
- Coverage 57.92% 57.76% -0.17%
==========================================
Files 166 170 +4
Lines 16433 16627 +194
==========================================
+ Hits 9519 9604 +85
- Misses 6914 7023 +109
Continue to review full report at Codecov.
|
@@ -0,0 +1,4 @@ | |||
file(GLOB files "*.png" "*.jpg") |
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.
file(GLOB files "*.png" "*.jpg") | |
file(GLOB files "*.png") |
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.
* | ||
*/ | ||
|
||
#include "ignition/rendering/ogre2/Ogre2COMVisual.hh" |
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.
alphabetize
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.
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
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.
works for me. I have a couple comments on the new APIs
ogre2/src/Ogre2COMVisual.cc
Outdated
this->dataPtr->sphereVis->SetLocalRotation(this->InertiaPose().Rot()); | ||
|
||
// Get the link's bounding box | ||
if (this->ParentLink().empty() || |
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 would like to avoid the need for users to call SetParentLink
function for setting linkName
because we should be able to access the parent once the COMVisual is attached to the parent visual.
Once idea is to override the PreRender
function:
- check to see if
HasParent()
is true and parent name is empty inPreRender
, - then Update
crossLines
based on the parent's bounding box.
Note that we should also avoid using the term link
as that's ign-gazebo specific. I think parentName
is more suitable.
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.
Fixed in f428ba1
. I have changed the API to use parentName
without changing the desired behavior. Also, I think the Ogre1 BoundingBox
issue (#345 (comment)) is probably not related to this PR. It's working fine for me in Ogre2.
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
{ | ||
this->parentName = this->Parent()->Name(); |
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.
from the user's perspective, they can get the parent name like you did here Parent()->Name()
so I think ParentName()
is a duplicate public facing API that we don't need to expose. Internally in this class, we can either make ParentName()
private or replace ParentName()
calls with this->parentName
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 have replaced all calls with this->parentName
3300dc2
.
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
Signed-off-by: Atharva Pusalkar <[email protected]>
{ | ||
// Compute radius of sphere with density of lead and equivalent mass. | ||
double sphereRadius; | ||
double dLead = 11340; |
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.
nit: use more verbose variable name densityLead
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.
Changed in aee5617
Signed-off-by: Atharva Pusalkar <[email protected]>
This pull request has been mentioned on Gazebo Community. There might be relevant details there: |
🎉 New feature
Summary
Adds center of mass visual consisting of a solid sphere with axis lines.
Test it
I have added an example in the
visualization_demo
.Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge