-
Notifications
You must be signed in to change notification settings - Fork 635
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
Enable package node migration for Dynamo Core 2.0+ graphs #9306
Changes from 14 commits
17588a4
5704464
ce79a73
5f7cd98
0638108
6ce664e
14199b3
0deb09a
98a05b6
4f819e5
9cc5039
6a5d1f8
e17b80a
2007a35
672b698
bfa3302
e51d511
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,13 +36,14 @@ public class NodeReadConverter : JsonConverter | |
{ | ||
private CustomNodeManager manager; | ||
private LibraryServices libraryServices; | ||
private NodeFactory nodeFactory; | ||
private bool isTestMode; | ||
|
||
|
||
public ElementResolver ElementResolver { get; set; } | ||
//map of all loaded assemblies including LoadFrom context assemblies | ||
private Dictionary<string, List<Assembly>> loadedAssemblies; | ||
|
||
[Obsolete("This constructor will be removed in Dynamo 3.0, please use new NodeReadConverter constructor with additional parameters to support node migration.")] | ||
public NodeReadConverter(CustomNodeManager manager, LibraryServices libraryServices, bool isTestMode = false) | ||
{ | ||
this.manager = manager; | ||
|
@@ -56,6 +57,20 @@ public NodeReadConverter(CustomNodeManager manager, LibraryServices libraryServi | |
} | ||
} | ||
|
||
public NodeReadConverter(CustomNodeManager manager, LibraryServices libraryServices, NodeFactory nodeFactory, bool isTestMode = false) | ||
{ | ||
this.manager = manager; | ||
this.libraryServices = libraryServices; | ||
this.nodeFactory = nodeFactory; | ||
this.isTestMode = isTestMode; | ||
//we only do this in test mode because it should not be required- | ||
//see comment below in NodeReadConverter.ReadJson - and it could be slow. | ||
if (this.isTestMode) | ||
{ | ||
this.loadedAssemblies = this.buildMapOfLoadedAssemblies(); | ||
} | ||
} | ||
|
||
private Dictionary<string,List<Assembly>> buildMapOfLoadedAssemblies() | ||
{ | ||
var allAssemblies = AppDomain.CurrentDomain.GetAssemblies(); | ||
|
@@ -106,6 +121,18 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |
} | ||
} | ||
|
||
// Check for and attempt to resolve an unknown type before proceeding | ||
if (type == null) | ||
{ | ||
// Check to see if the missing type can be resolved (AlsoKnownAs) | ||
var unresolvedName = obj["$type"].Value<string>().Split(',').FirstOrDefault(); | ||
Type newType; | ||
nodeFactory.ResolveType(unresolvedName, out newType); | ||
|
||
if (newType != null) // If resolved update the type | ||
{ type = newType; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it is preferred to have a single curly brace on a line (don't know what happened to coding conventions once followed by the team):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh wow, if we would like to get this topic up, please fix all the comments in this PR so they are in the same format 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry comments without a space were copied from the constructor I made obsolete, I would never commit such a crime 😂 |
||
} | ||
|
||
// If the id is not a guid, makes a guid based on the id of the node | ||
var guid = GuidUtility.tryParseOrCreateGuid(obj["Id"].Value<string>()); | ||
|
||
|
@@ -118,10 +145,13 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |
string assemblyLocation = objectType.Assembly.Location; | ||
|
||
bool remapPorts = true; | ||
|
||
// If type is still null at this point return a dummy node | ||
if (type == null) | ||
{ | ||
node = CreateDummyNode(obj, assemblyLocation, inPorts, outPorts); | ||
} | ||
// Attempt to create a valid node using the type | ||
else if (type == typeof(Function)) | ||
{ | ||
var functionId = Guid.Parse(obj["FunctionSignature"].Value<string>()); | ||
|
@@ -169,7 +199,15 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |
else if (typeof(DSFunctionBase).IsAssignableFrom(type)) | ||
{ | ||
var mangledName = obj["FunctionSignature"].Value<string>(); | ||
var priorNames = libraryServices.GetPriorNames(); | ||
var functionDescriptor = libraryServices.GetFunctionDescriptor(mangledName); | ||
string newName; | ||
|
||
// Update the function descriptor if a newer migrated version of the node exists | ||
if (priorNames.TryGetValue(mangledName, out newName)) | ||
{ functionDescriptor = libraryServices.GetFunctionDescriptor(newName); } | ||
|
||
// Use the functionDescriptor to try and restore the proper node if possible | ||
if (functionDescriptor == null) | ||
{ | ||
node = CreateDummyNode(obj, assemblyLocation, inPorts, outPorts); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?xml version="1.0"?> | ||
<migrations> | ||
<priorNameHint> | ||
<oldName>Examples.BasicExample.Create@Autodesk.DesignScript.Geometry.Point</oldName> | ||
<newName>Examples.NEWBasicExample.Create@Autodesk.DesignScript.Geometry.Point</newName> | ||
</priorNameHint> | ||
<priorNameHint> | ||
<oldName>Examples.BasicExample.Create@double,double,double</oldName> | ||
<newName>Examples.NEWBasicExample.Create@double,double,double</newName> | ||
</priorNameHint> | ||
<priorNameHint> | ||
<oldName>Examples.BasicExample.MultiReturnExample</oldName> | ||
<newName>Examples.NEWBasicExample.MultiReturnExample</newName> | ||
</priorNameHint> | ||
</migrations> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,211 @@ | ||
{ | ||
"Uuid": "3c91f94f-9c42-4a29-bf1c-ecdf340a3490", | ||
"IsCustomNode": false, | ||
"Description": null, | ||
"Name": "LegacyDynamoSamples_MigrationTest", | ||
"ElementResolver": { | ||
"ResolutionMap": {} | ||
}, | ||
"Inputs": [], | ||
"Outputs": [], | ||
"Nodes": [ | ||
{ | ||
"ConcreteType": "Dynamo.Graph.Nodes.ZeroTouch.DSFunction, DynamoCore", | ||
"NodeType": "FunctionNode", | ||
"FunctionSignature": "Examples.BasicExample.Create@Autodesk.DesignScript.Geometry.Point", | ||
"Id": "da2c975424ea4e1bae46b5272758e3c5", | ||
"Inputs": [ | ||
{ | ||
"Id": "a48fcf2706044ea3903eaebfa0daa8c9", | ||
"Name": "point", | ||
"Description": "A point.\n\nPoint\nDefault value : Autodesk.DesignScript.Geometry.Point.ByCoordinates(5, 5, 5)", | ||
"UsingDefaultValue": true, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
} | ||
], | ||
"Outputs": [ | ||
{ | ||
"Id": "7dc917d968794e26a94d3e2748163633", | ||
"Name": "BasicExample", | ||
"Description": "A BasicExample object.", | ||
"UsingDefaultValue": false, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
} | ||
], | ||
"Replication": "Auto", | ||
"Description": "Another example of a static constructor which uses a parameter with a default value. The default value is provided as a design script expression.\n\nBasicExample.Create (point: Point = Autodesk.DesignScript.Geometry.Point.ByCoordinates(5, 5, 5)): BasicExample" | ||
}, | ||
{ | ||
"ConcreteType": "Dynamo.Graph.Nodes.ZeroTouch.DSFunction, DynamoCore", | ||
"NodeType": "FunctionNode", | ||
"FunctionSignature": "Examples.BasicExample.Create@double,double,double", | ||
"Id": "c61389b2f09c432ab8727cfd41282f9a", | ||
"Inputs": [ | ||
{ | ||
"Id": "6b8a7b3b7b9d4bc28369802688120364", | ||
"Name": "x", | ||
"Description": "The x coordinate of the point.\n\ndouble\nDefault value : 42", | ||
"UsingDefaultValue": true, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
}, | ||
{ | ||
"Id": "194e917177a24a27bccd80d4d61ad82e", | ||
"Name": "y", | ||
"Description": "The y coordinate of the point.\n\ndouble\nDefault value : 42", | ||
"UsingDefaultValue": true, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
}, | ||
{ | ||
"Id": "3271aff8d6dd4c80be31a0ba305919fc", | ||
"Name": "z", | ||
"Description": "The z coordinate of the point.\n\ndouble\nDefault value : 42", | ||
"UsingDefaultValue": true, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
} | ||
], | ||
"Outputs": [ | ||
{ | ||
"Id": "d1fb031df35d49b688f7d8ef6d160dde", | ||
"Name": "BasicExample", | ||
"Description": "A HelloDynamoZeroTouch object.", | ||
"UsingDefaultValue": false, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
} | ||
], | ||
"Replication": "Auto", | ||
"Description": "Dynamo uses the pattern of static constructors. Don't forget to fill in the xml comments so that you will get help tips in the UI. You can also use default parameters, as we have here. With default parameters defined, you will not be required to attach any inputs to these ports in Dynamo.\n\nBasicExample.Create (x: double = 42, y: double = 42, z: double = 42): BasicExample" | ||
}, | ||
{ | ||
"ConcreteType": "Dynamo.Graph.Nodes.ZeroTouch.DSFunction, DynamoCore", | ||
"NodeType": "FunctionNode", | ||
"FunctionSignature": "Examples.BasicExample.MultiReturnExample", | ||
"Id": "b932a2ef364945f39b7b1d06da2a0e97", | ||
"Inputs": [], | ||
"Outputs": [ | ||
{ | ||
"Id": "54a9e99405ad46c7acdd3a0f66115907", | ||
"Name": "thing 1", | ||
"Description": "var", | ||
"UsingDefaultValue": false, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
}, | ||
{ | ||
"Id": "f0efc366311e458c8dbf8808b0cf0549", | ||
"Name": "thing 2", | ||
"Description": "var", | ||
"UsingDefaultValue": false, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
} | ||
], | ||
"Replication": "Auto", | ||
"Description": "The MultiReturn attribute can be used to specify the names of multiple output ports on a node that returns a dictionary. The node must return a dictionary to be recognized as a multi-out node.\n\nBasicExample.MultiReturnExample ( ): var[]..[]" | ||
}, | ||
{ | ||
"ConcreteType": "SampleLibraryUI.Examples.DropDownExample, SampleLibraryUI", | ||
"SelectedIndex": 2, | ||
"NodeType": "ExtensionNode", | ||
"Id": "45152d03fc75491baa4a0f2f9565cd61", | ||
"Inputs": [], | ||
"Outputs": [ | ||
{ | ||
"Id": "da224d826dcc4469a3941bc070448b1c", | ||
"Name": "item", | ||
"Description": "The selected item", | ||
"UsingDefaultValue": false, | ||
"Level": 2, | ||
"UseLevels": false, | ||
"KeepListStructure": false | ||
} | ||
], | ||
"Replication": "Disabled", | ||
"Description": "An example drop down node." | ||
} | ||
], | ||
"Connectors": [], | ||
"Dependencies": [], | ||
"Bindings": [], | ||
"View": { | ||
"Dynamo": { | ||
"ScaleFactor": 1.0, | ||
"HasRunWithoutCrash": true, | ||
"IsVisibleInDynamoLibrary": true, | ||
"Version": "2.1.0.7116", | ||
"RunType": "Automatic", | ||
"RunPeriod": "1000" | ||
}, | ||
"Camera": { | ||
"Name": "Background Preview", | ||
"EyeX": -17.0, | ||
"EyeY": 24.0, | ||
"EyeZ": 50.0, | ||
"LookX": 12.0, | ||
"LookY": -13.0, | ||
"LookZ": -58.0, | ||
"UpX": 0.0, | ||
"UpY": 1.0, | ||
"UpZ": 0.0 | ||
}, | ||
"NodeViews": [ | ||
{ | ||
"ShowGeometry": true, | ||
"Name": "BasicExample.Create", | ||
"Id": "da2c975424ea4e1bae46b5272758e3c5", | ||
"IsSetAsInput": false, | ||
"IsSetAsOutput": false, | ||
"Excluded": false, | ||
"X": 264.0, | ||
"Y": 34.75 | ||
}, | ||
{ | ||
"ShowGeometry": true, | ||
"Name": "BasicExample.Create", | ||
"Id": "c61389b2f09c432ab8727cfd41282f9a", | ||
"IsSetAsInput": false, | ||
"IsSetAsOutput": false, | ||
"Excluded": false, | ||
"X": 275.0, | ||
"Y": 130.75 | ||
}, | ||
{ | ||
"ShowGeometry": true, | ||
"Name": "BasicExample.MultiReturnExample", | ||
"Id": "b932a2ef364945f39b7b1d06da2a0e97", | ||
"IsSetAsInput": false, | ||
"IsSetAsOutput": false, | ||
"Excluded": false, | ||
"X": 265.0, | ||
"Y": 282.75 | ||
}, | ||
{ | ||
"ShowGeometry": true, | ||
"Name": "Drop Down Example", | ||
"Id": "45152d03fc75491baa4a0f2f9565cd61", | ||
"IsSetAsInput": false, | ||
"IsSetAsOutput": false, | ||
"Excluded": false, | ||
"X": 318.0, | ||
"Y": 408.75 | ||
} | ||
], | ||
"Annotations": [], | ||
"X": 0.0, | ||
"Y": 0.0, | ||
"Zoom": 1.0 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but I think we avoid using
this
as per existing coding conventions (which may have been forgotten along the way). Also if you use Resharper you'll see that a lot of these things are highlighted by the tool.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the old code and this file in particular is filled with
this
. Removing it in certain areas creates ambiguity between constructor parameters and class properties, such as above