Inertia visual #326

atharva-18 · 2021-05-24T06:26:45Z

🎉 New feature

Summary

Adds inertia visual to debug models.

Test it

I have added a sample called visualization_demo in the examples folder.

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 <atharvapusalkar18@gmail.com>

codecov · 2021-05-24T06:49:31Z

Codecov Report

Merging #326 (42a6a41) into main (cbc35f8) will increase coverage by 0.02%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
+ Coverage   57.91%   57.94%   +0.02%     
==========================================
  Files         162      166       +4     
  Lines       16162    16385     +223     
==========================================
+ Hits         9361     9494     +133     
- Misses       6801     6891      +90

Impacted Files	Coverage Δ
include/ignition/rendering/Scene.hh	`0.00% <ø> (ø)`
include/ignition/rendering/base/BaseScene.hh	`0.00% <ø> (ø)`
ogre/src/OgreInertiaVisual.cc	`0.00% <0.00%> (ø)`
ogre/src/OgreScene.cc	`27.39% <0.00%> (-0.39%)`	⬇️
...clude/ignition/rendering/base/BaseInertiaVisual.hh	`72.00% <72.00%> (ø)`
src/base/BaseScene.cc	`75.36% <86.36%> (+0.40%)`	⬆️
ogre2/src/Ogre2InertiaVisual.cc	`91.56% <91.56%> (ø)`
include/ignition/rendering/InertiaVisual.hh	`100.00% <100.00%> (ø)`
ogre2/src/Ogre2DynamicRenderable.cc	`75.22% <100.00%> (+2.50%)`	⬆️
ogre2/src/Ogre2Node.cc	`92.04% <100.00%> (+0.09%)`	⬆️
... and 7 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 cbc35f8...42a6a41. Read the comment docs.

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

ahcorde

Some small comments in the code. Two more things:

Can you add some tests to these new classes?
Can you add an example in the example folder? I'm fine doing this in another PR and maybe include in the same example wireframes too. And potencially all the other new features.

include/ignition/rendering/InertiaVisual.hh

ogre/include/ignition/rendering/ogre/OgreInertiaVisual.hh

ogre/src/OgreInertiaVisual.cc

ogre2/include/ignition/rendering/ogre2/Ogre2InertiaVisual.hh

ogre2/src/Ogre2InertiaVisual.cc

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

scpeters · 2021-05-24T17:49:35Z

I was expecting to see a call to ignition::math::MassMatrix3::EquivalentBox as we do in osrf/gazebo's InertiaVisual::Load. How do you currently set the size of the inertia box?

atharva-18 · 2021-05-25T05:51:06Z

@scpeters The size of the inertia box is currently set in ign-gazebo's SceneManager.

ahcorde

Thank you for adding the tests! Do you mind to change the status of the PR from Draft to a normal PR ?

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

atharva-18 · 2021-05-26T10:27:58Z

I have added a sample in the examples folder

ahcorde

Do you mind to add support for windows ? Take a look to this PR #291

examples/views_demo/GlutWindow.cc

examples/views_demo/Main.cc

scpeters · 2021-05-26T17:21:31Z

I was expecting to see a call to ignition::math::MassMatrix3::EquivalentBox as we do in osrf/gazebo's InertiaVisual::Load. How do you currently set the size of the inertia box?

@scpeters The size of the inertia box is currently set in ign-gazebo's SceneManager.

If this class doesn't actually take inertial parameters as direct inputs, I'm not sure it makes sense to call this class InertiaVisual. Currently it's just a box with 3 orthogonal lines. I think it will be easier to generate inertia visuals in other applications if we put the sizing logic in this class rather than require the downstream application to pass the pose and scale. For example, how about the following API?

virtual void Load(const ignition::math::Inertiald &_inertial);

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

atharva-18 · 2021-05-27T05:19:24Z

I was expecting to see a call to ignition::math::MassMatrix3::EquivalentBox as we do in osrf/gazebo's InertiaVisual::Load. How do you currently set the size of the inertia box?

@scpeters The size of the inertia box is currently set in ign-gazebo's SceneManager.

If this class doesn't actually take inertial parameters as direct inputs, I'm not sure it makes sense to call this class InertiaVisual. Currently it's just a box with 3 orthogonal lines. I think it will be easier to generate inertia visuals in other applications if we put the sizing logic in this class rather than require the downstream application to pass the pose and scale. For example, how about the following API?
virtual void Load(const ignition::math::Inertiald &_inertial);

I see. I think using math::inertiald also makes more semantic sense. Similar to Gazebo classic, we can call Load(const ignition::math::Inertiald &_inertial) which in turn calls Load(const ignition::math::Pose3d &_pose, const ignition::math::Vector3d &_scale) to create the visual. Thoughts @ahcorde?

ahcorde · 2021-05-28T19:40:42Z

@iche033 do you mind to take a look ?

iche033 · 2021-06-01T20:45:42Z

I see. I think using math::inertiald also makes more semantic sense. Similar to Gazebo classic, we can call Load(const ignition::math::Inertiald &_inertial) which in turn calls Load(const ignition::math::Pose3d &_pose, const ignition::math::Vector3d &_scale) to create the visual.

That sounds good to me. Looking at the implementation, I think one API suggestion is to call the function SetInertial(...) instead of Load(..) to indicate to the user that it is safe for this function to be called multiple times as opposed to be a once only thing on load.

examples/views_demo/CMakeLists.txt

examples/views_demo/GlutWindow.cc

examples/views_demo/Main.cc

include/ignition/rendering/base/BaseInertiaVisual.hh

iche033 · 2021-06-01T23:23:52Z

src/InertiaVisual_TEST.cc

+  ignition::math::Vector3d s(0.5, 0.5, 0.5);
+  inertiaVisual->Load(p, s);
+
+  EXPECT_NE(nullptr, inertiaVisual->BoxVisual());


can you expand the test to include other APIs like Material and OgreObject?

I think the methods to get Material and OgreObject are local to Ogre 1 and 2 classes only, similar to the LightVisual test and the Ogre1 LightVisual class

into inertia_visual

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

scpeters · 2021-06-03T16:21:05Z

I see. I think using math::inertiald also makes more semantic sense. Similar to Gazebo classic, we can call Load(const ignition::math::Inertiald &_inertial) which in turn calls Load(const ignition::math::Pose3d &_pose, const ignition::math::Vector3d &_scale) to create the visual.

That sounds good to me. Looking at the implementation, I think one API suggestion is to call the function SetInertial(...) instead of Load(..) to indicate to the user that it is safe for this function to be called multiple times as opposed to be a once only thing on load.

I see that you added the SetInertial(...) method in 3456a69, nice work. I did notice that the content of OgreInertiaVisual::SetInertial is identical to Ogre2InertiaVisual::SetInertial. Since none of that code is ogre-specific, I would move it to BaseInertiaVisual<T>::SetInertial and not override the ogre SetInertial methods. What do you think @iche033?

ahcorde · 2021-06-03T16:23:24Z

Since none of that code is ogre-specific, I would move it to BaseInertiaVisual<T>::SetInertial and not override the ogre SetInertial methods.

+1

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

atharva-18 · 2021-06-03T17:51:21Z

I see that you added the SetInertial(...) method in 3456a69, nice work. I did notice that the content of OgreInertiaVisual::SetInertial is identical to Ogre2InertiaVisual::SetInertial. Since none of that code is ogre-specific, I would move it to BaseInertiaVisual<T>::SetInertial and not override the ogre SetInertial methods. What do you think @iche033?

I moved the content to BaseInertiaVisual<T>::SetInertial from the Ogre methods.

into inertia_visual

atharva-18 · 2021-06-07T06:05:46Z

One possible fix to the Ogre2 bug is to destroy all child visuals whenever BaseInertiaVisual<T>::Destroy is called

@@ -86,6 +89,17 @@ namespace ignition
       T::Init();
     }
 
+    //////////////////////////////////////////////////
+    template <class T>
+    void BaseInertiaVisual<T>::Destroy()
+    {
+      for (unsigned int i = this->ChildCount(); i > 0; --i)
+      {
+        this->ChildByIndex(i - 1)->Destroy();
+      }
+      T::Destroy();
+    }
+

iche033 · 2021-06-08T00:28:46Z

what's the ogre2 bug? How about being explicit and just removing the BoxVisual instead of all child visuals

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

atharva-18 · 2021-06-08T06:39:17Z

what's the ogre2 bug? How about being explicit and just removing the BoxVisual instead of all child visuals

In Ogre2, destroying an InertiaVisual doesn't, in turn, destroy its BoxVisual. Destroying the box visual explicitly by this->dataPtr->boxVis->Destroy() is giving a segfault in the inertia visual test, specifically when calling engine->DestroyScene(scene).

ahcorde · 2021-06-09T15:43:28Z

@atharva-18 how I can reproduce the issue ?

atharva-18 · 2021-06-09T16:30:32Z

@atharva-18 how I can reproduce the issue ?

These are the steps:

cd to the visualization_demo directory and add inertiaVisual->Destroy() after Line 133 in the Main.cc file.
To produce the boxVisual "not getting destroyed" bug, comment out Ogre2InertiaVisual::Destroy() method.
Now, if you run the demo using Ogre2, the inertia visual's box remains at the origin, detached from its parent.

If we keep the Ogre2InertiaVisual::Destroy() method as it is, the box visual gets destroyed, but the unit test gives a segfault.
One way to avoid the segfault is to not explicitly destroy the box visual,

@@ -86,6 +89,17 @@ namespace ignition
       T::Init();
     }
 
+    //////////////////////////////////////////////////
+    template <class T>
+    void BaseInertiaVisual<T>::Destroy()
+    {
+      for (unsigned int i = this->ChildCount(); i > 0; --i)
+      {
+        this->ChildByIndex(i - 1)->Destroy();
+      }
+      T::Destroy();
+    }
+

Presumably, this bug is also present in AxisVisual.

iche033 · 2021-06-10T23:07:53Z

ogre2/src/Ogre2InertiaVisual.cc

+{
+  if (this->dataPtr->boxVis != nullptr)
+  {
+    this->dataPtr->boxVis->Destroy();


reset boxVis so when Destroy is called again, the above line won't be executed.

this->dataPtr->boxVis.reset();

After that, apply this patch, and see if it fixes the crash for you

diff --git a/ogre2/src/Ogre2Node.cc b/ogre2/src/Ogre2Node.cc index 63a43a93..cff1e3e8 100644 --- a/ogre2/src/Ogre2Node.cc +++ b/ogre2/src/Ogre2Node.cc @@ -64,6 +64,8 @@ Ogre::SceneNode *Ogre2Node::Node() const ////////////////////////////////////////////////// void Ogre2Node::Destroy() { + if (ogreNode) + return;

The crash is fixed, but UNIT_Scene_TEST seems to be failing

Start 55: UNIT_Scene_TEST 55/93 Test #55: UNIT_Scene_TEST .......................***Failed 1.25 sec

oops that should've been if (!ogreNode)

Can you take a look at the changes in this commit:

9575d3d

I think that fixes the test and crash.

Fixed in dd42817

iche033 · 2021-06-10T23:09:15Z

ogre2/src/Ogre2InertiaVisual.cc

+  {
+    this->dataPtr->boxVis->Destroy();
+  }
+  BaseInertiaVisual::Destroy();


I think this is not needed as we are handling most of the destruction of object ourselves.

Destroying the crossLines still requires us to call BaseInertiaVisual::Destroy(), see #326 (comment)

Fixed in dd42817

iche033 · 2021-06-10T23:09:47Z

ogre2/src/Ogre2InertiaVisual.cc

+  {
+    this->dataPtr->boxVis->Destroy();
+  }
+  BaseInertiaVisual::Destroy();


Destroy material:

if (this->dataPtr->material && this->Scene()) { this->Scene()->DestroyMaterial(this->dataPtr->material); this->dataPtr->material.reset(); }

I am getting this message in the console

visualization_demo: /var/lib/jenkins/workspace/ogre-2.1-debbuilder/repo/OgreMain/src/OgreHlmsDatablock.cpp:170: virtual Ogre::HlmsDatablock::~HlmsDatablock(): Assertion `mLinkedRenderables.empty() && "This Datablock is still being used by some Renderables." " Change their Datablocks before destroying this."' failed. Aborted (core dumped)

I think this is because of crossLines still using the material. Explicitly destroying crossLines before the material is giving an exception in the test

Start 17: UNIT_InertiaVisual_TEST 17/93 Test #17: UNIT_InertiaVisual_TEST ...............Child aborted***Exception: 0.33 sec

This material is also used in LightVisual where it is not destroyed.

Fixed in dd42817

iche033 · 2021-06-10T23:11:18Z

examples/visualization_demo/CMakeLists.txt

+  set(TARGET_THIRD_PARTY_DEPENDS
+    ${TARGET_THIRD_PARTY_DEPENDS}
+    GLEW::glew)


I was not able to build the demo with these lines as it complains about glew

I had to replace it with:

if (WIN32) set(TARGET_THIRD_PARTY_DEPENDS ${TARGET_THIRD_PARTY_DEPENDS} GLEW::glew ) else () set(TARGET_THIRD_PARTY_DEPENDS ${TARGET_THIRD_PARTY_DEPENDS} GLEW ) endif()

FIxed in ca3ac18

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

examples/visualization_demo/CMakeLists.txt

ahcorde

Last comment from me

ahcorde · 2021-06-14T11:29:38Z

examples/custom_shaders_uniforms/CMakeLists.txt

@@ -5,7 +5,7 @@ include_directories(SYSTEM
  ${PROJECT_BINARY_DIR}
 )

-find_package(ignition-rendering4)
+find_package(ignition-rendering6)


This change is not related to this PR I prefer to remove it from this PR

Removed the commit - https://github.com/ignitionrobotics/ign-rendering/compare/617af60fdaa475ec13612f41f1068d574df12b34..242888265c0c2f5cd4ac9a5612e0015fcfe65609

created PR for this change: #343

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

iche033

just have one more comment about ogre2 transparency setting for the purple box, otherwise it's working well for me.

iche033 · 2021-06-15T17:04:27Z

src/base/BaseScene.cc

+  material->SetTransparency(0.5);
+  material->SetCastShadows(false);
+  material->SetReceiveShadows(false);
+  material->SetLightingEnabled(false);


looks like the semi-transparent purple box is occluding the green lines in the ogre2 visualization. I think you can disable depth write to make the transparency effect more like ogre 1.x (which has depth write disabled by default when objects are semi-transparent).

material->SetDepthWriteEnabled(false);

Fixed in 42a6a414

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

osrf-triage · 2021-09-01T11:46:07Z

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

atharva-18 added 3 commits May 20, 2021 15:12

Add inertia visual classes

cc7dee6

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Add methods to create inertia visual

0c10dbf

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Fix box creation and material

1c84404

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

github-actions bot added the 🏯 fortress Ignition Fortress label May 24, 2021

Fix comments

9d687a0

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Fix override

9fd65e0

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

ahcorde self-requested a review May 24, 2021 07:07

ahcorde assigned atharva-18 May 24, 2021

ahcorde requested changes May 24, 2021

View reviewed changes

atharva-18 added 2 commits May 24, 2021 18:43

Add Inertia visual test

6e444dc

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Style fixes

aa50a7c

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

ahcorde approved these changes May 26, 2021

View reviewed changes

Add example

491cb61

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

atharva-18 marked this pull request as ready for review May 26, 2021 10:28

atharva-18 requested a review from iche033 as a code owner May 26, 2021 10:28

ahcorde requested changes May 26, 2021

View reviewed changes

examples/views_demo/GlutWindow.cc Outdated Show resolved Hide resolved

examples/views_demo/Main.cc Outdated Show resolved Hide resolved

examples/views_demo/Main.cc Outdated Show resolved Hide resolved

examples/views_demo/Main.cc Outdated Show resolved Hide resolved

atharva-18 added 2 commits May 27, 2021 10:15

Add Windows support

ba2be80

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Remove traces from updateCameras

1547997

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

iche033 reviewed Jun 1, 2021

View reviewed changes

Merge branch 'main' of https://github.com/ignitionrobotics/ign-rendering

36e240d

into inertia_visual

Add SetInertial method

3456a69

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Move code to BaseInertiaVisual<T>::SetInertial

4a35a36

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Merge branch 'main' of https://github.com/ignitionrobotics/ign-rendering

64e15b5

into inertia_visual

iche033 self-requested a review June 7, 2021 18:31

Add Ogre2 destroy method

63aabbf

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

iche033 reviewed Jun 10, 2021

View reviewed changes

Fix CMakeLists in example

ca3ac18

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

ahcorde requested changes Jun 14, 2021

View reviewed changes

examples/visualization_demo/CMakeLists.txt Outdated Show resolved Hide resolved

ahcorde requested changes Jun 14, 2021

View reviewed changes

atharva-18 added 3 commits June 14, 2021 17:22

Fix CMakeLists

8ca07c8

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Fix crash and test

513d17f

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

Fix spaces

2428882

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

atharva-18 force-pushed the inertia_visual branch from 617af60 to 2428882 Compare June 14, 2021 11:53

ahcorde requested a review from iche033 June 15, 2021 07:50

ahcorde approved these changes Jun 15, 2021

View reviewed changes

iche033 reviewed Jun 15, 2021

View reviewed changes

Set material depth write to false

42a6a41

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>

iche033 approved these changes Jun 15, 2021

View reviewed changes

ahcorde merged commit b0c9523 into gazebosim:main Jun 15, 2021

Inertia visual #326

Inertia visual #326

Conversation

atharva-18 commented May 24, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

codecov bot commented May 24, 2021 • edited Loading

Codecov Report

ahcorde left a comment

Choose a reason for hiding this comment

scpeters commented May 24, 2021

atharva-18 commented May 25, 2021

ahcorde left a comment

Choose a reason for hiding this comment

atharva-18 commented May 26, 2021

ahcorde left a comment

Choose a reason for hiding this comment

scpeters commented May 26, 2021

atharva-18 commented May 27, 2021 • edited Loading

ahcorde commented May 28, 2021

iche033 commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters commented Jun 3, 2021

ahcorde commented Jun 3, 2021

atharva-18 commented Jun 3, 2021

atharva-18 commented Jun 7, 2021

iche033 commented Jun 8, 2021

atharva-18 commented Jun 8, 2021

ahcorde commented Jun 9, 2021

atharva-18 commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atharva-18 Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osrf-triage commented Sep 1, 2021

atharva-18 commented May 24, 2021 •

edited

Loading

codecov bot commented May 24, 2021 •

edited

Loading

atharva-18 commented May 27, 2021 •

edited

Loading

atharva-18 Jun 11, 2021 •

edited

Loading