Add support for cloning entities #959

adlarkin · 2021-08-06T15:50:07Z

Signed-off-by: Ashton Larkin [email protected]

🎉 New feature

Part of #102

Summary

This PR completes "The Great Way", part 1 of #102 (comment). I had to make an ABI-breaking change in Component.hh by adding a new virtual Clone method for components, so this is targeting main. If there's a way to achieve component cloning functionality without breaking ABI, let me know!

Test it

New tests have been written in this PR (see src/Component_TEST.cc, src/EntityComponentManager_TEST.cc).

To test this manually, you can do the following:

start gazebo:

ign gazebo shapes.sdf

in a new terminal, run the following command to clone the red box. Since we set the cloned box's position.z is set to 3 in the service call, the cloned box should appear above the original box:

ign service -s /world/shapes/create --reqtype ignition.msgs.EntityFactory --reptype ignition.msgs.Boolean --timeout 300 --req 'clone_name: "box", pose: {position: {z: 3}}, name: "box_copy"'

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: Ashton Larkin <[email protected]>

adlarkin · 2021-08-06T15:58:40Z

Currently, I am having an issue where the cloned entity cannot be moved. Here's what I am seeing when I run through the steps in the Test It section of the PR description (box_copy is the clone. Also, notice how when I move the original box, box_copy moves back to the origin):

Does anyone know what might be causing this?

chapulina · 2021-08-06T23:29:54Z

Does anyone know what might be causing this?

I looked a bit into it and I'm not sure. I verified that:

the UserCommands system receives the pose command
all entities are created and appear to have the correct components, but I didn't double check each of them
the Physics system appears to correctly creates ign-physics entities
the Physics system receives and applies the WorldPoseCmd

I did notice that the code never reaches this point when moving the copied model, but it does when moving other models:

https://github.com/ignitionrobotics/ign-gazebo/blob/9fbb9b988c20711fcff706fb14d034516e027e6b/src/systems/physics/Physics.cc#L2251

My suggestion would be to trace back what's missing for the code to get there. I hope that helps

iche033

looks like the box and box_copy have the same canonical link in the physics system. Maybe when cloning the entity, you'll need to update the cloned entity's canonical link to the cloned child link?

src/EntityComponentManager.cc

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin · 2021-08-17T20:04:34Z

looks like the box and box_copy have the same canonical link in the physics system. Maybe when cloning the entity, you'll need to update the cloned entity's canonical link to the cloned child link?

Yeah, that was the issue! I addressed this in 8b0e17c, and things seem to be working now. So, this PR should be ready for review 🤞

One other thing to note is a TODO note that I left in the code: https://github.com/ignitionrobotics/ign-gazebo/blob/8b0e17c2887028a8662aea087d2905a31679ecae/src/systems/user_commands/UserCommands.cc#L744

This is referring to the following field in the entity_factory message: https://github.com/ignitionrobotics/ign-msgs/blob/7a702296e6b58c7c3bfd9638c296a9e62f48810c/proto/ignition/msgs/entity_factory.proto#L72-L74

We have a method for getting the pose of an entity in the world: https://github.com/ignitionrobotics/ign-gazebo/blob/09e2ea78bf10d0bf84871e4ad3d0e120b85100eb/include/ignition/gazebo/Util.hh#L38-L43

Do we have any other similar methods for getting the pose of an entity in a particular frame?

Signed-off-by: Ashton Larkin <[email protected]>

iche033 · 2021-08-18T18:57:14Z

Do we have any other similar methods for getting the pose of an entity in a particular frame?

There is this RelativePose function in the physics system that may be useful here?

iche033 · 2021-08-20T21:23:54Z

copying simple shapes and meshes work well. I then tried to copy lights and sensors and ran into issues:

lights - crashes when trying to clone a light with the service call
sensors - cloning works but the data is published to the same topic, e.g. for camera sensors, in ImageDisplay you'll see images switching between two camera views

I'm testing with fuel_textured_mesh.sdf world with the following cmds:

ign service -s /world/fuel/create --reqtype ignition.msgs.EntityFactory --reptype ignition.msgs.Boolean --timeout 300 --req 'clone_name: "tube_light", pose: {position: {z: 1}}, name: "tube_light_copy"'
ign service -s /world/fuel/create --reqtype ignition.msgs.EntityFactory --reptype ignition.msgs.Boolean --timeout 300 --req 'clone_name: "camera", pose: {position: {z: 3}}, name: "camera_copy"'

iche033 · 2021-08-24T18:20:53Z

sensors - cloning works but the data is published to the same topic,

actually I think this may be ok. It's the same behavior when user tries to insert the same sensor model twice into the scene. If a user wants different camera topics, they will need to leave out the <topic> field and let gazebo auto generate the topic name

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin · 2021-08-27T02:50:05Z

lights - crashes when trying to clone a light with the service call

Okay, so looking into this has been quite the debugging journey! There appeared to be a few issues:

When cloning a light, the name of the sdf::Light object in the Light component needs to be set to the cloned name. Otherwise, the CreateSpotLight call in the SceneManager fails because we are trying to use a name that is already in use. When the CreateSpotLight call fails, a nullptr is returned that is later de-referenced, causing a crash. I took care of updating the cloned sdf::Light's name in 1091cd1.
There's a possible race condition with the views. Part of the changes in Use statically-typed views for better performance #856 is that new entities aren't actually added to the view until the view is used. This is because we need access to the template signature of the view when adding entities to it. The race condition comes into play during PostUpdate calls for server systems - since the PostUpdate system calls are run in parallel, if multiple systems are trying to access the same view for the first time since new entities were added to the view, then this code can be problematic since multiple threads could try to read/modify data internal to the view (note the ClearToAddEntities call at the end). For now, I've made a temporary fix for this in fe95598, but I will open a new PR with a better fix that is more performance-friendly.

I've noticed some other strange behavior when testing cloning lights. I made the following change to src/rendering/SceneManager.cc:

--- a/src/rendering/SceneManager.cc
+++ b/src/rendering/SceneManager.cc
@@ -1108,7 +1108,13 @@ rendering::LightPtr SceneManager::CreateLight(Entity _id,
       break;
     case sdf::LightType::SPOT:
     {
+      ignerr << "about to create spotlight in scene..." << std::endl;
       light = this->dataPtr->scene->CreateSpotLight(name);
+      if (nullptr == light)
+        ignerr << "light is null!" << std::endl;
+      else
+        ignerr << "light is not null" << std::endl;
+      ignerr << std::endl;
       rendering::SpotLightPtr spotLight =
           std::dynamic_pointer_cast<rendering::SpotLight>(light);
       spotLight->SetInnerAngle(_light.SpotInnerAngle());

Then, I run ign gazebo fuel_textured_mesh.sdf. Once simulation is loaded, I do the following while simulation is paused:

Clone the light by running the following command: ign service -s /world/fuel/create --reqtype ignition.msgs.EntityFactory --reptype ignition.msgs.Boolean --timeout 300 --req 'clone_name: "tube_light", pose: {position: {z: 1}}, name: "tube_light_copy"'
Remove the light (pull up the entity tree, right click on tube_light_copy -> select "remove")
Clone the light again (run the command listed in step 1)

At this point, I see the following output in the console (the last two sets of output are from the clone calls, and the first two sets of output are from when simulation was started):

[Err] [SceneManager.cc:1111] about to create spotlight in scene...
[Err] [SceneManager.cc:1116] light is not null
[Err] [SceneManager.cc:1117] 
[GUI] [Err] [SceneManager.cc:1111] about to create spotlight in scene...
[GUI] [Err] [SceneManager.cc:1116] light is not null
[GUI] [Err] [SceneManager.cc:1117] 
[GUI] [Err] [SceneManager.cc:1111] about to create spotlight in scene...
[GUI] [Err] [SceneManager.cc:1116] light is not null
[GUI] [Err] [SceneManager.cc:1117] 
[GUI] [Err] [SceneManager.cc:1111] about to create spotlight in scene...
[GUI] [Err] [SceneManager.cc:1116] light is not null
[GUI] [Err] [SceneManager.cc:1117]

Now, press play to start simulation. I see the following output (followed by a crash):

[Err] [SceneManager.cc:1111] about to create spotlight in scene...
[Err] [SceneManager.cc:1116] light is not null
[Err] [SceneManager.cc:1117] 
[Err] [SceneManager.cc:1111] about to create spotlight in scene...
[Err] [BaseStorage.hh:927] Another item already exists with name: tube_light_copy
[Err] [SceneManager.cc:1114] light is null!
[Err] [SceneManager.cc:1117]

So, it looks like the the server tries to create the light twice, but the second try fails since it's a duplicate (it fails due to the reasoning I explained earlier for having to change the cloned sdf::Light's name). However, I'd argue that the server should only be trying to create the spotlight once since we removed the cloned light before re-cloning it when simulation was paused. So, while I'm not sure what exactly is going on here, it seems like buggy behavior elsewhere in the system. Does anyone have some more thoughts about this?

codecov · 2021-08-27T02:51:39Z

Codecov Report

Merging #959 (838f350) into main (4be711c) will decrease coverage by 0.02%.
The diff coverage is 63.86%.

@@            Coverage Diff             @@
##             main     #959      +/-   ##
==========================================
- Coverage   63.47%   63.44%   -0.03%     
==========================================
  Files         241      241              
  Lines       19577    19656      +79     
==========================================
+ Hits        12426    12471      +45     
- Misses       7151     7185      +34

Impacted Files	Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh	`100.00% <ø> (ø)`
include/ignition/gazebo/rendering/SceneManager.hh	`100.00% <ø> (ø)`
src/gui/plugins/scene3d/Scene3D.cc	`10.85% <0.00%> (-0.02%)`	⬇️
src/systems/user_commands/UserCommands.cc	`56.03% <0.00%> (-1.92%)`	⬇️
src/rendering/SceneManager.cc	`28.77% <28.57%> (-0.09%)`	⬇️
src/rendering/RenderUtil.cc	`36.51% <57.14%> (+0.01%)`	⬆️
src/EntityComponentManager.cc	`84.95% <87.69%> (+0.28%)`	⬆️
include/ignition/gazebo/components/Component.hh	`100.00% <100.00%> (ø)`
src/gui/plugins/plot_3d/Plot3D.cc	`43.47% <0.00%> (-4.35%)`	⬇️
src/SimulationRunner.cc	`92.91% <0.00%> (-1.02%)`	⬇️

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 4be711c...838f350. Read the comment docs.

This reverts commit fe95598. Signed-off-by: Ashton Larkin <[email protected]>

adlarkin · 2021-08-30T22:41:18Z

There's a possible race condition with the views
For now, I've made a temporary fix for this in fe95598, but I will open a new PR with a better fix that is more performance-friendly.

I've opened a better fix in #1001 which seems to resolve the race condition issue, so I have reverted the temporary fix in d9de439.

adlarkin · 2021-09-01T13:27:09Z

#1001 has been merged, and this PR has been updated to include these changes in 876724b. So, now, I believe that the only other issue (for lights, at least) is what I described in the second half of #959 (comment) (cloning -> deleting -> re-cloning while simulation is paused), which seems to be a bug that already existed before this PR.

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin · 2021-09-01T19:29:38Z

I found that when cloning actors, the same fix with the light sdf (#959 (comment)) had to be applied. Cloning actors should work now with 2fbb384.

I believe that cloning support now exists for simple shapes, meshes, sensors, lights, and actors. This should be ready for another round of review.

iche033 · 2021-09-03T19:34:42Z

So, now, I believe that the only other issue (for lights, at least) is what I described in the second half of #959 (comment) (cloning -> deleting -> re-cloning while simulation is paused), which seems to be a bug that already existed before this PR.

I think the problem is the order in which the rendering thread processes requests from the ECM. This may be what's happening: When the user Clones -> Deletes -> Clones a tube_light, and hits Play, the sensors system starts processing these requests. Since sim was paused when these operations took place, these requests all come in at the same time. The rendering thread then carries out these operations in the following order as seen here:

Delete tube_light_copy
Create tube_light_copy
Create tube_light_copy -> crash because light with the same name already exists.

At minimum, I think we should catch duplicate lights in SceneManager e.g. using scene->HasLightName check, so we can avoid crashes when we roll out the clone feature. We can then ticket this issue about creating and deleting entities with the same name when sim is paused.

adlarkin · 2021-09-14T19:33:39Z

At minimum, I think we should catch duplicate lights in SceneManager e.g. using scene->HasLightName check, so we can avoid crashes when we roll out the clone feature

I went ahead and made the following changes to catch duplicate lights:

diff --git a/src/rendering/SceneManager.cc b/src/rendering/SceneManager.cc
index 283c12f1..b8f891ac 100644
--- a/src/rendering/SceneManager.cc
+++ b/src/rendering/SceneManager.cc
@@ -1101,6 +1101,12 @@ rendering::LightPtr SceneManager::CreateLight(Entity _id,
     name = parent->Name() +  "::" + name;
 
   rendering::LightPtr light;
+  if (this->dataPtr->scene->HasLightName(name))
+  {
+    ignerr << "A light named [" << name << "] already exists in the scene.\n";
+    return light;
+  }
+

However, I must say that I am still a little confused by the behavior. If I follow the same pattern of creating -> deleting -> creating the light copy while simulation is paused, and then press play, no crash occurs and I see this error message, as expected:

[Err] [SceneManager.cc:1106] A light named [tube_light_copy] already exists in the scene.

But, after doing this, if I press play and then delete -> create the light copy again, I see the same error message when re-creating the light, which doesn't make sense to me since the light has been deleted and re-created successfully in the GUI. So, my concern is that while things may appear to be okay on the GUI, something still isn't right on the server side.

chapulina

I did a first pass. There are some things that concern me about the changes to the ECM as I wrote below. But I think that the addition of Component::Clone is good, and that's the actual ABI breaking change. So if we can't sort out the ECM until code freeze, I think we can at least get the Component::Clone function into Fortress and add the ECM stuff later.

include/ignition/gazebo/EntityComponentManager.hh

src/EntityComponentManager.cc

src/systems/user_commands/UserCommands.cc

src/EntityComponentManager.cc

Signed-off-by: Ashton Larkin <[email protected]>

Signed-off-by: Louise Poubel <[email protected]>

chapulina

Great work! The usefulness of the new API and transport service goes beyond the copy/paste GUI functionality.

Signed-off-by: Louise Poubel <[email protected]>

chapulina · 2021-09-22T06:14:53Z

The Windows build failed with an infra issue. I'm going to merge this, hopefully it compiles on Windows 🤞 Otherwise we fix it tomorrow

clone entities seems to work, but cloned entity can't be moved in GUI

63cb07d

Signed-off-by: Ashton Larkin <[email protected]>

github-actions bot added the 🏯 fortress Ignition Fortress label Aug 6, 2021

iche033 reviewed Aug 7, 2021

View reviewed changes

src/EntityComponentManager.cc Show resolved Hide resolved

adlarkin self-assigned this Aug 7, 2021

adlarkin added the editor Tools to edit entities in simulation label Aug 9, 2021

adlarkin and others added 2 commits August 17, 2021 13:49

handle cloning canonical links

8b0e17c

Signed-off-by: Ashton Larkin <[email protected]>

Merge branch 'main' into adlarkin/clone_entities

c92c7db

adlarkin marked this pull request as ready for review August 17, 2021 19:57

adlarkin requested a review from chapulina as a code owner August 17, 2021 19:57

codecheck

4c5afe6

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin requested a review from iche033 August 17, 2021 21:53

adlarkin and others added 3 commits August 26, 2021 19:43

temporary patch for race condition

fe95598

Signed-off-by: Ashton Larkin <[email protected]>

update name of cloned sdf::light component

1091cd1

Signed-off-by: Ashton Larkin <[email protected]>

Merge branch 'main' into adlarkin/clone_entities

ad3ac95

adlarkin mentioned this pull request Aug 30, 2021

Lock views during system PostUpdates #1001

Merged

7 tasks

Revert "temporary patch for race condition"

d9de439

This reverts commit fe95598. Signed-off-by: Ashton Larkin <[email protected]>

adlarkin force-pushed the adlarkin/clone_entities branch from 07feabb to d9de439 Compare August 30, 2021 22:40

Merge branch 'main' into adlarkin/clone_entities

876724b

support cloning actors

2fbb384

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin mentioned this pull request Sep 8, 2021

Support copy/paste #1013

Merged

8 tasks

chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021

chapulina suggested changes Sep 17, 2021

View reviewed changes

adlarkin added 3 commits September 17, 2021 20:30

review feedback

ba5f116

Signed-off-by: Ashton Larkin <[email protected]>

Merge branch 'main' into adlarkin/clone_entities

396d16f

update SceneManager to use components::Name instead of name from SDF

cef373f

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin requested a review from chapulina September 20, 2021 20:59

chapulina added 2 commits September 21, 2021 18:41

Hide canonical links outside of public header

96396c8

Signed-off-by: Louise Poubel <[email protected]>

Merge branch 'main' into adlarkin/clone_entities

1cb545b

chapulina approved these changes Sep 22, 2021

View reviewed changes

codecheck

838f350

Signed-off-by: Louise Poubel <[email protected]>

chapulina merged commit ae1a048 into main Sep 22, 2021

chapulina deleted the adlarkin/clone_entities branch September 22, 2021 06:15

This was referenced Sep 28, 2021

5 ➡️ main #1062

Merged

Set a cloned joint's parent/child link names to the cloned parent/child link names #1075

Merged

Add detachable joint cloning support #1080

Open

Kakcalu13 mentioned this pull request May 16, 2022

Crash after respawn using sensors #1489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cloning entities #959

Add support for cloning entities #959

adlarkin commented Aug 6, 2021 •

edited

Loading

adlarkin commented Aug 6, 2021

chapulina commented Aug 6, 2021

iche033 left a comment •

edited

Loading

adlarkin commented Aug 17, 2021

iche033 commented Aug 18, 2021

iche033 commented Aug 20, 2021

iche033 commented Aug 24, 2021

adlarkin commented Aug 27, 2021

codecov bot commented Aug 27, 2021 •

edited

Loading

adlarkin commented Aug 30, 2021

adlarkin commented Sep 1, 2021

adlarkin commented Sep 1, 2021

iche033 commented Sep 3, 2021 •

edited

Loading

adlarkin commented Sep 14, 2021

chapulina left a comment

chapulina left a comment

chapulina commented Sep 22, 2021

Add support for cloning entities #959

Add support for cloning entities #959

Conversation

adlarkin commented Aug 6, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

adlarkin commented Aug 6, 2021

chapulina commented Aug 6, 2021

iche033 left a comment • edited Loading

Choose a reason for hiding this comment

adlarkin commented Aug 17, 2021

iche033 commented Aug 18, 2021

iche033 commented Aug 20, 2021

iche033 commented Aug 24, 2021

adlarkin commented Aug 27, 2021

codecov bot commented Aug 27, 2021 • edited Loading

Codecov Report

adlarkin commented Aug 30, 2021

adlarkin commented Sep 1, 2021

adlarkin commented Sep 1, 2021

iche033 commented Sep 3, 2021 • edited Loading

adlarkin commented Sep 14, 2021

chapulina left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

chapulina commented Sep 22, 2021

adlarkin commented Aug 6, 2021 •

edited

Loading

iche033 left a comment •

edited

Loading

codecov bot commented Aug 27, 2021 •

edited

Loading

iche033 commented Sep 3, 2021 •

edited

Loading