iTwin / presentation

Monorepo for iTwin.js Presentation Library
https://www.itwinjs.org/presentation/
MIT License
5 stars 0 forks source link

"Element" selectionScope is adding additional elements to the selectionSet #767

Open Rock2000 opened 3 weeks ago

Rock2000 commented 3 weeks ago

I have schema elements that can have child elements. When the user selects the parent element, using the "Element" selectionScope, the unified selection module adds the child elements to the selectionSet. So instead of the single element that was selected being in the selectionSet, there are 3. This makes it look like the user multi-selected 3 elements to tools that use the selectionSet. For example we have manipulators that look at the selectionSet, and only if a single element is selected do we create manipulators. If there are multiple elements in the selectionSet we don't create any manipulators. So in this case, the user sees no manipulators even though they selected a single element.

It seems like for the "Element" selectionScope, the selectionSet should just be the elements the user manually selected.

Rock2000 commented 2 weeks ago

Somewhat related, I wonder if the selection scope stuff options would be better exposed in the property pane. So far our users do not understand how the selection scope works. If we added a small toolbar or something in the property pane which allowed the user to navigate up to the parent element (change to select the parent) or navigate to a child via a flyout menu or something (select the child element) I think that would be easier for users to discover and grasp.

saskliutas commented 2 weeks ago

Somewhat related, I wonder if the selection scope stuff options would be better exposed in the property pane. So far our users do not understand how the selection scope works. If we added a small toolbar or something in the property pane which allowed the user to navigate up to the parent element (change to select the parent) or navigate to a child via a flyout menu or something (select the child element) I think that would be easier for users to discover and grasp.

PropertyGrid component from @itwin/property-grid-react package has support for navigating through element ancestors: https://github.com/iTwin/viewer-components-react/tree/master/packages/itwin/property-grid#ancestor-navigation

To enable it you need to provide ancestorsNavigationControls prop to PropertyGrid component. You can find an example in Usage section: https://github.com/iTwin/viewer-components-react/tree/master/packages/itwin/property-grid#usage

grigasp commented 2 weeks ago

To add some context, I'd like to explain what the unified selection system does that causes the described issue.

The workflow is as follows:

  1. User picks an element in graphics view. This puts the element into selection set, hilite set and raises an onChange event.
  2. Unified selection system captures the selection set change event and syncs it to unified selection. It adjusts the ids in selection set based on selection scope and puts them to unified selection storage. The issue description mentions that the active selection scope is "Element" - in this case the picked element is added to the storage. Adding an element to unified selection storage raises the selectionChangeEvent event.
  3. Unified selection system captures the unified selection change event and syncs it to core-frontend. It requests a hilite set for the selected instances and the result is applied on core-frontend's hilite and selection sets. When requesting hilite set for a geometric element, the result contains id of the element itself and all its recursive children - as a result, those children ids are added to hilite and selection sets, causing the described problem.

More details about the two concepts are in the unified-selection package README's Hilite sets and Selection scopes sections. While they're both related to selection, at the same time they're both independent of each other:

grigasp commented 2 weeks ago

What has been considered so far:

  1. When a hilite set is retrieved based on unified/logical/global selection upon its change, only update viewport's hilite set, but not selection set.

    • The result would be that child elements of the selected one would still be hilited, but viewport tools wouldn't see them as selected.
    • This could break existing viewport tools that rely on elements' selection. Because the elements are hilited, users will think they're selected as well, and will expect tools to work on all of them, but tools will only see the parent as selected.
  2. Only apply hilite and selection set changes upon unified/logical/global selection change when the change happens with other than "Element" selection scope active.

    • This would solve the problem if the active selection scope is "Element", but not otherwise.
    • This would lead to situations when the same element is selected, but is hilited in the viewport differently. Selecting the element with "Element" scope would only hilite the element itself, but selecting the exact same element with the different scope would hilite its children too. Users might find that confusing.
    • Some components allow selecting non-graphical elements that have graphical children. At the moment selecting such an element always hilites its children in the viewport, no matter what the active scope is. With the change, users would have to make sure the active scope is not "Element". As such, this could be considered a breaking change.
    • This would add another responsibility to selection scope concept. At the moment it's only purpose is to tell how to map element picked in the viewport to what gets into unified/logical/global selection. With the change, it would also tell how elements should be hilited. While the current purpose applies to only elements picked in the viewport, the second purpose would apply to selections in all other components too. Seems confusing.

So, TL;DR: we don't have a solution that wouldn't break existing workflows. User workflows need to be investigated, considering both - read-only and the new read-write - scenarios. Will try to get some UX input on the matter.

grigasp commented 1 week ago

I feel like it's important to note that there have been user complaints about how selection scopes work in read-only apps. The situations were related to deep parent-child relationships, e.g.:

With the above example elements' hierarchy, users wanted to pick PhysicalObject in the view and see properties of the Segment. However, having 3 selection scopes doesn't suffice here - they can only target:

It's important to note that what you get depends on what you happen to click in graphics view, and you don't really know that until you click. So if Pipe was a graphical element and you happened to click on it, then "Assembly" scope would select Segment for the user. Because of this, I believe adding additional selection scopes would probably only make things more complicated, but wouldn't solve this issue.

So far, such complaints were resolved by suggesting users to traverse selected elements' hierarchy using Properrty widget's ancestor navigation feature. However, it's not the most convenient, because of the need to adjust selection every time they select an element.

FelixGirard commented 1 week ago

We also had a need where a physical object is tied to a spatial one (in another partition). Since they are on different partition, they can't use the parent-child relationship.

We would still want some features to be the same, for example the way multiple elements get highlighted even if only one is displayed in the property panel.

It would be nice to give us more control over those steps to add our custom rules.