Fix pose of plane visual with non-default normal vector

[azeey]

Trying to verify my comment led me to this bug. It looks like the rotation due to a plane's normal vector is accounted for during the creation of the visual but not in pose update. The creation is in: https://github.com/ignitionrobotics/ign-gazebo/blob/ddd7fcda563b1f8c47c92610181a2d5f797c80bc/src/rendering/SceneManager.cc#L261-L267 and the pose update is done in: https://github.com/ignitionrobotics/ign-gazebo/blob/ddd7fcda563b1f8c47c92610181a2d5f797c80bc/src/rendering/RenderUtil.cc#L860 https://github.com/ignitionrobotics/ign-gazebo/blob/ddd7fcda563b1f8c47c92610181a2d5f797c80bc/src/rendering/RenderUtil.cc#L425

To test this, run the shapes.sdf example and set the plane visual's normal to 1 0 0.

diff --git a/examples/worlds/shapes.sdf b/examples/worlds/shapes.sdf
index 5dec565b..2136df07 100644
--- a/examples/worlds/shapes.sdf
+++ b/examples/worlds/shapes.sdf
@@ -35,3 +35,3 @@ Try moving a model:
             <plane>
-              <normal>0 0 1</normal>
+              <normal>1 0 0</normal>
             </plane>
@@ -42,3 +42,3 @@ Try moving a model:
             <plane>
-              <normal>0 0 1</normal>
+              <normal>1 0 0</normal>
               <size>100 100</size>

The plane should intersect with the shapes like so

Without this patch, the plane is not affected

I believe this fixes #26.

[iche033]

looks good to me. I think the removal of user data should be fine as well since we don't expect the geometry pose to change at run time. Approving once builds turn green

[chapulina]

Most visuals don't have a local pose. I think only the planes do. So removing this check is essentially duplicating the number of visuals in the scene by nesting an extra visual inside every visual. It may be worth it looking into performance impacts.

Another thing to keep in mind is that there may be code assuming that the visual with the entity ID has a geometry, as opposed to having a child visual. So I think this has the potential to break existing plugins and other user code.

[iche033]

just ran into an issue with this new node structure. This unfortunately broke thermal cameras which rely on the entity ID stored in the geometry to point to the parent visual but now it's pointing to a geometry visual.

[azeey]

Okay, would it be better to add a local pose member to rendering::Geometry? Do we have other alternatives?

[iche033]

hmm how about we do this for the ground plane as special case? i.e.

    if (localPose != math::Pose3d::Zero)
    {
      rendering::VisualPtr geomVis = this->dataPtr->scene->CreateVisual(name + "_geom");
      geomVis->SetLocalPose(localPose);
      geomVis->AddGeometry(geom);
      visualVis->AddChild(geomVis);
    }
    else
    {
      visualVis->AddGeometry(geom);
    }
    visualVis->SetLocalScale(scale);

[azeey]

hmm how about we do this for the ground plane as special case? i.e.

Yup, I can do that, but it would affect all plane shapes, not just the ground plane. This could still break existing code though if, as @chapulina said, the code assumes that the visual with the entity ID always has a geometry, as opposed to having a child visual.

[azeey]

Changed in d3c0732

[iche033]

This could still break existing code though

yeah.. I can't think of a good workaround for now. Adding localPose to Geometry in ign-rendering also means internally we'll be creating a node to store the geometry and apply transformation to this parent node.

One approach I'm thinking of is to transform the vertices of the plane that gets created in ign-rendering based on its normal. Alternatively add an Transform function ign-common's Mesh class to we can rotate it before feeding the mesh data to ign-rendering.

[chapulina]

I tried the current fix and it didn't seem to break any GUI functionality, like selection, highlight and collision visualization. So at least our plugins seem to be ok with the change. There's a small chance that this breaks downstream plugins, but I think the chance is small enough that the benefits outweigh it. Let's get this in!