Scale mode - Part3

[caguero]

🎉 New feature

Summary

This pull request updates the ModelSDF component of the entity (in the GUI's ECM) that has been scaled.

Test it

It depends on pull request #881. Run Gazebo with -v 4, scale a simple shape and you should see in the terminal the updated SDF. We'll remove these debug statements when we integrate this branch into the model editor.

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

[jennuine]

Looks good so far. Gave some minor feedback.

Will there be a part 4 to address saving the scaled model/SDF?

[jennuine]

We should account for //links that may have //collisions but no //visuals. For example,

<?xml version="1.0" ?>
<sdf version="1.7">
  <world name="test_world">
    <model name="m0">
      <pose>0 0 0 0 0 0</pose>
      <link name="l0">
        <collision name="c0">
          <geometry>
            <box>
              <size>1 1 1</size>
            </box>
          </geometry>
        </collision>
      </link>
    </model>
  </world>

If you right-click on the model m0 in the entity tree > View > Collisions, then scale the collision, the modelSdf component doesn't get updated

[caguero]

We should account for //links that may have //collisions but no //visuals. For example,

<?xml version="1.0" ?>
<sdf version="1.7">
  <world name="test_world">
    <model name="m0">
      <pose>0 0 0 0 0 0</pose>
      <link name="l0">
        <collision name="c0">
          <geometry>
            <box>
              <size>1 1 1</size>
            </box>
          </geometry>
        </collision>
      </link>
    </model>
  </world>

If you right-click on the model m0 in the entity tree > View > Collisions, then scale the collision, the modelSdf component doesn't get updated

I've modified this function to support more use cases in f126b66.

[jennuine]

Currently, this works but will be kind of odd for a user. For the test case above (where a //link only contains a //collision), the collision is not able to be scaled until after the collision visual is created regardless if it's visible or not:

The way View > Collisions works, a rendering::Visual is not automatically created for all entities in the scene, only when a user requests them. Then the visual is stored and when the user toggles them on/off the visual is hidden/shown.

Is it possible to update the SDF when the collision visual isn't created yet?

[caguero]

Looks good so far. Gave some minor feedback.

Will there be a part 4 to address saving the scaled model/SDF?

Exactly, that's the one missing PR for this series.

[caguero]

@jennuine , this is good for another pass. Thanks!

[jennuine]

A couple of things I've noticed during testing:

1. Scaling collisions when visual isn't created yet: Scale mode - Part3 #882 (comment)
2. Does not update nested models. When running ign gazebo -v 4 nested_model.sdf, model_01 SDF geometries are not updated (visuals are): https://github.com/ignitionrobotics/ign-gazebo/blob/7b326dcbf1b76576cd543cb59621c130b989bd27/examples/worlds/nested_model.sdf#L88-L108
3. Is it possible to update //inertial properties (when present) based on the scaling factor?
4. There is a name collision error when transforming position or rotation regardless if the object has been scaled or not. It prints each time the object is moved and if it's moved twice then the errors for each link in the scene prints twice, see gif below. This may have been introduced in a previous PR.

[caguero]

A couple of things I've noticed during testing:

1. Scaling collisions when visual isn't created yet: Scale mode - Part3 #882 (comment)
2. Does not update nested models. When running ign gazebo -v 4 nested_model.sdf, model_01 SDF geometries are not updated (visuals are): https://github.com/ignitionrobotics/ign-gazebo/blob/7b326dcbf1b76576cd543cb59621c130b989bd27/examples/worlds/nested_model.sdf#L88-L108
3. Is it possible to update //inertial properties (when present) based on the scaling factor?
4. There is a name collision error when transforming position or rotation regardless if the object has been scaled or not. It prints each time the object is moved and if it's moved twice then the errors for each link in the scene prints twice, see gif below. This may have been introduced in a previous PR.

All your points make sense and sound very reasonable. However. the goal for this pull request is to scale only simple shapes. I can see that (3) might fit here but I'd prefer to leave it outside of this pull request, as we might need to scale the mass, inertia matrix, and not sure what is the best way to do it. I don't think we've done this in Gazebo classic either.

[jennuine]

All your points make sense and sound very reasonable. However. the goal for this pull request is to scale only simple shapes. I can see that (3) might fit here but I'd prefer to leave it outside of this pull request, as we might need to scale the mass, inertia matrix, and not sure what is the best way to do it. I don't think we've done this in Gazebo classic either.

Continuing from #882 (review):

(1) To turn on collisions for a model, you should check if the model has a SDF collision that is a simple shape then under this check you could add something like this: https://github.com/ignitionrobotics/ign-gazebo/blob/f46616a0a3794ac3393ca825f7abd12cbeb3b4c1/src/gui/plugins/scene3d/Scene3D.cc#L838-L840

(2) If the rendered visual is updated, shouldn't the SDF geometries be updated as well? The nested model is made up of simple shapes.

(3) That makes sense to leave it outside this PR.

(4) This is an error that is introduced in one of the PR series and doesn't appear outside this PR. It should be resolved before merging into dome.

[caguero]

All your points make sense and sound very reasonable. However. the goal for this pull request is to scale only simple shapes. I can see that (3) might fit here but I'd prefer to leave it outside of this pull request, as we might need to scale the mass, inertia matrix, and not sure what is the best way to do it. I don't think we've done this in Gazebo classic either.

Continuing from #882 (review):

(1) To turn on collisions for a model, you should check if the model has a SDF collision that is a simple shape then under this check you could add something like this:

https://github.com/ignitionrobotics/ign-gazebo/blob/f46616a0a3794ac3393ca825f7abd12cbeb3b4c1/src/gui/plugins/scene3d/Scene3D.cc#L838-L840

(2) If the rendered visual is updated, shouldn't the SDF geometries be updated as well? The nested model is made up of simple shapes.

(3) That makes sense to leave it outside this PR.

(4) This is an error that is introduced in one of the PR series and doesn't appear outside this PR. It should be resolved before merging into dome.

@jennuine , this is ready for another review. As we discussed offline:

• The scaling tool is limited to unit simple shapes (cylinders, spheres and boxes).
• When scaling is complete (on mouse released), I modified the code for updating the ECM in the client.
• There's some logic to figure out if the model is scalable. When the model isn't scalable, the scale button is disabled. For this reason, you need to click on the model first, and then, click the scale button.
• I fixed (4).
• I fixed another issue when scaling cylinders and spheres. Now, they should preserve its shape.

[jennuine]

There's some logic to figure out if the model is scalable. When the model isn't scalable, the scale button is disabled. For this reason, you need to click on the model first, and then, click the scale button.

Is this implemented yet? When I run a world with complex shapes (e.g., diff_drive.sdf or nested_model.sdf), I'm able to select the scale button. Then when I try to scale the model, there's a seg fault.

The steps where:

1. ign gazebo -v diff_drive.sdf
2. Select one of the robots, this warning appears:

[GUI] [Wrn] [Application.cc:669] [QT] file::/TransformControl/TransformControl.qml:129: ReferenceError: disableScaleButton is not defined

3. Select scale and after trying to scale the model it seg faults

[caguero]

There's some logic to figure out if the model is scalable. When the model isn't scalable, the scale button is disabled. For this reason, you need to click on the model first, and then, click the scale button.

Is this implemented yet? When I run a world with complex shapes (e.g., diff_drive.sdf or nested_model.sdf), I'm able to select the scale button. Then when I try to scale the model, there's a seg fault.

Sorry, I did a last minute naming change and I introduced a typo in the qml code. It should work now. 33a6214

[jennuine]

Scaling can still be enabled for a nonscalable object when transitioning from a scalable object. If I click scale on a box then click the nonscalable object, the scale axis is showing. It can also still be enabled using the shortcut key s. In these cases, trying to scale the nonscalable object leads to a seg fault

[jennuine]

It would be nice to have a way to know the scale is disabled for particular objects in the top toolbar. For example, maybe the tooltip can be updated when hovering over the scale icon or the icon is grayed out

[caguero]

By the way, I looked into this but apparently the tooltip text doesn't work if the button is disabled. I also changed the color property when the button was disabled but it wasn't very evident. @iche033 , let me know if you have any suggestions here.

[caguero]

Scaling can still be enabled for a nonscalable object when transitioning from a scalable object. If I click scale on a box then click the nonscalable object, the scale axis is showing. It can also still be enabled using the shortcut key s. In these cases, trying to scale the nonscalable object leads to a seg fault

Both issues should be fixed in 6d6eb62.

[jennuine]

LGTM, thanks for iterating