Refactor Node selection for better UX and performance #2605
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yann-lty
force-pushed
the
fix/nodeSelectionPerfs
branch
from
dd49c79 to
d6a89cb
Compare
fabiencastan
approved these changes
Dec 5, 2024
There was a problem hiding this comment.
Great work!
Notes:
- The inclusion of InputNode in the selection should not impact the compute/submit option (in the same way that the "Delete Data" does not exist on InputNodes but is allowed on a selection of nodes including InputNodes). On the other hand, one CompatibilityNode included in the selection blocks the launch of a compute/submit as it should be.
- In some software the selection is live while defining the selection (but not sure if the limited added value is worth regarding the perf impact).
Implementing this method allows to use QObjectListModel in combination with other Qt classes that can act on a model (eg: QSelectionItemModel, QSortFilterProxyModel...).
Forward mouse 'released' and 'clicked' events for giving more control over node selection management.
- SelectionBox: generic Selection box component. - DelegateSelectionBox: specialized SelectionBox to select model delegates from an instantiator (Repeater, ListView). Also Introduce a Geom2D helper class to provide missing features for intersection testing in QML.
Switch selection management backend to a QItemSelectionModel, while keeping the current 'selectedNodes' API for now. Use DelegateSectionBox for node selection in the graph, and rewrite the handling of node selection / displacement.
Add and use an explicit method to remove the currently selected nodes in a graph.
Avoid having the node context menu always evaluating the current state of the selected nodes for its own display, by dynamically creating it on demand with a Loader. Use callbacks for recomputing/resubmitting actions, instead of storing state in the UI components.
Delay the loading of the node context menu once the node selection has been updated, for it to consider the proper selection.
Re-write the computability status of the current node selection as properties within the node menu component. Note that this should be further improved to better scale with the size of the selection, as it requires to traverse the graph for each node.
Avoid to evaluate the computability/submitability status of each node twice by caching the information when creating the node context menu.
Remove dynamic tooltip for cut/copy actions that displays all selected node names: - This inline textual information is hard to process as a user. - Avoid binding to and iteration over the selection.
Convenient function to directly work on the current node selection.
Expose `getSelectedNode` that relies on the QItemSelectionModel for imperative code in QML that still requires to access the selected node instances.
yann-lty
force-pushed
the
fix/nodeSelectionPerfs
branch
from
d6a89cb to
a3268f4
Compare
Instead of connecting to onSelectionChanged, use ItemSelectionModel.hasSelection property, that can be use for direct bindings with the same behavior. https://doc.qt.io/qt-6/qml-qtqml-models-itemselectionmodel.html#hasSelection-prop
Closer to the standard behavior of the Ctrl modifier key for selection.
fabiencastan
approved these changes
Dec 9, 2024
fabiencastan
added
the
feature
fabiencastan
changed the title
Implement additive selection behavior when selecting downstream nodes from a node, using Alt+Shift+Click.
fabiencastan
approved these changes
Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses several issues related to the Node selection management, mostly related to performance and user experience.
Selection performance
Selection precision
Features list
Implementation remarks
Selection management
This PR changes the selection management backend to use QItemSelectionModel, instead of manually maintaining an extra QObjectListModel. This allows to rely on Qt to perform selection commands (ClearAndSelect, Deselect, Toggle..).
It also fixes the main issue of having a lot of properties binding to the selection model, which was on top of that emitting its notify signal way to often.
Node context menu
The node context menu was one of the most costly component, due to many direct bindings it had with the selection model.
And it was created at application startup and living until application shutdown, even if unused.
It is now created dynamically only when explicitly required, and evaluate the computability status of selected nodes only on creation.
Selection User Experience
Precision
The previous implementation was performing the box select on the Python side, only considering the default node sizes.
This led to imprecision in the interactive selection, where the varying visual size of the Node was never taken into account.
Now, the selection happens on the QML side.
Modifiers
This PR changes the use of some modifiers to lean towards more standard UX.
Note: To enable this change, the alternate view panning shortcut (Shift+Drag) has been moved to Alt+Drag.