Fix attribute value change propagation and callback handling #2586
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
Introduce test fixture to simplify graph testing in isolation.
Suite of tests to validate the behavior of desc.Node 'on{Attribute}Changed' behavior.
Test value change propagation and callback trigger for attribute connected to Group and ListAttribues.
Move the attributeChanged callback logic within Node, as this is a Node level mecanism (callbacks being declared on the node desc). This commit also moves the attribute valueChanged signal outside of the Attribute's constructor. This seems to be the root cause to undesired side-effects breaking Qt's evaluation system, data model access and consequently Meshroom's UI.
New test suite that focuses on testing the behavior of the attribute changes callback system when such an attribute is connected to an upstream dynamic output value.
yann-lty
changed the title
With valueChanged signal being connected outside of the Attribute's constructor, the emitSignals logic is not required to prevent early valueChanged propagation.
cbentejac
approved these changes
Oct 29, 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 aims to improve attribute value change propagation, introduced with the concept of "onAttributeChanged" callback system on node descriptors.
The goal is to fix data model access issues happening randomly in Meshroom, leading to broken UI components as shown below in the ImageGallery:
Features list
Implementation remarks
Part of the current implementation lives in the Attribute class, binding the
valueChangedsignal to a slot directly within the class constructor. This seems to be the root issue that breaks Qt's evaluation system.This PR move that connection logic to the
attributeFactoryfunction, and bind it to a slot defined on theNodeclass, where all the attribute callback and value change propagation is now dealt with.Unit tests were added to validate the expected behavior of this system.
And while not being able to write a specific test to validate this PR fixes the main visible problem stated in the description, many manual tests that were failing fast on the current implementation are now working fine.