Joint visual #366

atharva-18 · 2021-07-19T15:47:41Z

🎉 New feature

Summary

Adds joint visual consisting of an axis and an arrow visual.
Gazebo Classic's implementation can be found here - https://github.com/osrf/gazebo/blob/gazebo11/gazebo/rendering/JointVisual.cc

Test it

I have added an example in the visualization_demo folder.
To test it with Ignition Gazebo use the visualize_joints branch.

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]>

into joint_visual

ahcorde · 2021-07-19T15:51:23Z

include/ignition/rendering/JointVisual.hh

+#include <string>
+#include <ignition/math/Vector3.hh>
+#include "ignition/rendering/config.hh"
+#include "ignition/rendering/Object.hh"
+#include "ignition/rendering/RenderTypes.hh"
+#include "ignition/rendering/Visual.hh"


Suggested change

#include <string>

#include <ignition/math/Vector3.hh>

#include "ignition/rendering/config.hh"

#include "ignition/rendering/Object.hh"

#include "ignition/rendering/RenderTypes.hh"

#include "ignition/rendering/Visual.hh"

#include <string>

#include <ignition/math/Vector3.hh>

#include "ignition/rendering/config.hh"

#include "ignition/rendering/Object.hh"

#include "ignition/rendering/RenderTypes.hh"

#include "ignition/rendering/Visual.hh"

ahcorde · 2021-07-19T15:52:55Z

include/ignition/rendering/JointVisual.hh

+          const std::string &_xyzExpressedIn) = 0;
+
+      /// \brief Update the parent axis' arrow visual if it exists.
+      /// \param[in] _arrowVisual Arrow visual to be updated.


Suggested change

/// \param[in] _arrowVisual Arrow visual to be updated.

ahcorde · 2021-07-19T15:53:01Z

include/ignition/rendering/JointVisual.hh

+      /// \param[in] _axis Axis vector.
+      /// \param[in] _xyzExpressedIn Frame in which the axis vector is
+      /// expressed.
+      /// \param[in] _parentName Name of the joint parent.


Suggested change

/// \param[in] _parentName Name of the joint parent.

include/ignition/rendering/base/BaseArrowVisual.hh

ahcorde · 2021-07-19T15:56:14Z

include/ignition/rendering/base/BaseArrowVisual.hh

+      while (this->ChildCount() > 0u)
+      {
+        auto visual = std::dynamic_pointer_cast<Visual>(this->ChildByIndex(0));
+        visual->Destroy();


Suggested change

visual->Destroy();

if(visual)

{

visual->Destroy();

}

ahcorde · 2021-07-19T15:58:48Z

include/ignition/rendering/base/BaseJointVisual.hh

+#include "ignition/rendering/base/BaseObject.hh"
+#include "ignition/rendering/base/BaseRenderTypes.hh"
+#include "ignition/rendering/ArrowVisual.hh"
+#include "ignition/rendering/AxisVisual.hh"
+#include "ignition/rendering/JointVisual.hh"
+#include "ignition/rendering/Scene.hh"


Suggested change

#include "ignition/rendering/base/BaseObject.hh"

#include "ignition/rendering/base/BaseRenderTypes.hh"

#include "ignition/rendering/ArrowVisual.hh"

#include "ignition/rendering/AxisVisual.hh"

#include "ignition/rendering/JointVisual.hh"

#include "ignition/rendering/Scene.hh"

#include "ignition/rendering/ArrowVisual.hh"

#include "ignition/rendering/AxisVisual.hh"

#include "ignition/rendering/JointVisual.hh"

#include "ignition/rendering/Scene.hh"

#include "ignition/rendering/base/BaseObject.hh"

#include "ignition/rendering/base/BaseRenderTypes.hh"

ahcorde · 2021-07-19T15:59:40Z

include/ignition/rendering/base/BaseJointVisual.hh

+
+      /// \brief Flag to update the parent axis visual.
+      protected: bool updateParentAxis = false;
+


Suggested change

ahcorde · 2021-07-19T16:00:01Z

include/ignition/rendering/base/BaseJointVisual.hh

+      if (this->ParentAxisVisual())
+        this->ParentAxisVisual()->PreRender();


Suggested change

if (this->ParentAxisVisual())

this->ParentAxisVisual()->PreRender();

if (this->ParentAxisVisual())

{

this->ParentAxisVisual()->PreRender();

}

ahcorde · 2021-07-19T16:01:20Z

ogre/src/OgreScene.cc

+    const std::string &_name)
+{
+  OgreJointVisualPtr visual(new OgreJointVisual);
+    bool result = this->InitObject(visual, _id, _name);


Suggested change

bool result = this->InitObject(visual, _id, _name);

bool result = this->InitObject(visual, _id, _name);

ahcorde · 2021-07-19T16:01:34Z

ogre2/include/ignition/rendering/ogre2/Ogre2RenderTypes.hh

@@ -87,6 +88,7 @@ namespace ignition
    typedef shared_ptr<Ogre2GpuRays>              Ogre2GpuRaysPtr;
    typedef shared_ptr<Ogre2Grid>                 Ogre2GridPtr;
    typedef shared_ptr<Ogre2InertiaVisual>        Ogre2InertiaVisualPtr;
+    typedef shared_ptr<Ogre2JointVisual>        Ogre2JointVisualPtr;


codecov · 2021-07-19T16:12:02Z

Codecov Report

Merging #366 (b2936e0) into main (afa2270) will increase coverage by 0.43%.
The diff coverage is 82.54%.

❗ Current head b2936e0 differs from pull request most recent head fc6c6e3. Consider uploading reports for the commit fc6c6e3 to get more accurate results

@@            Coverage Diff             @@
##             main     #366      +/-   ##
==========================================
+ Coverage   58.16%   58.59%   +0.43%     
==========================================
  Files         170      174       +4     
  Lines       16788    17062     +274     
==========================================
+ Hits         9765     9998     +233     
- Misses       7023     7064      +41

Impacted Files	Coverage Δ
include/ignition/rendering/ArrowVisual.hh	`100.00% <ø> (ø)`
include/ignition/rendering/AxisVisual.hh	`0.00% <ø> (-100.00%)`	⬇️
include/ignition/rendering/JointVisual.hh	`0.00% <0.00%> (ø)`
include/ignition/rendering/Node.hh	`100.00% <ø> (ø)`
include/ignition/rendering/Scene.hh	`0.00% <ø> (ø)`
include/ignition/rendering/base/BaseScene.hh	`0.00% <ø> (ø)`
ogre/src/OgreJointVisual.cc	`0.00% <0.00%> (ø)`
ogre/src/OgreScene.cc	`26.66% <0.00%> (-0.37%)`	⬇️
ogre2/src/Ogre2JointVisual.cc	`50.00% <50.00%> (ø)`
include/ignition/rendering/base/BaseNode.hh	`81.63% <60.00%> (-0.57%)`	⬇️
... and 13 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 afa2270...fc6c6e3. Read the comment docs.

Signed-off-by: Atharva Pusalkar <[email protected]>

into joint_visual

ahcorde · 2021-07-22T15:13:48Z

include/ignition/rendering/base/BaseJointVisual.hh

+        quat.Axis((v.Cross(u)).Normalize(), angle);
+      _arrowVisual->SetLocalRotation(quat);
+
+      if (_xyzExpressedIn == "__model__")


what do you think about creating here a enum, instead of this string ? It looks weird

The value of _xyzExpressedIn is __model__ whenever the joint axis vector is expressed in the parent frame. How about a boolean value, for example, useParentFrame that is set to true whenever the joint axis vector is expressed in the parent frame? Using _xyzExpressedIn as a string looks more verbose, however.

@iche033 thoughts?

I think using the string _model_ can be tricky. We can have joints connecting two different models and also nested models. Ign-rendering also doesn't have the concept of a 'model' as it's just a scene tree with nodes in hierarchical structure. The current implementation looks like it's relative to its parent so I think a bool useParentFrame fits better.

An alternative option is to take the name or id of a node as an argument. The function then creates the axis relative to that node.

friendly ping @atharva-18

I think using the string _model_ can be tricky. We can have joints connecting two different models and also nested models. Ign-rendering also doesn't have the concept of a 'model' as it's just a scene tree with nodes in hierarchical structure. The current implementation looks like it's relative to its parent so I think a bool useParentFrame fits better.

An alternative option is to take the name or id of a node as an argument. The function then creates the axis relative to that node.

The axis is relative to the parent visual, so using useParentFrame would be a better option (The user would also have to fetch and pass the parent node id every time in the other case).

https://github.com/ignitionrobotics/ign-rendering/blob/4cde82345b3653cd3ee9f1907770daae2859626c/include/ignition/rendering/base/BaseJointVisual.hh#L399-L404

alright sounds good. Let's go with useParentFrame

Added in bfbb44b

chapulina · 2021-07-28T02:51:45Z

Part of gazebosim/gz-sim#106

into joint_visual

Signed-off-by: Atharva Pusalkar <[email protected]>

ahcorde

some minor fixes

ahcorde · 2021-08-17T12:10:42Z

include/ignition/rendering/Scene.hh

@@ -881,6 +881,35 @@ namespace ignition
      public: virtual InertiaVisualPtr CreateInertiaVisual(
                  unsigned int _id, const std::string &_name) = 0;

+      /// \brief Create new Joint visual. A unique ID and name will


Suggested change

/// \brief Create new Joint visual. A unique ID and name will

/// \brief Create new joint visual. A unique ID and name will

here and below

ahcorde · 2021-08-17T12:14:00Z

src/JointVisual_TEST.cc

+  if (!engine)
+  {
+    igndbg << "Engine '" << _renderEngine
+              << "' is not supported" << std::endl;


Suggested change

<< "' is not supported" << std::endl;

<< "' is not supported" << std::endl;

ahcorde · 2021-08-17T12:25:52Z

@iche033 do you mind to review it ? I would love to include this on Fortress

Signed-off-by: Atharva Pusalkar <[email protected]>

iche033 · 2021-08-17T20:27:11Z

@iche033 do you mind to review it ? I would love to include this on Fortress

yep, I'll get to this soon

iche033

works well for me. I have a few minor comments on style and doxygen comments. One suggestion is about the consistency between joint type and visualization, see comment in visualization_demo

iche033 · 2021-08-17T21:07:39Z

include/ignition/rendering/base/BaseArrowVisual.hh

+    template <class T>
+    void BaseArrowVisual<T>::Destroy()
+    {
+      while (this->ChildCount() > 0u)


will this go into infinite loop? Do you need to actually remove the child after destroying it?

When I ran the ArrowVisual and JointVisual unit tests, the ChildCount() prints out 0 though.

The child count is decremented every time Destroy() is called on one of its children.
Doing this step is necessary because the arrow visual associated with a joint still remains in the scene after removing the model:

This is similar to what I faced in the InertiaVisual class, the purple box remained in the scene even after destroying its parent visual (Inertia).

Maybe we can, to be safe, store the references to arrow visual children (shaft, head, and rotation) explicitly in variables, and destroy them whenever ArrowVisual->Destroy() is called?

Currently, arrow shaft, rotation, and head are accessed by index:
https://github.com/ignitionrobotics/ign-rendering/blob/4884de6cc4c0b798d9214101f53b726fa73503fe/include/ignition/rendering/base/BaseArrowVisual.hh#L103-L120

ok sounds good. I think this is fine. Can you add this snippet below to the end of ArrowVisual_TEST.cc so we have some coverage for this bit of code?

// test destroy ArrowVisualPtr visual2 = scene->CreateArrowVisual(); ASSERT_NE(nullptr, visual2); visual2->Destroy(); EXPECT_EQ(0u, visual2->ChildCount());

Added in fc6c6e3

iche033 · 2021-08-17T21:18:13Z

include/ignition/rendering/base/BaseJointVisual.hh

+
+      // Documentation inherited.
+      public: virtual void SetAxis(const ignition::math::Vector3d &_axis,
+          const bool _useParentFrame) override;


we started omitting const for primitive types, so just bool _useParentFrame

iche033 · 2021-08-17T21:20:12Z

include/ignition/rendering/base/BaseNode.hh

@@ -48,6 +48,8 @@ namespace ignition

      public: virtual math::Pose3d LocalPose() const override;

+      public: virtual math::Pose3d InitialLocalPose() const override;


can you add // Documentation inherited here?

iche033 · 2021-08-17T21:20:35Z

include/ignition/rendering/base/BaseNode.hh

@@ -196,6 +198,11 @@ namespace ignition

      protected: math::Vector3d origin;

+      protected: bool initialLocalPoseSet = false;
+
+      protected: ignition::math::Pose3d initialLocalPose =


can you add doxygen comments for these new variables.

iche033 · 2021-08-17T21:21:19Z

include/ignition/rendering/base/BaseScene.hh

@@ -277,6 +277,12 @@ namespace ignition
      protected: virtual InertiaVisualPtr CreateInertiaVisualImpl(
                     unsigned int _id, const std::string &_name) = 0;

+      /// \brief Implementation for creating Joint visual.
+      /// \param[in] _id Unique id
+      /// \param[in] _name Name of Joint visual


add \return

iche033 · 2021-08-17T21:31:20Z

examples/visualization_demo/Main.cc

+  jointParentBox->SetLocalRotation(0, 0, 0);
+  jointParentBox->SetMaterial(blue);
+  jointParentBox->Material()->SetTransparency(0.5);
+  jointParentBox->Material()->SetDepthWriteEnabled(false);


jointParentBox->Material()->SetTransparency(0.5); jointParentBox->Material()->SetDepthWriteEnabled(false);

do you still need to these 2 calls? The blue material should have these set.

Alternatively, set to a different color material so it's easier to distinguish between parent and child box

Changed material to gray in 3d31542

iche033 · 2021-08-17T21:37:16Z

include/ignition/rendering/JointVisual.hh

+      /// \return True if parent axis was updated else false.
+      public: virtual bool UpdateParentAxis(
+          const ignition::math::Vector3d &_axis,
+          const bool _useParentFrame = false) = 0;


you can omit const for primitive types. Same goes for other functions in this class.

iche033 · 2021-08-17T21:58:45Z

examples/visualization_demo/Main.cc

+  JointVisualPtr jointVisual = _scene->CreateJointVisual();
+  jointChildBox->AddChild(jointVisual);
+  jointVisual->SetType(JointVisualType::JVT_REVOLUTE);
+  ignition::math::Vector3d axis2(1.0, 0.0, 0.0);


set to a non-unit axis to show what the joint visual would look like? ,e.g. axis2(1.0, 1.0, 1.0)

iche033 · 2021-08-17T22:00:02Z

examples/visualization_demo/Main.cc

+  jointParentBox->AddGeometry(_scene->CreateBox());
+  jointParentBox->SetOrigin(0.0, 0.0, 0.0);
+  jointParentBox->SetLocalPosition(2.0, 0.5, 0.0);
+  jointParentBox->SetLocalRotation(0, 0, 0);


use a different rotation from child to show what the parent axis looks like with rotation?

iche033 · 2021-08-17T22:04:02Z

examples/visualization_demo/Main.cc

+  // create joint visual
+  JointVisualPtr jointVisual = _scene->CreateJointVisual();
+  jointChildBox->AddChild(jointVisual);
+  jointVisual->SetType(JointVisualType::JVT_REVOLUTE);


I was a little confused at first to see 2 joint visuals while the type is JVT_REVOLUTE. I would expect this to be JVT_REVOLUTE2 or JVT_UNIVERSAL. Maybe only create the parent axes or make it visible if joint type is revolute2 / universal?

714d523 should fix it

Signed-off-by: Atharva Pusalkar <[email protected]>

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

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 21 commits June 29, 2021 17:11

Added main class

134faeb

Signed-off-by: Atharva Pusalkar <[email protected]>

Added base API

21746c7

Signed-off-by: Atharva Pusalkar <[email protected]>

Added scene impl

e2b2440

Signed-off-by: Atharva Pusalkar <[email protected]>

Added UpdateAxisImpl

50f6d45

Signed-off-by: Atharva Pusalkar <[email protected]>

Add rotation visual

5dd198c

Signed-off-by: Atharva Pusalkar <[email protected]>

Add ShowAxisHead method

bfd743f

Signed-off-by: Atharva Pusalkar <[email protected]>

codecheck

409ddd1

Signed-off-by: Atharva Pusalkar <[email protected]>

Add ScaleToChild method

deecbb1

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix parent axis visual

71a0e7e

Signed-off-by: Atharva Pusalkar <[email protected]>

Add hinge2 example

392d3d6

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix parent scaling

b4f86e0

Signed-off-by: Atharva Pusalkar <[email protected]>

Update API to set axes

49e7247

Signed-off-by: Atharva Pusalkar <[email protected]>

Add Destroy impl

9b3afe5

Signed-off-by: Atharva Pusalkar <[email protected]>

Update ArrowVisual test

9f66ae4

Signed-off-by: Atharva Pusalkar <[email protected]>

Update AxisVisual test

c7e2f38

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix variable name

543de15

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix visibility and destroy

464347c

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix destroy child visuals

00c2c73

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix BaseArrowVisual::Destroy

4da8e7a

Signed-off-by: Atharva Pusalkar <[email protected]>

Add test

f0c535c

Signed-off-by: Atharva Pusalkar <[email protected]>

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

6925f90

into joint_visual

atharva-18 requested a review from iche033 as a code owner July 19, 2021 15:47

github-actions bot added the 🏯 fortress Ignition Fortress label Jul 19, 2021

ahcorde assigned atharva-18 Jul 19, 2021

ahcorde requested changes Jul 19, 2021

View reviewed changes

atharva-18 added 4 commits July 19, 2021 22:02

Use IGN_PI

b141395

Signed-off-by: Atharva Pusalkar <[email protected]>

Style comments

42d6394

Signed-off-by: Atharva Pusalkar <[email protected]>

Add curly braces

0e16df2

Signed-off-by: Atharva Pusalkar <[email protected]>

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

54feb2c

into joint_visual

ahcorde reviewed Jul 22, 2021

View reviewed changes

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

4cde823

into joint_visual

atharva-18 mentioned this pull request Aug 9, 2021

Visualize joints gazebosim/gz-sim#961

Merged

8 tasks

Add useParentFrame

bfbb44b

Signed-off-by: Atharva Pusalkar <[email protected]>

ahcorde requested changes Aug 17, 2021

View reviewed changes

Style fixes

4884de6

Signed-off-by: Atharva Pusalkar <[email protected]>

ahcorde approved these changes Aug 17, 2021

View reviewed changes

iche033 reviewed Aug 17, 2021

View reviewed changes

atharva-18 added 7 commits August 18, 2021 11:51

Remove const

d0a4bed

Signed-off-by: Atharva Pusalkar <[email protected]>

Style fixes

745e563

Signed-off-by: Atharva Pusalkar <[email protected]>

Add documentation inherited

45cb492

Signed-off-by: Atharva Pusalkar <[email protected]>

Change joint parent material

3d31542

Signed-off-by: Atharva Pusalkar <[email protected]>

Improve joint visual demo

9edace1

Signed-off-by: Atharva Pusalkar <[email protected]>

Fix Revolute2 and Universal joint visual types

714d523

Signed-off-by: Atharva Pusalkar <[email protected]>

Add destroy check to test

fc6c6e3

Signed-off-by: Atharva Pusalkar <[email protected]>

iche033 approved these changes Aug 19, 2021

View reviewed changes

ahcorde merged commit 3cbd121 into gazebosim:main Aug 19, 2021


		/// \brief Flag to update the parent axis visual.
		protected: bool updateParentAxis = false;

		if (this->ParentAxisVisual())
		this->ParentAxisVisual()->PreRender();

	bool result = this->InitObject(visual, _id, _name);
	bool result = this->InitObject(visual, _id, _name);

	/// \brief Create new Joint visual. A unique ID and name will
	/// \brief Create new joint visual. A unique ID and name will

	<< "' is not supported" << std::endl;
	<< "' is not supported" << std::endl;

		@@ -48,6 +48,8 @@ namespace ignition

		public: virtual math::Pose3d LocalPose() const override;

		public: virtual math::Pose3d InitialLocalPose() const override;

Joint visual #366

Joint visual #366

Conversation

atharva-18 commented Jul 19, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

atharva-18 Jul 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chapulina commented Jul 28, 2021

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

Choose a reason for hiding this comment

ahcorde commented Aug 17, 2021

iche033 commented Aug 17, 2021

iche033 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atharva-18 Aug 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atharva-18 Aug 18, 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

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

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 Jul 19, 2021 •

edited

Loading

codecov bot commented Jul 19, 2021 •

edited

Loading

atharva-18 Jul 22, 2021 •

edited

Loading

iche033 left a comment •

edited

Loading

atharva-18 Aug 18, 2021 •

edited

Loading

atharva-18 Aug 18, 2021 •

edited

Loading