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

Add internal attributes in "Notes" tab #1744

Merged
merged 19 commits into from
Mar 5, 2023
Merged

Conversation

cbentejac
Copy link
Contributor

@cbentejac cbentejac commented Jul 29, 2022

Description

This PR adds support for internal attributes, which are attributes that exist for all nodes independently from their type.
Internal attributes are displayed in the "Notes" tab of the graph editor (also added in this PR) and can be set for each node individually and independently.

Four internal attributes are added with this PR: comments/invalid comments (to write some comments regarding a node), label (to rename a node), and color (to change a node's color).

Regarding the "comments" internal attribute, an icon is added to the node's header if a comment has been set, and hovering over that icon displays a tooltip containing the comment.

Internal attributes will be written to the template files if and only if they have non-default values (so they need to have been set). If no internal attribute has a non-default value, the "internalInputs" entry is not even written in the template file. The unit test checking on version/description conflicts in templates is updated accordingly.

A new property, uidIgnoreValue, is added to the generic Attribute object. Its goal is to leave an attribute out of the node's UID computation if its value is set to the value it specifies. By default, it is set to None and any attribute that is considered for the UID computation will be taken into account. In the case of the internal attributes, uidIgnoreValue="" for the "invalidation" attribute: this prevents from invalidating all the existing graphs simply because this attribute has been added; instead, nodes will be invalidated because of that specific attribute if and only if its value is set to a non-empty string (when the invalidation is actually wanted).

Features list

  • Add support for internal attributes (attributes that will appear in the "Notes" tab)
  • Support multiline text field parameters and color selector text field parameters
  • Add a first set of internal parameters: "comments", "invalid comments", "label" and "color"
  • Add a "comment" icon in the header of nodes which have their "comment" attribute set, and a tooltip displaying that comment when the icon is hovered
  • Add a new attribute property, uidIgnoreValue, which allows to ignore an attribute during the node's UID computation depending on its value

Implementation remarks

uidIgnoreValue is set to None by default for all types of attributes. It also is only exposed in the constructor of the StringParam attributes.

@cbentejac cbentejac marked this pull request as draft July 29, 2022 16:01
@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from 9f49363 to 457c826 Compare August 1, 2022 07:08
@cbentejac cbentejac marked this pull request as ready for review August 1, 2022 07:09
@cbentejac cbentejac force-pushed the dev/internalAttributes branch from 457c826 to 365c76f Compare August 1, 2022 10:00
meshroom/core/node.py Outdated Show resolved Hide resolved
meshroom/core/node.py Outdated Show resolved Hide resolved
@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from 4d3ed5c to 1387090 Compare August 2, 2022 12:24
Copy link
Member

@fabiencastan fabiencastan 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 deal with node upgrade of internal attributes.

@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from 40e6783 to 32eacc3 Compare August 31, 2022 15:58
@cbentejac
Copy link
Contributor Author

Need to deal with node upgrade of internal attributes.

Done

@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from 8022140 to 3aecc8d Compare September 8, 2022 10:14
@fabiencastan fabiencastan added this to the Meshroom 2022.1.0 milestone Oct 10, 2022
@fabiencastan
Copy link
Member

fabiencastan commented Oct 10, 2022

  • If we rename one internalAttribute, the upgrade is not working fine.
  • Add an icon on nodes with comments (with the comment in tooltip)

@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from 9644f9f to 5cee129 Compare October 13, 2022 16:21
@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from f517502 to 218b267 Compare October 24, 2022 09:08
@mh0g mh0g self-requested a review November 9, 2022 13:32
@mh0g
Copy link
Contributor

mh0g commented Nov 9, 2022

On windows via the gui:

  • colors tested, works
  • invalidation tested, works
  • renaming node tested, works

Also tested with meshroom batch

Note: would it be possible to include the custom name (if any), in the status file created by each of the node?
As the folder name is not affected by the pr, this could be a convenient way to better differentiate two nodes of the same types.

@mh0g
Copy link
Contributor

mh0g commented Nov 9, 2022

Does not break backward compatibility on projects from current develop .

@cbentejac cbentejac force-pushed the dev/internalAttributes branch from 752bb11 to bdb5097 Compare November 18, 2022 16:32
@cbentejac cbentejac force-pushed the dev/internalAttributes branch from c74e3d0 to 9657771 Compare February 2, 2023 16:22
meshroom/core/node.py Outdated Show resolved Hide resolved
cbentejac and others added 17 commits February 15, 2023 08:36
Add two internal attributes, "Comment" and "Invalid comment", in
a specific "Notes" tab, which will contain any further internal
attribute. Internal attributes exist for all nodes.
Setting the "label" internal attribute allows to give a custom
label to replace the node's default label.
Setting this attribute allows the user to change the color
of a node, either by directly providing an SVG color name or an
hexadecimal color code, or by picking a color with the selector.
If the "Comments" internal attribute is filled, add a corresponding
icon in the node's header, as well as a tooltip that contains the
comment.
"hasAttribute" was previously never called before attempting to
access an attribute. With the addition of internal attributes, we
want to check the attribute's/internal attribute's before accessing
it to avoid KeyError exceptions. "hasAttribute" (and the newly added
"hasInternalAttribute") would not parse the attribute's name before
checking for its existence, meaning that errors could be generated for
list or group attributes as their checked name could contain other
elements (e.g. "featuresFolder[0]" for a ListAttribute named
"featuresFolder").
…nly default values

Non-default internal attributes need to be written in the templates,
but the "internalInputs" entry in the dictionary should not be written
at all if all the internal attributes are set to their default values.
If some internal attributes are saved in the templates, their description
should be checked just like the input attributes to ensure there are no
conflicts.
pyCompatibility has been removed at the same time as Python 2 support.
The "label", "color" and "comment" properties are not constant anymore,
their changes in value are notified with the internalAttributesChanged()
signal, like the "invalidation" property.

This implies that the connection on "internalAttributesChanged" on the
QML side is not needed anymore.
…tes' tooltip

The tooltip now displays both the invalidation message, followed by the
comments. The invalidation message is displayed first in bold font,
followed by an empty line and the comments in regular font.

The tooltip now appears if at least one of the invalidation or comment
messages exists.

The invalidation and comment messages are formatted with HTML tags prior
to their display. The descriptions of both attributes is also updated
to indicate which one is displayed in bold or regular font.
@cbentejac cbentejac force-pushed the dev/internalAttributes branch 2 times, most recently from d601e4a to 6d9eec5 Compare February 15, 2023 10:39
meshroom/core/desc.py Outdated Show resolved Hide resolved
By default, an attribute that belongs to the UID group 0 is taken into
the node's UID computation independently from its value, as long as it is
enabled.

When such an attribute is added to a node's list of attributes, it
automatically invalidates all computations made for this node prior to
its addition.

This commits adds a new attribute property, "uidIgnoreValue". This property
determines whether the attribute must be taken into consideration during
the node's UID computation: if the value of the attribute is the same as
"uidIgnoreValue", then it should be ignored; otherwise, it should be taken
into account. By default, "uidIgnoreValue" is set to "None", meaning that
any attribute that may be considered during the UID computation will be
taken into account.

In the context of the internal attributes, "uidIgnoreValue" is set to empty
string, so the "invalidation" attribute will not automatically
invalidate 100% of the nodes from existing graphs until its value is set
to a non-empty string.
@cbentejac cbentejac force-pushed the dev/internalAttributes branch from 6d9eec5 to 311ab9c Compare February 17, 2023 14:37
meshroom/core/node.py Outdated Show resolved Hide resolved
hasInternalAttribute() should not support more cases than internalAttribute()
@fabiencastan fabiencastan merged commit ce2085f into develop Mar 5, 2023
@fabiencastan fabiencastan deleted the dev/internalAttributes branch March 5, 2023 12:46
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.

4 participants