Add MultiSelection Resource to enable selecting multiple entities. Up…

[JohnTGZ]

New feature implementation

Implemented feature

This PR implements the ability to select multiple models in the site which is briefly described in issue #337 .

User workflow:

1. Hold down the Left SHIFT key.
2. Select multiple models

Implementation description

One of the primary changes involves introducing the MultiSelection resource which is a HashSet to hold multiple entities.
While multiple entities are selected by holding down the SHIFT key:

• The Inspect widget is disabled.
• The MultiSelection resource will be updated to hold the selected entities
• Multiple entities can be deleted at one go.

GenAI declaration

• I did not use GenAI

[JohnTGZ]

@xiyuoh Hi, please take a look and provide any thoughts/suggestions on the implementation, thank you!

[mxgrey]

Thanks for kicking off this feature development. I've wanted a multi-select for a long time but I've been held back by how intertwined the selection mechanisms are.

I've left some recommendations for changes that I think we need to make before we can move forward with this PR. The current approach you've made is very minimal in lines of code changed and minimal in disruption, which are good qualities, but I fear it's creating a fragmented selection pipeline that will leave us with some technical debt in the long run.

[mxgrey]

Instead of disabling the inspector widget, I think we should show something like a list of the objects that are currently selected.

I would recommend introducing a DeselctionWidget as a contrast to the SelectionWidget. Instead of a finger icon to indicate selecting the object, we can use an x icon to indicate de-selcting it. When the user hovers the x, it has the same hover effect as the SelectionWidget, but clicking on the x will remove the object from the list of selected items.

[mxgrey]

I don't think we should use keyboard_input directly here, for a bunch of reasons:

1. This hard-codes the use of left shift as the thing that differentiates between multi-select vs single-select, but we may want to allow key remapping or other ways to toggle multi-select
2. Not all interaction modes can make use of multi-select. For example it wouldn't make sense to build up a selection of multiple anchors while creating a lane.

I think what I would suggest instead is to add a field to the Select structure that indicates whether or not multi-select is allowed. Then it's up to the systems that generate the Select event to decide if multi-select is supported for the action. We can introduce a resource

#[derive(Resource)]
pub struct InspectionSettings {
    pub multi_select: bool,
}

which the inspector mode will use to decide whether the Select event should have multi-select enabled or not. This InspectionSettings resource can have its multi_select field toggled by a system that listens for when the left shift key goes up or down. In the future we can generalize that to allow keybindings, and downstream users can introduce their own way to toggle the setting.

[JohnTGZ]

I think this approach is better. I was wondering what would be the best way to restrict multi-selection to say model_instances and not lanes, anchors etc. and this does the trick and is more scalable.

To summarize:

1. We update the Select event to have a multi_select field which will determine if we are able to select multiple entities:

pub struct Select {
    #[deref]
    pub selection_candidate: Option<SelectionCandidate>,
    pub multi_select: bool,
}

2. Add a system that monitors Res<ButtonInput<KeyCode>>, where upon change in the SHIFT keyboard input, it will update the InspectionSettings resource.

3. The individual inspector functions will read the InspectionSettings resource to update the multi_select field of Select when triggering a Select event.

4. The selection_update service receives the Select event and reads it to determine whether to enable/disable multi-selection

[JohnTGZ]

In the midst of implementing this and there are some things Im unclear about and your advice would help:

1. Where is a good place to put the InspectionSettings resource? I am inclined to put it in crates/rmf_site_picking/src/select/resources.rs
2. For checking if the SHIFT key goes up or down, I was thinking to extend deselect_on_esc to check for .pressed(…) or .released(…) on SHIFT and then, updating the InspectionSettings resource accordingly.

[JohnTGZ]

Okay that might not work, because the input to deselect_on_esc is a KeyCode and not ButtonInput<KeyCode>. Im thinking that using an async service might not be the best either, and perhaps a typical Bevy system should suffice

[mxgrey]

I don't think we should fragment the selection pipeline like this. Let's just update the existing Selection resource to contain a HashSet<Entity> instead of an Option<Entity>, and we can leave it to downstream systems to decide what to do with the hash set.

In the interest of generalizing Selection I think we should change it into a struct, e.g.:

pub struct Selection {
    pub selected: HashSet<Entity>,
}

We should get rid of the Deref and DerefMut and instead provide helper functions:

impl Selection {
    pub fn get_single(&self) -> Option<Entity> {
        if self.selected.len() > 1 {
            return None;
        }

        self.selected.iter().next().copied()
    }
}

[JohnTGZ]

Agreed. Having get_single as a method is a good idea and tidies up the use of Selection in other areas

[JohnTGZ]

Hi @mxgrey , thank you for all the suggestions. I have re-implemented the multi-selection mechanism as follows:

1. Replace Option<Entity> in Selection resource with HashSet<Entity>, added a get_single() method and update services and systems to accommodate this change.
2. Added a system that tracks changes in the SHIFT key and updates a InspectorSettings resource to trigger multi-selection. This resource can be read by services downstream to determine whether to enable multi-selection or not.
3. Updated the Inspector widget to display a list of entities currently selected (if multi-selection is enabled). I think visually this could look better so I am thinking to something of a visual separator between the different entities

I have yet to implement the de-selection mechanism but would like to get some feedback on the current implementation.

[JohnTGZ]

Alright, I have made significant changes since the last time I requested a review, so I will summarize the entire PR here:

Multi-selection of instances in the world scene:

• Holding down SHIFT key should enable multi_select:
  • Unselected entities can be added to the existing selection by clicking on them
  • Deselect an entity by clicking on an already selected instance
• Releasing the SHIFT key should disable multi_select:
  • Only a single instance can be selected. Clicking on an already selected instance just re-selects it.
  • If multiple instances are already selected, clicking on an instance will clear the current selections and result in just that one entity being selected.

Added the ViewMultiSelection collapsible widget into the Standard Properties Panel.

• This widget is only visible when at least 2 instances are selected.
• Displays all selected instances and has buttons to deselect and delete individual entities
• Has a MultiEditPoseWidget which allows us to modify the XYZ Transform and yaw of all selected instances at once.

Points for discussion:

• When there are already multiple selected instances, the instances are deselected by sending a Select event with that same entity id, which is handled in selection_update. I wonder if this mechanism might have potential problems for any future workflow/extensions.
• In MultiEditPoseWidget, the displayed yaw is currently set as the first selected instance but is not representative of the yaw of the other selected entities. I wonder how else can we better handle this? I was thinking not to display any yaw value, but simply have the user specify an offset, but that would make it challenging for the user to compare and adjust values too.

[mxgrey]

Thanks @JohnTGZ , there are a lot of great concepts being introduced in this PR.

I've left some recommendations for how I think we can tighten up the implementation.

[mxgrey]

I think we can cut down on a lot of unnecessary changes in this PR by tweaking this API:

pub fn new(candidate: Option<Entity>) -> Self {
    Self {
        candidate: candidate.map(|c| SelectionCandidate::new(c)),
        multi_select: false,
    }
}

pub fn multi_select(mut self, on: bool) -> Self {
    self.multi_select = on;
    self
}

This will allow us to revert the changes that were made to many of the existing uses of Select::new. For new cases where we need to include the multi-select behavior, we can do e.g.

Select::new(hovered.0)
    .multi_select(self.inspection_settings.multi_select)

[mxgrey]

Same as before, let's take the multi_select argument out of here and just use the .multi_select(_) builder pattern.

[mxgrey]

Small nitpick: Let's define Default for InspectionSettings, e.g.

impl Default for InspectionSettings {
    fn default() -> Self {
        Self {
            multi_select: false,
        }
    }
}

then we can just insert InspectionSettings::default() here.

[mxgrey]

This is going to cause InspectionSettings to be modified every single schedule frame, which is going to confuse any system that wants to react to changes in the InspectionSettings resource (such systems will be told that a change has happened on every single frame).

There are two ways we could consider for improving this:

1. Check if left shift is pressed every frame and only update InspectionSettings if it doesn't match:

let multi_select = keyboard_input.pressed(KeyCode::ShiftLeft);
if inspection_settings.multi_select != multi_select {
    inspection_settings.multi_select = multi_select;
}

2. React to changes in the button status

if keyboard_input.just_pressed(KeyCode::ShiftLeft) {
    inspection_settings.multi_select = true;
}
if keyboard_input.just_released(KeyCode::ShiftLeft) {
    inspection_settings.multi_select = false;
}

I think overall approach (1) is preferable to approach (2). All of the different keyboard_input methods are O(1) so there's not much of a performance concern for either approach, but approach (2) has some very small risk of being "sticky" if there's ever a situation where pressed/released signal gets missed.

[mxgrey]

Seems we can remove this. It isn't getting used as a system param because you call world.get_resource::<Selection>() instead, pulling the resource directly from the world instead of from this struct.

[mxgrey]

It's not wrong to use for_each like this, but it's not considered best practice in Rust. When a regular for-loop can suffice, you're recommended to use one:

for previously_selected in &selection.selected {
    if let Ok(mut selected) = query_selected.get_mut(*previously_selected) {
        selected.is_selected = false;
    }
}

[mxgrey]

Here as well, you should just use a regular for-loop.

[mxgrey]

Use a regular for-loop instead of .for_each.

[mxgrey]

This message seems odd. With the refactor that I recommended earlier, I don't think we need to do this check or produce this label.

[mxgrey]

Notice how the selector_widget also checks for .hovered() and then emits the Hover(_) event. We should do likewise to help users know which item is being considered for removal before they click.