DYN-5369: variable number of input ports for Watch3D nodes #13896

LongNguyenP · 2023-04-12T16:21:34Z

Purpose

This PR implements DYN-5369 (Allowing the Watch3D node to have variable number of input ports)

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

Release Notes

This PR makes the Watch3D node into a VariableInputNode. This allows the user to avoid using the List.Create.

Reviewers

FYIs

LongNguyenP · 2023-04-12T16:24:17Z

@mjkkirschner
After some thoughts, I think the API breaking change problem does not exist. It is true that Watch3d now inherits from VariableInputNode and needs to implement the two abstract method (and it already does), but this will not cause any problem for any existing third-party nodes that inherit from Watch3d because the two abstract methods have already been implemented in the Watch3d. These third-party nodes should just work as they are. Would be great if you can confirm whether I am correct here.

mjkkirschner · 2023-04-13T01:54:54Z

Hi @LongNguyenP,

I tested your question - by creating a custom watch3d node inheriting from Watch3D referencing 2.18 binaries, and then loaded it into dynamo built against your branch.

It did load correctly, and the UI was updated to include the new buttons.

the docs do mention your question as well:

Introducing a new base class

So long as it does not introduce any new abstract members or change the semantics or behavior of existing members, a type can be introduced into a hierarchy between two existing types. For example, between .NET Framework 1.1 and .NET Framework 2.0, we introduced DbConnection as a new base class for SqlConnection which previously derived from Component.

and indicate that it IS an API break... but given my test, I am not sure under what circumstances, maybe only if the type does not override these methods, I am not sure.

@pinzart90 any thoughts on this? I am inclined to overlook it because I doubt watch3d is inherited frequently.

src/DynamoCoreWpf/Controls/DynamoNodeButton.cs

mjkkirschner

1.A few questions and
2. I think this needs tests for the new methods / interactions.

src/DynamoCoreWpf/Controls/DynamoNodeButton.cs

mjkkirschner · 2023-04-13T01:59:58Z

src/Libraries/Watch3DNodeModels/Watch3D.cs

@@ -171,34 +171,10 @@ public void SetWatchSize(double w, double h)

        public override IEnumerable<AssociativeNode> BuildOutputAst(List<AssociativeNode> inputAstNodes)
        {
-            if (IsPartiallyApplied)


interesting, this seems like it could break some graphs, what did watch3d do when partially applied? Did it actually work?

I am not sure what IsPartiallyApplied mean? (I am still quite ignorant of many of Dynamo's inner working details) Does this mean the node will output a Function when the input is a Function? If so, this behavior is still maintined in this PR

kind of, partially applied is a functional programming term - it means that a function has been created with some input parameters set and others still free.

Can you please confirm what happened before and after your changes if you actually invoke the function on some input, you can use map node - I don't think this is so important but honestly I have no idea what the behavior was, so let's see...

My feeling is that it used to just return the input object, lets see if it still does that?

your screenshot is not what I mean -
we want the output of the Watch3D node to be a function - or rather, I am trying to understand if that used to work and now it is broken - which I am guessing is the case because of the code you have removed.

@mjkkirschner You are right, in the current master branch the Watch3D node will output a function (rather than "null) if the only-input port is free.

So looks like this PR will break any existing graph that relies on this function-output behavior, even thought I doubt if anyone would use Watch3D in such a way.

src/Libraries/Watch3DNodeModels/Watch3D.cs

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

mjkkirschner · 2023-04-13T02:02:59Z

src/Libraries/Watch3DNodeModelsWpf/Watch3DNodeViewCustomization.cs

+                new DynamoNodeButton(nodeView.ViewModel.NodeModel, "AddInPort")
+                {
+                    Content = "+",
+                    Style = (Style)SharedDictionaryManager.DynamoModernDictionary["AddRemoveButton"]


is it possible to use nameof or some other method to have this fail at compile time if the key is changed?

I am not sure if nameof is possible here. This part of the code is based on the NodeViewCustomization of the VariableInputNode and there the name "AddInPort" etc.. were actually hardcoded

could you pull all of these out into consts?

mjkkirschner · 2023-04-14T18:17:48Z

looks like a bunch of failing tests in last build.

LongNguyenP · 2023-04-15T13:05:25Z

Yep it seems that in this PR, Dyn files that contain Watch3D node saved in previous version did not open correctly. Looks like the deseiralization method of the VariableInputNode is not compatible with existing Watch3d implementation Investigating now. L

pinzart90 · 2023-04-17T13:32:57Z

Hi @LongNguyenP,

I tested your question - by creating a custom watch3d node inheriting from Watch3D referencing 2.18 binaries, and then loaded it into dynamo built against your branch.

It did load correctly, and the UI was updated to include the new buttons.

the docs do mention your question as well:
Introducing a new base class

So long as it does not introduce any new abstract members or change the semantics or behavior of existing members, a type can be introduced into a hierarchy between two existing types. For example, between .NET Framework 1.1 and .NET Framework 2.0, we introduced DbConnection as a new base class for SqlConnection which previously derived from Component.
and indicate that it IS an API break... but given my test, I am not sure under what circumstances, maybe only if the type does not override these methods, I am not sure.

@pinzart90 any thoughts on this? I am inclined to overlook it because I doubt watch3d is inherited frequently.

I do not think there will be any binary compatibility breaking changes or source code breaking changes as long as the Watch3D class implements all the abstract methods of the new intermediary base class VariableInputNode
There might be Behavioral breaking changes if the new Intermediary base class changes the overrides of the NodeModel virtual methods in unexpected ways

LongNguyenP · 2023-04-18T02:31:09Z

Hi @LongNguyenP,
I tested your question - by creating a custom watch3d node inheriting from Watch3D referencing 2.18 binaries, and then loaded it into dynamo built against your branch.
It did load correctly, and the UI was updated to include the new buttons.
the docs do mention your question as well:
Introducing a new base class

So long as it does not introduce any new abstract members or change the semantics or behavior of existing members, a type can be introduced into a hierarchy between two existing types. For example, between .NET Framework 1.1 and .NET Framework 2.0, we introduced DbConnection as a new base class for SqlConnection which previously derived from Component.
and indicate that it IS an API break... but given my test, I am not sure under what circumstances, maybe only if the type does not override these methods, I am not sure.
@pinzart90 any thoughts on this? I am inclined to overlook it because I doubt watch3d is inherited frequently.
I do not think there will be any binary compatibility breaking changes or source code breaking changes as long as the Watch3D class implements all the abstract methods of the new intermediary base class VariableInputNode There might be Behavioral breaking changes if the new Intermediary base class changes the overrides of the NodeModel virtual methods in unexpected ways

Great. Thanks for confirming

mjkkirschner · 2023-04-18T02:46:48Z

@LongNguyenP please don't update these xml files unless you have a good reason - the fact that you need to illustrates that legacy user graphs that were saved in .xml will potentially be broken. Have you been able to find out why the failures are occurring with xml deserialization.

We still need to support xml deserialization!

LongNguyenP · 2023-04-18T06:50:27Z

We still need to support xml deserialization!

Understood. I am digging deeper in the issue with XML now.en
BTW. Do you remember at which point Dynamo swtiched from XML to JSON dyn files?

…ully" This reverts commit 18a4cdf.

LongNguyenP · 2023-04-18T11:40:19Z

Ok I think I have got it. It was indeed because the XML deseiralization of the VariableInputNode (which Watch3D now inherits) was expecting a an XML attribute called "inputcount", which did not exist in the existing Watch3D's XML serialized data.

I have addressed this and the tests now pass on my local machine. Let's see they will pass on Jenkins too.

LongNguyenP · 2023-04-18T12:39:39Z

Looks like all tests now pass on Jenkins!

Except CoreUserInterfaceTests.WorkspaceContextMenu_TestIfOpenOnRightClick. However, this test passes on my machine and does not seem to have anything to do with Watch3d. maybe just due to Jenkins . Investigating...

LongNguyenP · 2023-04-18T23:59:59Z

All tests have passed now !

mjkkirschner · 2023-04-21T16:08:41Z

We still need to support xml deserialization!

Understood. I am digging deeper in the issue with XML now.en BTW. Do you remember at which point Dynamo swtiched from XML to JSON dyn files?

this happened when we moved from dynamo 1.x to 2.x

mjkkirschner · 2023-04-21T16:13:04Z

I also notice the watch3d node looks a bit different -FYI @Jingyi-Wen for review.

mjkkirschner · 2023-04-21T16:13:40Z

Looks like all tests now pass on Jenkins!

Except CoreUserInterfaceTests.WorkspaceContextMenu_TestIfOpenOnRightClick. However, this test passes on my machine and does not seem to have anything to do with Watch3d. maybe just due to Jenkins . Investigating...

yeah this test is just flaky, don't worry about that one, I think it's marked failure now.

LongNguyenP · 2023-04-25T01:11:37Z

I also notice the watch3d node looks a bit different -FYI @Jingyi-Wen for review.

We made some change to the UI/UX design. We wanted to use per-port context menu for add and removing ports (instead of using the plus or minus button). This way the user can insert a new node in-between existing node and remove a node in the middle if needed. But this require significantly more work so @saintentropy and I thought we should just keep using the plus and minus buttons for now and make another task for the per-port context menu later (I believe this won't break any existing graph that uses plus and minus buttons)

mjkkirschner · 2023-04-25T23:34:57Z

I also notice the watch3d node looks a bit different -FYI @Jingyi-Wen for review.

We made some change to the UI/UX design. We wanted to use per-port context menu for add and removing ports (instead of using the plus or minus button). This way the user can insert a new node in-between existing node and remove a node in the middle if needed. But this require significantly more work so @saintentropy and I thought we should just keep using the plus and minus buttons for now and make another task for the per-port context menu later (I believe this won't break any existing graph that uses plus and minus buttons)

thanks @LongNguyenP - last thing can you please see my clarifying comments on the partial function application (function passing) and check the behavior before and after.

aparajit-pratap · 2023-04-26T15:51:49Z

@LongNguyenP please give an appropriately descriptive title to your Dynamo PRs, other than just the JIRA ticket number. It helps reviewers both internally and externally what the overall intention of the change is.

Watch3D now accepts multiple input ports

c4d8a52

LongNguyenP requested review from mjkkirschner and saintentropy April 12, 2023 16:21

LongNguyenP changed the title ~~DYN-5369~~ DYN-5369-2 Apr 12, 2023

mjkkirschner reviewed Apr 13, 2023

View reviewed changes

src/DynamoCoreWpf/Controls/DynamoNodeButton.cs Show resolved Hide resolved

mjkkirschner requested changes Apr 13, 2023

View reviewed changes

A few fixes for PR review

5465a03

LongNguyenP added 2 commits April 15, 2023 20:08

ShowWarningOnRemovingPort is "true" by default

09bf061

Added a new test for multiple input ports of Watch3D node

eda9ddd

Update some test dyn files to JSON so tests can pass successfully

18a4cdf

LongNguyenP added 2 commits April 18, 2023 15:55

Revert "Update some test dyn files to JSON so tests can pass successf…

1f081cb

…ully" This reverts commit 18a4cdf.

Fix deserialization issue of old Watch3d nodes saved in XML dyn files

f5c76f3

Change string to resource value

e90a6ba

mjkkirschner requested a review from Jingyi-Wen April 21, 2023 16:13

LongNguyenP changed the title ~~DYN-5369-2~~ DYN-5369: variable number of input ports for Watch3D nodes Apr 27, 2023

mjkkirschner approved these changes May 3, 2023

View reviewed changes

LongNguyenP merged commit 258cdaa into master May 4, 2023

LongNguyenP deleted the DYN-5369-2 branch May 4, 2023 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-5369: variable number of input ports for Watch3D nodes #13896

DYN-5369: variable number of input ports for Watch3D nodes #13896

LongNguyenP commented Apr 12, 2023 •

edited

Loading

LongNguyenP commented Apr 12, 2023

mjkkirschner commented Apr 13, 2023

mjkkirschner left a comment

mjkkirschner Apr 13, 2023

LongNguyenP Apr 18, 2023

mjkkirschner Apr 21, 2023

mjkkirschner Apr 21, 2023

LongNguyenP Apr 26, 2023

mjkkirschner Apr 13, 2023

LongNguyenP Apr 13, 2023

mjkkirschner Apr 14, 2023

mjkkirschner commented Apr 14, 2023

LongNguyenP commented Apr 15, 2023

pinzart90 commented Apr 17, 2023 •

edited

Loading

LongNguyenP commented Apr 18, 2023

mjkkirschner commented Apr 18, 2023

LongNguyenP commented Apr 18, 2023

LongNguyenP commented Apr 18, 2023

LongNguyenP commented Apr 18, 2023 •

edited

Loading

LongNguyenP commented Apr 18, 2023

mjkkirschner commented Apr 21, 2023

mjkkirschner commented Apr 21, 2023

mjkkirschner commented Apr 21, 2023

LongNguyenP commented Apr 25, 2023

mjkkirschner commented Apr 25, 2023

aparajit-pratap commented Apr 26, 2023

DYN-5369: variable number of input ports for Watch3D nodes #13896

DYN-5369: variable number of input ports for Watch3D nodes #13896

Conversation

LongNguyenP commented Apr 12, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

LongNguyenP commented Apr 12, 2023

mjkkirschner commented Apr 13, 2023

mjkkirschner left a 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 commented Apr 14, 2023

LongNguyenP commented Apr 15, 2023

pinzart90 commented Apr 17, 2023 • edited Loading

LongNguyenP commented Apr 18, 2023

mjkkirschner commented Apr 18, 2023

LongNguyenP commented Apr 18, 2023

LongNguyenP commented Apr 18, 2023

LongNguyenP commented Apr 18, 2023 • edited Loading

LongNguyenP commented Apr 18, 2023

mjkkirschner commented Apr 21, 2023

mjkkirschner commented Apr 21, 2023

mjkkirschner commented Apr 21, 2023

LongNguyenP commented Apr 25, 2023

mjkkirschner commented Apr 25, 2023

aparajit-pratap commented Apr 26, 2023

LongNguyenP commented Apr 12, 2023 •

edited

Loading

pinzart90 commented Apr 17, 2023 •

edited

Loading

LongNguyenP commented Apr 18, 2023 •

edited

Loading