Fix assigning site IDs to new lanes in lifts #226
Conversation
Signed-off-by: Luca Della Vedova <[email protected]>
rmf_site_editor/src/site/save.rs
Outdated
| if let Ok(anchor_group) = cabin_anchor_groups.get(*site_child) { | ||
| if let Ok(anchor_children) = children.get(**anchor_group) { | ||
| for anchor_child in anchor_children { | ||
| if let Ok(e) = level_children.get(*anchor_child) { |
There was a problem hiding this comment.
This is not the best naming, I reused the level_children query that already queries for anchors, but I can create a duplicated anchors query if we want the cleanliness
There was a problem hiding this comment.
level_children queries for a lot more things than just anchors, so I definitely think we should not be using it for this. As an example of why.. It's plausible that at some point we'd implement animated lift doors by recycling the DoorType component inside a child of the lift. However we don't necessarily want that child entity to have its own SiteID since it's really part of the same "site element" as the lift. If we assign SiteIDs to all these different child types without considering whether it's really an element that gets saved, then we'll be contaminating the SiteIDs.
There was a problem hiding this comment.
Looking at the previously existing implementation, I'm realizing that we were already assigning SiteIDs to level_children entities within lifts. Now I'm conflicted and wondering if there was a reason we were doing this previously, e.g. to allow walls and models to be placed inside of lifts.
rmf_site_editor/src/site/save.rs
Outdated
| if let Ok(anchor_group) = cabin_anchor_groups.get(*site_child) { | ||
| if let Ok(anchor_children) = children.get(**anchor_group) { | ||
| for anchor_child in anchor_children { | ||
| if let Ok(e) = level_children.get(*anchor_child) { |
There was a problem hiding this comment.
level_children queries for a lot more things than just anchors, so I definitely think we should not be using it for this. As an example of why.. It's plausible that at some point we'd implement animated lift doors by recycling the DoorType component inside a child of the lift. However we don't necessarily want that child entity to have its own SiteID since it's really part of the same "site element" as the lift. If we assign SiteIDs to all these different child types without considering whether it's really an element that gets saved, then we'll be contaminating the SiteIDs.
Signed-off-by: Michael X. Grey <[email protected]>
Bug fix
Fixed bug
Fixes #224
Fix applied
The function to assign site IDs to new entities was not being called for anchors that were children of lifts, hence the subsequent query to save the site would not find the anchor (its ID would be missing).
The fix is to make sure we assign a site ID to anchors that are children of the lift cabin.
To test:
Without this PR you should get this error:
With this PR it should save successfully.