notify other GUI plugins of added/removed entities via GUI events

[adlarkin]

Signed-off-by: Ashton Larkin ashton@openrobotics.org

🎉 New feature

Related to #1106

Summary

This PR builds on top of #1122. If a GUI plugin modifies the GUI's ECM directly, other GUI plugins may need to be aware of new/removed entities. Since gui plugins currently don't have a PostUpdate method (they only have Update), the approach here is to have a GUI plugin emit a AddedRemovedEntities event, which contains all of the plugin's new/removed entities. Other GUI plugins that need this information should listen for this event and then update accordingly.

This PR creates the new AddedRemovedEntities GUI event, and also implements how the entity tree GUI plugin updates whenever it receives information from this event. This PR does not implement any GUI plugins that add/remove entities and emits this event (this will be done in other PRs).

When this code is forward-ported to Garden, the plan is to modify the gazebo::GuiSystem interface to add a PostUpdate method so that we don't need to use this event approach anymore (we can't make this change in Fortress because it will break ABI).

Test it

To test, one can create/remove entities in a gazebo::GuiSystem plugin, emit an event with this information, and see if the entity tree is updated properly.

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 #1138 (1480dd4) into ign-gazebo6 (979f069) will decrease coverage by 0.11%.
The diff coverage is 4.16%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1138      +/-   ##
===============================================
- Coverage        63.84%   63.73%   -0.12%     
===============================================
  Files              256      256              
  Lines            20077    20116      +39     
===============================================
+ Hits             12818    12820       +2     
- Misses            7259     7296      +37     

Impacted Files	Coverage Δ
include/ignition/gazebo/gui/GuiEvents.hh	0.00% <0.00%> (ø)
src/gui/plugins/entity_tree/EntityTree.hh	100.00% <ø> (ø)
src/gui/plugins/entity_tree/EntityTree.cc	9.00% <4.54%> (-0.62%)	⬇️

---

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 979f069...1480dd4. Read the comment docs.

[iche033]

I see the events propagated to the entity tree and it displays newly added entities now, thanks. I noticed that the id of the new entity added to the entity tree starts at 0 though. I think it's an 64 vs 32 bit unsigned int issue.

[chapulina]

While working on a release, I noticed that the event introduced in this PR is very similar to another one created on #1053. I'd like to conciliate both of them before making a release. I'm just not sure what's the intended use case for this event. It sounds like it's notifying of entities that are created only on the GUI, but not on the server yet, right? Isn't it important for the receiver of this event to understand this distinction? I may be misunderstanding the point here, but at the moment I think this addition is confusing to both developers and users.

• Developers: The way the event is setup now, as a plugin developer, I'd expect to receive all new entities from this event, even ones created from the server, but that's not the case. It's also not clear to me what are the entity creation steps with this workflow. For example, will an entity created on the GUI be also deleted from the GUI before it is created on the server? I'd expect so, because the entity IDs will change...

• Users: I think it can become confusing for users if they see entities on the tree which aren't on the server yet. The state that they see on the GUI will not correspond to the one they get from tools like ign model, or even from their server plugins. They will also see meaningless entity IDs displayed on the UI. I think we should make the distinction between "entities being simulated" and "entities under construction" very clear to users. So probably these events may need to hold some extra information.

---

Sorry to only catch this now, but we haven't released it yet, so we can improve the situation. I was hoping some of my questions would have been raised before this PR was merged, or maybe would have been addressed on gazebosim/design#8. @adlarkin , @iche033 , @nkoenig , what do you think of reverting this PR until we have a concrete use case up for review? As it stands right now, no plugins are using it, right?

[iche033]

It was added mainly because EachNew / EachRemoved is not always triggered due to the order in which GUI plugins are updated in. One solution proposed by @adlarkin was to extend the GUI plugin to support PostUpdate (and also PreUpdate) function so we can rely on EachNew / EachRemoved events. That however breaks ABI so it'll need to be done in Garden.

I think we can revert this PR for the release. We do need an alternative way to be notified of added / removed entities in the GUI plugin though. Currently the model_editor branch uses these events. We could consider renaming the events to be more explicitly tied to GUI / model editor if it helps to avoid confusion, e.g. ModelEditorAddedRemovedEntities

[nkoenig]

While working on a release, I noticed that the event introduced in this PR is very similar to another one created on #1053. I'd like to conciliate both of them before making a release.

It seems like the two events could be consolidated, but it doesn't seem like the consolidation would block a release.

I'm just not sure what's the intended use case for this event. It sounds like it's notifying of entities that are created only on the GUI, but not on the server yet, right? Isn't it important for the receiver of this event to understand this distinction? I may be misunderstanding the point here, but at the moment I think this addition is confusing to both developers and users.

This event is designed to support the model editor. Models can be added or removed while paused, and those changes are not sent to the server. The changes only exist on the GUI's ECM. This design decision should be in the design document.

The event in this PR lets other plugins know of GUI additions and removals so that the user can see the additions and removals. For example, the EntityTree responds to this event.

We could rename the event and/or add extra documentation if that would help clarify things. I think you can still make a release with this event in place as it doesn't break anything.

[chapulina]

Thanks for the extra context.

It seems like the two events could be consolidated

Now that I understand the use case a bit more, I think that if we wanted to consolidate, we'd need an extra flag to explain if the event is GUI-only or not.

We could consider renaming the events to be more explicitly tied to GUI / model editor if it helps to avoid confusion, e.g. ModelEditorAddedRemovedEntities

I think this is also a good idea. Right now a user doesn't know the difference between this event and RemovedEntities.

I don't have a preference between:

1. combining the events and adding a flag
2. keeping them separate but making the use case explicit in the name

It seems to me that all plugins which listen to one of them may want to listen to both?

The event in this PR lets other plugins know of GUI additions and removals so that the user can see the additions and removals. For example, the EntityTree responds to this event.

I haven't tried the model_editor branch yet, but are you making a distinction between entities that are only staged to be created, and entities that are already created? Just with this PR, there would be no distinction; as I explained above, I think that's an usability problem.

The event isn't even emitted by any widgets at the moment, so downstream users may try to use it and be frustrated to see that it doesn't have the expected results.

I think you can still make a release with this event in place as it doesn't break anything.

If we release this event as it is, we can't change it anymore. We can't rename it, combine it, or even add a flag. I'm coming from the place of a downstream developer who is not up-to-date with the model editor's implementation, sees this new event added and gets confused about the usage.

On the flip side, reverting this PR doesn't break anything. That's why I suggested doing that instead.

[adlarkin]

Thanks @nkoenig and @iche033 for providing additional context. I will add a few more thoughts/comments as well:

I haven't tried the model_editor branch yet, but are you making a distinction between entities that are only staged to be created, and entities that are already created? Just with this PR, there would be no distinction; as I explained above, I think that's an usability problem.

We've only thought about entities that are "staged to be created" (i.e., entities that were created by a GUI plugin, but haven't been shared with the server if simulation is still paused). I do agree that this can be confusing to users if they use command line tools, which would give different information (we didn't think about this scenario at the time of implementing this event). Perhaps it would be worth updating things like the entity tree to show that these new entities are "pending" while simulation is still paused.

Models can be added or removed while paused, and those changes are not sent to the server. The changes only exist on the GUI's ECM. This design decision should be in the design document.

I just updated the design document to make this clear: gazebosim/design@df6f649

For example, will an entity created on the GUI be also deleted from the GUI before it is created on the server? I'd expect so, because the entity IDs will change...

I actually don't think the entity IDs should change: see #1122 and #1145 for some workarounds that were made to ensure that newly created GUI entities have a unique ID.

[chapulina]

I actually don't think the entity IDs should change: see #1122 and #1145 for some workarounds that were made to ensure that newly created GUI entities have a unique ID.

So you mean that the entity will keep its "GUI ID" after it's created on the server?

I'm wondering how plugins like SceneBroadcaster will handle this. I guess I need to look into the model_editor branch, I imagine that's part of what you folks are working on now.

[adlarkin]

So you mean that the entity will keep its "GUI ID" after it's created on the server?

I'm not sure that I understand what you mean by "GUI ID". From my understanding, an entity that exists in the GUI/server has the same ID on both the GUI and server. Is that not true? Is "GUI ID" different than an entity's ID?

[chapulina]

I'm not sure that I understand what you mean by "GUI ID".

Ah sorry, so I'm probably thinking about the way things worked pre-model-editor. For example, when we spawn a simple shape from the Shapes plugin, we create a "visual preview" that has an ID which doesn't collide with the server IDs. That's how I was thinking of the "GUI IDs": a temporary ID for an entity's preview before it is created for real. But from your comments, I guess that's not just a placeholder ID, but the actual entity ID that will be created on the server, right?

[adlarkin]

But from your comments, I guess that's not just a placeholder ID, but the actual entity ID that will be created on the server, right?

Yeah, that's right - it should be the actual entity ID, since the GUI ECM is being modified directly (if anyone thinks this could be a problem - or if I'm wrong 😬 - feel free to chime in).

---

Regarding the discussion about reverting this PR and/or updating the event before the release is made, I don't have a strong preference. If we don't revert this PR and decide to update the event so that it can be included in the release, I think it would be best to rename this event to something more specific to the model editor, because as @iche033 previously mentioned, the event introduced here is a temporary workaround until we introduce things like PostUpdate to GUI plugins in Garden. So, since this event will probably be deprecated in Garden, I don't think it makes sense to have things that aren't related to the model editor using it.

I will note that I do think it's a good idea to enhance the UI a bit by marking entities modified/created/removed by the model editor as "staged" until the server processes these updates. For example, perhaps in the entity tree, staged created entities can be colored green, staged removed entities can be colored red, and staged modified entities can be colored yellow/orange. Then, in the component inspector, we can add a field that indicates these entities are staged/not shared with the server yet. If doing something like this is a non-trivial amount of work, then maybe it would be best to revert this PR for now (or move it over to the model_editor branch) while these updates are made.

[chapulina]

Please see my suggestion in #1213. Let me know if I misunderstood the use case for this event.