Skip to content
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

Merged
merged 22 commits into from
Jun 29, 2021
Merged

Center of mass visual #345

merged 22 commits into from
Jun 29, 2021

Conversation

atharva-18
Copy link
Contributor

@atharva-18 atharva-18 commented Jun 17, 2021

🎉 New feature

Summary

Adds center of mass visual consisting of a solid sphere with axis lines.
CoM VISUAL

Test it

I have added an example in the visualization_demo.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

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]>
@atharva-18 atharva-18 requested a review from iche033 as a code owner June 17, 2021 12:06
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 17, 2021
@atharva-18
Copy link
Contributor Author

Ogre1 doesn't seem to have a proper scaling for the cross-lines -
Screenshot from 2021-06-17 17-24-09

Signed-off-by: Atharva Pusalkar <[email protected]>
@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #345 (4ac0699) into main (8c34f9d) will decrease coverage by 0.16%.
The diff coverage is 40.90%.

❗ Current head 4ac0699 differs from pull request most recent head aee5617. Consider uploading reports for the commit aee5617 to get more accurate results
Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
ogre/src/OgreCOMVisual.cc 0.00% <0.00%> (ø)
ogre/src/OgreScene.cc 27.02% <0.00%> (-0.38%) ⬇️
ogre2/src/Ogre2COMVisual.cc 48.35% <48.35%> (ø)
include/ignition/rendering/base/BaseCOMVisual.hh 86.11% <86.11%> (ø)
src/base/BaseScene.cc 75.73% <86.36%> (+0.37%) ⬆️
include/ignition/rendering/COMVisual.hh 100.00% <100.00%> (ø)
ogre2/src/Ogre2Scene.cc 80.73% <100.00%> (+0.15%) ⬆️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c34f9d...aee5617. Read the comment docs.

ogre2/src/Ogre2COMVisual.cc Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
file(GLOB files "*.png" "*.jpg")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
file(GLOB files "*.png" "*.jpg")
file(GLOB files "*.png")

Copy link
Contributor Author

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alphabetize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atharva-18 and others added 3 commits June 21, 2021 20:03
Copy link
Contributor

@iche033 iche033 left a 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

include/ignition/rendering/COMVisual.hh Outdated Show resolved Hide resolved
this->dataPtr->sphereVis->SetLocalRotation(this->InertiaPose().Rot());

// Get the link's bounding box
if (this->ParentLink().empty() ||
Copy link
Contributor

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 in PreRender,
  • 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.

Copy link
Contributor Author

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();
Copy link
Contributor

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

Copy link
Contributor Author

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;
Copy link
Member

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

Copy link
Contributor Author

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]>
@ahcorde ahcorde requested review from iche033 and scpeters June 28, 2021 13:45
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/google-summer-of-code-2021-new-gui-widgets-in-ignition-gazebo/1081/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants