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

Discard attribute changed callbacks during graph loading #2598

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

yann-lty
Copy link
Member

@yann-lty yann-lty commented Nov 14, 2024

Description

Ensure attribute changed callbacks are not triggered on graph loading.

When loading a graph, we expect nodes to be deserialized in the exact state they were saved.
Attribute callbacks may modify attribute values and impact the UID of the nodes.
This was leading to UID compatibility conflicts on file opening, for Graph containing Nodes using such mechanisms.

Features list

  • Add "strictCompatibility" mode for graph loading
  • Discard attribute callbacks when graph is being loaded

Implementation remarks

While this behavior was covered by a test, it turned out to be a false positive.
The error was hidden behind the fact that the tested Node was actually UID conflicting due to the callback, and swapped to a Compatibility Node. Therefore, attribute values were restored to the serialized ones, with expected values.
The introduction of the "strictCompatibility" mode allows to test this behavior more efficiently.

@yann-lty yann-lty force-pushed the fix/attributeCallbackOnGraphLoad branch 2 times, most recently from 9acd7f8 to db5dc3b Compare November 14, 2024 18:08
Provide a way to control how to handle node compatibility issues
when loading a Graph with the `strictCompatibility` parameter.
Introduce a new error type, GraphCompatibilityError, to be raised
when loading a Graph with compatibility issues with strictCompatibility enabled.
Update tests that should fail if nodes are loaded as
CompatibilityNodes (resulting in false positives).
@yann-lty yann-lty force-pushed the fix/attributeCallbackOnGraphLoad branch from db5dc3b to 61d3590 Compare November 14, 2024 18:11
@cbentejac cbentejac added this to the Meshroom 2024.1.0 milestone Nov 15, 2024
Copy link

@waaake waaake left a comment

Choose a reason for hiding this comment

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

Did a test around this, the current changes did solve part of the problem which was loading a graph with no onAttrChanged callbacks triggered and no unwanted CompatibilityIssues raised.

My tests involved having a ChoiceParam Combo Box getting values populated in an onAttrChanged function,
and when the graph is saved and loaded back, the last value on the ChoiceParam is retained, but is shown in Red or invalid in the GUI, as the value is no longer part of the default values on the ChoiceParam which are loaded when the node gets deserialised.

Heart of the issue comes from the fact, we're not serialising the current set of values on the param in the graph file to be able to load them back upon loading.

Guessing this issue came into being with the introduction of allowing dynamic values on the ChoiceParam, as now the choiceParam can get updated values during any method calls.

Thinking about the best way would be to serialise values from such params.

Graph: The loaded Graph instance.

Raises:
GraphCompatibilityError: If the Graph has node compatibility issues and `strictCompatibility` is False.
Copy link

Choose a reason for hiding this comment

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

Suggested change
GraphCompatibilityError: If the Graph has node compatibility issues and `strictCompatibility` is False.
GraphCompatibilityError: If the Graph has node compatibility issues and `strictCompatibility` is True.

I believe this should be True for the graph to raise an Error ?

@waaake
Copy link

waaake commented Nov 15, 2024

To be able to reproduce what I did, please use the nodes from https://gist.github.com/waaake/d96a5ae6bd8f00c4a7ee0f99cd6a9242

Steps:

  1. Create FirstNode.
  2. Create Second Node.
  3. Connect FirstNode.x -> SecondNode.y.
  4. SecondNode.choice should now have the values as ["A", "B", "C", "D"] with the current value set as "D".
  5. Change the value of this parameter to "B".
  6. Save the graph.
  7. Load the graph back.
  8. Now the value on the choice parameter is set as "B" but marked invalid with a Red colour.
  9. If we click on the choice parameter, the values are no longer present causing the current value on the param to be deemed as invalid.

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.

3 participants