Add support for GUI entity creation

[adlarkin]

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

🎉 New feature

Related to #1106

Summary

In order to create entities in the GUI (while paused) and then share these new entities with the server, we need to:

1. Make sure that entity IDs in the GUI are unique (i.e., they don't collide with server Entity IDs)
2. Update the "new entity state" in the GUI's ECM after new GUI entity additions are processed. Otherwise, on consecutive iterations while paused, the RenderUtil will try to create entities that were already created.

Item 1 above was resolved through a hack that sets the GUI ECM's entity creation ID offset to one that is (probably) larger than the server ECM's entity creation ID offset (I left a TODO note in the code about improving this). Item 2 above actually appears to be a bug that was never fixed. The fix for this is to move the GUI ECM's ClearNewlyCreatedEntities and ProcessRemoveEntityRequests calls to the GuiRunner::Implementation::UpdatePlugins method. This allows the GUI ECM's state to be properly updated when simulation is both running and paused (the previous approach of having these ECM method calls in GuiRunner::Implementation::ProcessState meant that these ECM method calls were only made when simulation is running).

Test it

@iche033 can you try to test this with your GUI model editor work in ign-gazebo6...model_editor_add_link? You should be able to remove these lines from your branch because this PR sets the entity creation offset for the ECM that's used by all GUI plugins.

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

[codecov]

Codecov Report

Merging #1122 (31b4dfb) into ign-gazebo6 (a2abed8) will increase coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 31b4dfb differs from pull request most recent head 4e417eb. Consider uploading reports for the commit 4e417eb to get more accurate results

@@               Coverage Diff               @@
##           ign-gazebo6    #1122      +/-   ##
===============================================
+ Coverage        63.84%   64.02%   +0.18%     
===============================================
  Files              256      255       -1     
  Lines            20076    20065      -11     
===============================================
+ Hits             12817    12847      +30     
+ Misses            7259     7218      -41     

Impacted Files	Coverage Δ
src/gui/GuiRunner.cc	81.57% <100.00%> (+3.31%)	⬆️
src/EntityComponentManager.cc	84.22% <0.00%> (-2.82%)	⬇️
src/SimulationRunner.cc	92.91% <0.00%> (-1.02%)	⬇️
src/systems/sensors/Sensors.cc	70.48% <0.00%> (-0.13%)	⬇️
src/systems/physics/Physics.cc	70.82% <0.00%> (ø)
src/gui/GuiRunner.hh
src/rendering/SceneManager.cc	28.74% <0.00%> (+0.19%)	⬆️
src/systems/imu/Imu.cc	73.68% <0.00%> (+0.42%)	⬆️
...int_position_controller/JointPositionController.cc	53.14% <0.00%> (+0.54%)	⬆️
.../plugins/component_inspector/ComponentInspector.cc	6.22% <0.00%> (+0.98%)	⬆️
... and 5 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 a2abed8...4e417eb. Read the comment docs.

[iche033]

this fixes the issue for me when new entities are created.

As discussed, we still need a way to propagate new ECM changes to other GUI plugins but that's outside the scope of this PR.

[adlarkin]

we still need a way to propagate new ECM changes to other GUI plugins but that's outside the scope of this PR

I'm concerned that the changes I proposed in this PR aren't actually a proper "fix". As we discussed offline, we run the chance of other GUI plugins not being aware of new entities that were added in a GUI plugin's Update if we clear the GUI ECM's new entity state at the end of a GUI Update. For example, consider the following example:

• GUI's Update begins. Plugin updates occur in the following order:
  • The EntityTree plugin processes its update, looking for new/removed entities to update the tree with
  • Another GUI plugin processes its update, which involves adding a new entity
• GUI's Update ends, and the ECM clears its new entity state

The problem with the scenario above is that EntityTree will never be aware of the new entity that was created by the other GUI plugin, since the new entities are cleared by the time EntityTree processes its next update. So, although the new entity exists, we will never see it in the entity tree.

I think the proper solution is to follow the pattern of server systems. We should give GUI plugins a PostUpdate so that all GUI plugins have the chance to see ECM changes that took place during Update. Then, at the end of PostUpdate, we can do things like clear the ECM's new entity state.

The solution I've proposed would involve updating other GUI plugin logic as needed (for example, having the EntityTree check for new entities in PostUpdate), and would also break ABI (so, we'd need to target Garden).

Another solution proposed by @iche033 is to send any changes made on the GUI back to the server, and then have the server broadcast the changes out to all GUI plugins. While this solution would not break ABI, I worry that this would be difficult code to maintain, and also wonder how much performance overhead would be introduced by going from GUI -> Server -> GUI for every GUI change. I think that implementing this approach would be just as much (or possibly even more) work than giving GUI systems a PostUpdate. I also prefer the approach of adding a PostUpdate because we can ensure thread safety by making the ECM read-only in PostUpdate.

I'm open to other suggestions/thoughts if anyone has them!

[nkoenig]

One minor suggestion.

[adlarkin]

Merge conflicts have been resolved in 98b5e2d. I have set this PR to auto-merge (squash).

As a follow-up to #1122 (comment), the solution we discussed offline is to use gui events to notify other gui systems of new/removed entities. This will prevent us from breaking ABI. When we forward port this work to Garden, we will upate the gazebo::GuiSystem interface to have a PostUpdate.