Updating the colors of the ports according to the Function State #12826

filipeotero · 2022-04-25T17:10:17Z

Purpose

This PR contains the code improvement that changes the colors of the input/output of the nodes by checking the class and library of the object to define if the node is a 'Function'.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Reviewers

@mjkkirschner @QilongTang @Amoursol

FYIs

@RobertGlobant20 @jesusalvino

mjkkirschner · 2022-04-25T20:50:16Z

As I mentioned in the thread, this is not unique to python nodes, but any class that inherits from NodeModel.

filipeotero · 2022-04-25T23:17:50Z

As I mentioned in the thread, this is not unique to python nodes, but any class that inherits from NodeModel.

Sorry, I've understood that this is an issue that happens for Python Nodes because it has default values when created.
Could you be more specific? Do we need to check the output result of the node then? If yes, is there any specific property that returns the type of the return?

Amoursol · 2022-04-26T13:43:21Z

As I mentioned in the thread, this is not unique to python nodes, but any class that inherits from NodeModel.

Sorry, I've understood that this is an issue that happens for Python Nodes because it has default values when created. Could you be more specific? Do we need to check the output result of the node then? If yes, is there any specific property that returns the type of the return?

@filipeotero as Mike suggested, this same problem will exist on other nodes. Take the following example of the "Color Range" node:

This has an issue with the "Function" grey-bar output also. It also has an issue with the input vertical bars too which we didn't catch for Global Launch :( Essentially, the input ports are also satisfied, but show as unsatisfied.

FYI @QilongTang

filipeotero · 2022-04-27T21:50:21Z

Updated the code to check the Result of the node by getting the CachedValue of the model.
Some examples of the nodes after this implementation:

@Amoursol Are there more examples to be covered for this issue? Thanks!
FYI: @QilongTang @mjkkirschner

mjkkirschner · 2022-04-27T21:59:46Z

src/DynamoCoreWpf/ViewModels/Core/InPortViewModel.cs

-            //Is in function state
-            if (node.NodeModel.IsPartiallyApplied)
+            //Boolean that defines if the node has a default value from the cache value
+            isReturningDefaultValue = !string.IsNullOrEmpty(node.NodeModel.CachedValue?.StringData);


I don't understand this logic, many nodes return strings which are not default values, and many default values are not strings?

I don't understand this either. How can you say if a node has default value(s) assigned to one or more input ports if you examine its output (cachedValue)?

Instead of getting the info of a Function State from isPartiallyApplied, I'm getting the info from the CachedValue because It's what is appearing in the little console below the node. The idea was to identify if the node has a result, even though It's a null result.
Another thing is that I had to differentiate between the output 'Function' (in which the inputs have to change the color between red/blue and the output grey) and other ones (inputs always blue and output disabled).
I would love to hear any other suggestions from you guys!
Thanks.

I don't think we need this flag isReturningDefaultValue.

mjkkirschner · 2022-04-27T22:01:04Z

src/DynamoCoreWpf/ViewModels/Core/InPortViewModel.cs

+            isReturningDefaultValue = !string.IsNullOrEmpty(node.NodeModel.CachedValue?.StringData);
+
+            //This variable checks if the node is a function class
+            bool isFunctionNode = node.NodeModel.CachedValue != null &&


I think - there are better options - this is too fragile and could break. Let me look.

src/DynamoCoreWpf/ViewModels/Core/OutPortViewModel.cs

aparajit-pratap · 2022-04-28T03:14:42Z

src/DynamoCoreWpf/ViewModels/Core/InPortViewModel.cs

@@ -321,7 +342,7 @@ private void SetupDefaultPortColorValues()
                PortBorderBrushColor = PortBorderBrushColorKeepListStructure;
            }
            // Port has a default value, shows blue marker
-            else if (UsingDefaultValue && DefaultValueEnabled)
+            else if (UsingDefaultValue && DefaultValueEnabled || (isReturningDefaultValue && !isFunctionNode))


So this will show the port with a blue marker only after the node has executed? Is that the only requirement or is the requirement to also include showing the blue marker for default value ports on NodeModel nodes before they execute because the latter could be quite hard to determine.

Yes, after the node has been executed.
Both requirements are valid, right @Amoursol?

I'm not sure how we can achieve the second requirement for nodemodel nodes. @mjkkirschner do you know if we identify a default argument for a ZT node (before it executes) by looking at its signature?

I don't understand how you can prove anything about the default value from the return value of the output ports -

I think it's important to point out that the python node executes because it doesn't NEED its inputs to be satisfied to execute, not because it has default values, it doesn't have default values for its ports, nor do many other explicit NodeModel nodes, like ColorRange etc. (some do of course, but these are two separate things)

NodeModel nodes are compiled to some arbitrary designscript code, and that code runs, unless argument checking is part of that code, it's going to execute.

I'm not yet getting how default values factor into the requirements.

just saw your comment above @filipeotero regarding getting rid of isReturningDefaultValue - sounds good to me.

I think, more strictly speaking, @filipeotero is trying to determine if a node's input ports are "satisfied" (not necessarily default) by inspecting the node output after it has run, and if it returns a non-Function value, he's assuming all ports to be satisfied, whereas if it returns a Function, he's marking them as unsatisfied (if not connected). @filipeotero can you confirm if I'm understanding this correctly.

yes @aparajit-pratap, you are right.

Correct @filipeotero @aparajit-pratap - it's about port satisfaction. This simply means the node will run as intended. On non NodeModel Nodes, this happens when you wire in or unwire a connector. Doesn't have to wait for node execution.

aparajit-pratap · 2022-04-28T03:18:11Z

src/DynamoCoreWpf/ViewModels/Core/InPortViewModel.cs

-            //Is in function state
-            if (node.NodeModel.IsPartiallyApplied)
+            //Boolean that defines if the node has a default value from the cache value
+            isReturningDefaultValue = !string.IsNullOrEmpty(node.NodeModel.CachedValue?.StringData);


I don't understand this either. How can you say if a node has default value(s) assigned to one or more input ports if you examine its output (cachedValue)?

aparajit-pratap · 2022-04-28T18:12:40Z

src/Engine/ProtoCore/Reflection/MirrorData.cs

+            {
+                get
+                {
+                    return svData.IsPointer && Class.Name.Equals("Function") && Class.LibraryMirror.Name.Equals("FunctionObject.ds");


Do we have constants already defined for "Function" and "FunctionObject.ds"? Can you search for these in the codebase? If not, I think we should create them in a common utility class and make them accessible in the ProtoCore project at least.

I didn't find any constants defined for that. Do you have a class in mind for that?

ProtoCore.Utils.CoreUtils maybe

Ok, done. Thanks!

aparajit-pratap · 2022-04-28T18:22:57Z

src/DynamoCoreWpf/ViewModels/Core/InPortViewModel.cs

-            if (node.NodeModel.IsPartiallyApplied)
+            //This variable checks if the node is a function class
+            isFunctionNode = node.NodeModel.CachedValue != null &&
+                node.NodeModel.CachedValue.IsFunction;


Shouldn't this be:

isFunctionNode = node.NodeModel.IsPartiallyApplied || (node.NodeModel.CachedValue != null && node.NodeModel.CachedValue.IsFunction);

node.NodeModel.IsPartiallyApplied will not make sense for now because there are cases where the node doesn't return a function ever, like the python node.

hmm, I actually think it should be

isFunctionNode = node.NodeModel.IsPartiallyApplied && (node.NodeModel.CachedValue != null && node.NodeModel.CachedValue.IsFunction);

We could probably write a node which returns another function from all valid inputs - and it would now turn ports blue, even if it was not partially unsatisfied.

ok, updating the code...

@filipeotero the check if (node.NodeModel.IsPartiallyApplied) was there earlier, so by removing this check, won't you break behavior? I think this check is necessary for those nodes for which you can determine their "unsatisfied" state even before execution.

I mean a case like this that was working earlier in manual mode will fail won't it:

src/Engine/ProtoCore/Utils/CoreUtils.cs

QilongTang · 2022-05-03T17:49:47Z

Triggered tests running at: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/5912/

QilongTang · 2022-05-03T20:05:26Z

No new regressions were introduced in this PR. Merging.

Checking if it's a python node

80e4d53

QilongTang requested a review from mjkkirschner April 25, 2022 20:24

QilongTang added this to the 2.15.0 milestone Apr 25, 2022

checking the results of the node instead of IsPartiallyApplied

e1c55d2

mjkkirschner reviewed Apr 27, 2022

View reviewed changes

src/DynamoCoreWpf/ViewModels/Core/OutPortViewModel.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 28, 2022

View reviewed changes

Creating a flag that checks is a function node

e43a0e7

aparajit-pratap reviewed Apr 28, 2022

View reviewed changes

Adding IsPartiallyApplied in the logic and const string in the CoreUtils

2aebd73

aparajit-pratap reviewed Apr 28, 2022

View reviewed changes

src/Engine/ProtoCore/Utils/CoreUtils.cs Outdated Show resolved Hide resolved

Renaming constants

bc92e6f

filipeotero changed the title ~~Checking if it's a python node for input/output colors~~ Updating the colors of the ports according to the Function State Apr 29, 2022

QilongTang approved these changes May 3, 2022

View reviewed changes

QilongTang merged commit c64c66b into DynamoDS:master May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating the colors of the ports according to the Function State #12826

Updating the colors of the ports according to the Function State #12826

filipeotero commented Apr 25, 2022 •

edited

Loading

mjkkirschner commented Apr 25, 2022

filipeotero commented Apr 25, 2022

Amoursol commented Apr 26, 2022

filipeotero commented Apr 27, 2022

mjkkirschner Apr 27, 2022 •

edited

Loading

aparajit-pratap Apr 28, 2022

filipeotero Apr 28, 2022

aparajit-pratap Apr 28, 2022

mjkkirschner Apr 27, 2022

aparajit-pratap Apr 28, 2022

filipeotero Apr 28, 2022

aparajit-pratap Apr 28, 2022

mjkkirschner Apr 28, 2022 •

edited

Loading

mjkkirschner Apr 28, 2022

aparajit-pratap Apr 28, 2022 •

edited

Loading

filipeotero Apr 28, 2022

Amoursol Apr 29, 2022

aparajit-pratap Apr 28, 2022

aparajit-pratap Apr 28, 2022

filipeotero Apr 28, 2022

aparajit-pratap Apr 28, 2022

filipeotero Apr 28, 2022

aparajit-pratap Apr 28, 2022

filipeotero Apr 28, 2022

mjkkirschner Apr 28, 2022 •

edited

Loading

filipeotero Apr 28, 2022

aparajit-pratap Apr 28, 2022 •

edited

Loading

aparajit-pratap Apr 28, 2022

QilongTang commented May 3, 2022

QilongTang commented May 3, 2022

Updating the colors of the ports according to the Function State #12826

Updating the colors of the ports according to the Function State #12826

Conversation

filipeotero commented Apr 25, 2022 • edited Loading

Purpose

Declarations

Reviewers

FYIs

mjkkirschner commented Apr 25, 2022

filipeotero commented Apr 25, 2022

Amoursol commented Apr 26, 2022

filipeotero commented Apr 27, 2022

mjkkirschner Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang commented May 3, 2022

QilongTang commented May 3, 2022

filipeotero commented Apr 25, 2022 •

edited

Loading

mjkkirschner Apr 27, 2022 •

edited

Loading

mjkkirschner Apr 28, 2022 •

edited

Loading

aparajit-pratap Apr 28, 2022 •

edited

Loading

mjkkirschner Apr 28, 2022 •

edited

Loading

aparajit-pratap Apr 28, 2022 •

edited

Loading