Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support display and connection of attributes that are part of GroupAttributes #2544

Draft
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

cbentejac
Copy link
Contributor

@cbentejac cbentejac commented Sep 20, 2024

Description

This PR is based on #2538 and must be merged once #2538 itself has been integrated into develop.
It also relies on some UI fixes introduced in #2563.

The main feature of this pull request is that it adds the support of connections between attributes that are part of a GroupAttribute. Prior to this PR, these attributes could neither be connected individually, nor be displayed in the Graph Editor; they were however visible and editable in the Node Editor.

GroupAttributes are now displayed in the Graph Editor with right chevron, which means they can be expanded when clicked upon. When this happens, all the children attributes of the clicked GroupAttribute are displayed below it, displayed in a tree view with the indentation reflecting their depth level. Children attributes are displayed recursively, and a GroupAttribute that contains a child GroupAttribute will have it displayed as well (but collapsed by default).

Attributes that are connected will always be displayed, even if they belong to GroupAttributes that are currently collapsed. If an edge connecting such an attribute is removed, then this attribute will be updated to reflect its parent's visibility status.

TreeView

Additionally, some unit tests are added.

Features list

  • Add the notion of depth for attributes that are children of a GroupAttribute. An attribute that has no parent will have a depth of 0, and it will increase of 1 for every parent;
  • Add a flatStaticChildren property for every attribute that may have children that contains the flattened list of said children;
  • Correctly evaluate links when they reference children attributes, hence adding the support of the connection of individual attributes within groups;
  • For children attributes, have their parent's exposed property take precedence over their own. This for example prevents having attributes within groups that are exposed while the group itself is not, which would break the hierarchy of the display for these GroupAttributes;
  • Add an icon to expand or collapse GroupAttributes and display their children attributes in their node in the Graph Editor with a tree view. These children attributes are graphically connectable;
  • Display the full name of the attribute in its tooltip in the Graph Editor. If an attribute childAttribute is a child of a group groupAttribute, its name will be displayed as groupAttribute.childAttribute, making the relationship between the two obvious.

@cbentejac cbentejac added this to the Meshroom 2024.1.0 milestone Sep 20, 2024
Base automatically changed from dev/simplifyNodeAPI to develop September 20, 2024 20:57
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 2 times, most recently from 12210e5 to 12bc150 Compare September 23, 2024 16:57
@cbentejac cbentejac changed the title Add display of attributes that are part of GroupAttributes Add display of attributes that are part of GroupAttributes and allow connection of these attributes to other ones Sep 23, 2024
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch from 66ce81e to 0e0455d Compare September 26, 2024 15:07
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch from 0e0455d to 0cdfd8b Compare October 4, 2024 16:12
@cbentejac cbentejac changed the base branch from develop to fix/minorUiFixes October 7, 2024 09:50
@cbentejac cbentejac self-assigned this Oct 7, 2024
@cbentejac cbentejac marked this pull request as ready for review October 7, 2024 16:20
@cbentejac cbentejac changed the title Add display of attributes that are part of GroupAttributes and allow connection of these attributes to other ones Support display and connection of attributes that are part of GroupAttributes Oct 8, 2024
Base automatically changed from fix/minorUiFixes to develop October 16, 2024 07:42
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 2 times, most recently from 3e6b6c0 to ab0a2b9 Compare October 21, 2024 09:04
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 2 times, most recently from 9bc279f to 0d2954e Compare November 26, 2024 11:45
@cbentejac cbentejac marked this pull request as draft November 29, 2024 17:49
Copy link
Contributor Author

@cbentejac cbentejac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check if the generation of the attributes within a group in the node cannot be performed in the same way as in the node editor.

@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch from 0d2954e to 7b5d84f Compare December 4, 2024 17:36
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 3 times, most recently from 6a34cd4 to a5d7265 Compare December 12, 2024 19:32
@cbentejac cbentejac marked this pull request as ready for review December 13, 2024 08:39
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch from a5d7265 to cc473d2 Compare December 13, 2024 10:04
@cbentejac cbentejac requested a review from yann-lty December 13, 2024 10:09
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 2 times, most recently from ff4f5d5 to 449cc90 Compare December 13, 2024 13:50
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 98.83721% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.38%. Comparing base (faff99f) to head (6aae86c).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
meshroom/core/node.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2544      +/-   ##
===========================================
+ Coverage    69.90%   70.38%   +0.48%     
===========================================
  Files          121      123       +2     
  Lines         7078     7173      +95     
===========================================
+ Hits          4948     5049     +101     
+ Misses        2130     2124       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 3 times, most recently from 22a80d4 to 78419fa Compare December 19, 2024 16:19
For attributes in `GroupAttribute` and `ListAttribute`, the notion of
parent attribute exists through the `root` property. As a parent can
itself have a parent, the `depth` property describes how many levels
there are between the attribute and the root level.

A value of 0 means that the attribute is at the root level, and it
increases as it gets deeper.
`flattenedChildren` returns the list of all the attributes that refer
to this attribute as their parent (either direct or indirect) through
the `root` property. The search for the children attributes is recursive
and alllows to retrieve at once all the nested attributes, independently
from their level.

At the moment, only `ListAttribute` and `GroupAttribute` will return a
non-empty list of flattened children attributes.
…escription's

The `exposed` property, which determines whether the attribute is
displayed on the upper part of the node in the Graph Editor, is set
for each attribute individually in their node's description.

If an attribute has a parent (meaning it depends on a `GroupAttribute` or
a `ListAttribute`) whose `exposed` property value differs, it does not
make sense to display it separately from it. The attribute's `exposed`
should align with its parent's.
If an attribute belongs to a `GroupAttribute` or a `ListAttribute`,
it has a parent, and its full name is "parentName.attributeName".
Instead of displaying only "attributeName" in the tooltip, this commit
now displays "parentName.attributeName" to ensure that the link is obvious.
Rename `getFlattenedChildren` into `getFlatStaticChildren` and have a
default version for all the Attributes, as well as a dedicated one for
GroupAttributes. Children of ListAttributes are currently not taken
into account when gathering the list of children.
The test ensures that if the attributes within `GroupAttributes` are
connected to each other, the graph can be saved and reloaded without
triggering compatibility issues for these nodes.
…lapsed

Just like any other connected attribute, any child attribute within a
Group that is either the source or destination of an edge needs to remain
on display, independently from the status of its parent (collapsed or
expanded).
…bility

When edges could exist while being invisible in the graph, pins needed
to be enabled/disabled manually according to the visibility of the edge.
This was especially important when closing the application and removing
all the (hidden) edges as the pins needed to be properly destryed as well.

Edges are not hidden anymore, and any attribute that should be hidden
but is in fact connected remains visible as long as the connection exists.
If an attribute has a parent (`root is not None`), directly use the
parent's values for the `depth` and `exposed` properties instead of
setting them from scratch everytime from a `while` loop.

If the attribute has no parent, then `depth = 0` and `exposed` is set
directly from the description.
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch 2 times, most recently from 448bbf3 to 767377c Compare December 30, 2024 19:01
…tes`

As connections between groups are currently not supported, add connecting
any `GroupAttribute` to the list of connections to refuse.
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch from 767377c to 2cc458a Compare December 31, 2024 12:25
Prior to this commit, the edge that was being dragged to be connected
appeared as soon as the `pressed` signal was emitted by the mouse. Now
that the user can actually click (press & release) without dragging the
mouse to expand or collapse groups, we want to be able to distinguish
cases where the user presses the mouse to click (to expand/collapse
groups) from those where the user presses the mouse to drag it (to
connect edges).

Instead of triggering the drag as soon as the `pressed` signal is
emitted, we wait to see if the mouse moves significantly enough to
indicate the will to drag; an arbitrary limit of 10 pixels along the
X- or Y-axis is used to determine that the user means to drag the mouse.
@cbentejac cbentejac marked this pull request as draft December 31, 2024 12:56
@cbentejac cbentejac force-pushed the dev/connectGroupAttrs branch from 2cc458a to 6aae86c Compare December 31, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant