Improve Core.Data Serialization and Add Remember Node #14111

saintentropy · 2023-06-26T17:25:12Z

Purpose

The purpose of this PR is to improve the implementation of the Data.ParseJson and Data.StringifyJson to support json serialization of most Dynamo OOB data types. Today these Data nodes fail to serialize or later to deserialize most Objects vs Value types. Geometry for example always fails in StringifyJson. This PR will add support for the following Data found in Dynamo OOB.

ProtoGeometry types with the exception of Topology (as Topology does not make as much sence in isolation from the source)
- Primitive Types (BoundingBox, CoordinateSystem, Plane, Vector, Arc, Circle, Ellipse, EllipseArc, Line, NurbsCurve, PolyCurve, Polygon, Point, and NurbsSurface) are supposed via schema defined in Autodesk Schema Service
- Dynamo Geometry Types (Rectangle, Cuboid, Cone, Cylinder, Sphere, and Mesh) are supported via Dynamo specific schema.
- Complex Geometry Types (PolySurface, Helix, Curves, Surfaces, Solids, TSplines) are supported via SAB and TSM
Dynamo Core Types including Color, Location and Image (via png)

This PR also moves the implementation of the Remember node from the Generative Design package to Core. The implementation of the Remember Node is identical but the serialization format has been updated to use Data.ParseJson and Data.StringifyJson. The version in GD will be marked as deprecated and removed from the package in future releases. Due to the format change we are not migrating the node.

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
This PR contains no files larger than 50 MB

Release Notes

Data.ParseJson and Data.StringifyJson nodes have been improved to support serialization of most Dynamo standard data types. This includes all Geometry types except Topology and Color, Location and Image.

Data.Remember node has been moved from the GenerativeDesign package to Dynamo core library. The new node has improved accuracy in how it stores and restores data.

Reviewers

@sm6srw @QilongTang Please assign

FYIs

@mjkkirschner @twastvedt @LongNguyenP

mjkkirschner · 2023-06-27T18:30:35Z

@saintentropy this build is broken.

LongNguyenP · 2023-09-22T15:22:07Z

The build failed because Jenkins could not get the correct LibG nuget package with the json (de)serializaition method, which is actually not available yet.

aparajit-pratap · 2023-10-13T18:15:19Z

src/Libraries/CoreNodeModels/Remember.cs

+    [NodeName("Remember")]
+    [NodeDescription(nameof(Properties.Resources.RememberDescription), typeof(Properties.Resources))]
+    [NodeCategory("Core.Data")]
+    [InPortNames(">")]


Node is designed to look like a watch node with ">" port strings

src/Libraries/CoreNodeModels/Remember.cs

mjkkirschner · 2023-11-03T21:41:20Z

src/Libraries/CoreNodes/Data.cs

+#if NET6_0_OR_GREATER
+        [SupportedOSPlatform("windows")]
+#endif
+        private class ImageConverter : JsonConverter


perhaps this should be called PNGImageConverter?

mjkkirschner · 2023-11-10T17:10:52Z

src/Libraries/CoreNodeModels/Remember.cs

+    {
+        private string cache = "";
+
+        [JsonProperty("Cache")]


why do you need this when the property has the same name?

src/Libraries/CoreNodeModels/Remember.cs

mjkkirschner · 2023-11-10T17:11:37Z

src/Libraries/CoreNodeModels/Remember.cs

+        {
+            switch (e.PropertyName)
+            {
+                case "State":


mjkkirschner · 2023-11-10T17:15:30Z

src/Libraries/CoreNodes/Data.cs

+                }
+            }
+
+            if (jObject.ContainsKey("typeid"))


whats the difference between serializations that contain "typeid" and "$typeid" ?

In ASP json that us $typeid as reserved keyword for the type. In the changeset world they used typeid

mjkkirschner · 2023-11-10T17:17:39Z

src/Libraries/CoreNodes/Data.cs

+
+                    case "dynamo.data:location-1.0.0":
+                        return DynamoUnits.Location.ByLatitudeAndLongitude(
+                        (double)jObject["Latitude"],


curious - what are your thoughts about this vs adding protected or internal json constructors and just letting newtonsoft figure it out?

I'm fine with this for now, just wanted to evaluate the +/-s

I think that is better approach. I was waiting for the .NET7 so we could introduce an interface to support serialization registration similar to the IGraphicItem. (static abstract is now supported in interfaces NET7 and above). I can do this now without the interface for the internal types just to set the pattern.

ah I see, so we'll have to wait on the interface approach until after we move to .net 8... I guess you could do it now and change the language target of this csproj to 11?

What would the interface look like?

mjkkirschner · 2023-11-10T21:57:51Z

test/DynamoCoreTests/DSCoreDataTests.cs

+            // Verify objects match when serializing / de-serializing geometry type
+            AssertPreviewValue("abb39e07-db08-45cf-9438-478defffbf68", true);
+
+            // Verify current failure cases are all false


I don't understand this comment.

mjkkirschner · 2023-11-10T21:59:32Z

test/DynamoCoreTests/DSCoreDataTests.cs

+        }
+
+        [Test]
+        [Category("Failure")]


why does this fail? Can you add a TODO, task number etc?

mjkkirschner · 2023-11-10T21:59:41Z

test/DynamoCoreTests/DSCoreDataTests.cs

+        }
+
+        [Test]
+        [Category("Failure")]


why does this fail - etc

mjkkirschner · 2023-11-10T21:59:51Z

test/DynamoCoreTests/DSCoreDataTests.cs

+        }
+
+        [Test]
+        [Category("Failure")]


why does this fail

Failures are waiting for the upstream PR in LibG

mjkkirschner · 2023-11-10T22:00:59Z

test/DynamoCoreTests/DSCoreDataTests.cs

+        [Category("UnitTests")]
+        public void RoundTripForColorReturnsSameResult()
+        {
+            var color = DSCore.Color.ByARGB(25, 30, 35, 40);


Is there any benefit to the preceding tests being implemented using graphs instead of implementing them to how these tests are written without involving graph deserialization? I find them easier to debug. The graph ones in my opinion are opaque.

Yeah... Sorry I was just following the pattern of the other tests for Json. Also added benefit of much greater coverage. We can create a follow up task and pull all the tests into unit test style if it is critical

nah, not critical, maybe just preferred for future tests.

mjkkirschner · 2023-11-10T22:03:02Z

src/Libraries/CoreNodes/Data.cs

+        public static bool CanObjectBeCached(object inputObject)
+        {
+            if (inputObject == null
+                || (inputObject is string inputString && inputString == "null")


when do we have a "null" string?

what about an empty string?

Empty string is ok :-)

mjkkirschner · 2023-11-10T22:04:04Z

test/DynamoCoreTests/DSCoreDataTests.cs

+
+        [Test]
+        [Category("UnitTests")]
+        public void CanObjectBeCachedRejectsEmptyString()


this test seems to have the wrong name.

mjkkirschner · 2023-11-10T22:07:28Z

test/Tools/docGeneratorTestFiles/TestMdOutput_CoreNodeModels/CoreNodeModels.Remember.md

@@ -0,0 +1,4 @@
+## Remember - Documentation
+This documentation file is auto generated by NodeDocumentationMarkdownGenerator, Version=2.13.0.2212, Culture=neutral, PublicKeyToken=null.


this seems like a great node to supply better documentation for.
FYI @hwahlstrom

mjkkirschner · 2023-11-10T22:08:40Z

src/Libraries/CoreNodes/Properties/Resources.resx

@@ -141,6 +141,12 @@
  <data name="EnumDateOfWeekWednesday" xml:space="preserve">
    <value>Wednesday</value>
  </data>
+  <data name="Exception_Deserialize_Unsupported_Type" xml:space="preserve">
+    <value>The stored data can not be rebuilt.</value>


mjkkirschner · 2023-11-10T22:09:26Z

src/Libraries/CoreNodes/Data.cs

+            }
+            catch(Exception ex)
+            {
+                throw new NotSupportedException(string.Format(Properties.Resources.Exception_Serialize_Unsupported_Type, inputObject.GetType().FullName));


would it be helpful to also log the real exception to the console or concat it?

mjkkirschner · 2023-11-10T22:20:15Z

src/Libraries/CoreNodes/Data.cs

+            }
+        }
+
+        private class ColorConveter : JsonConverter


why use a bunch of converters for this instead of decorating the class with attributes. I guess I get a bit of a code smell when the serialization and deserialization are handled in different places, one using a converter and one using regular constructors. Can it be unified?

I think the main reason is the overlap with the node -> file serialization and the node object -> data serialization. I hope to introduce the interface so that it can be explicit.

Nevermind.... file stuff doesn't apply to ZT. But would be handled by the interface

mjkkirschner · 2023-11-10T22:25:34Z

src/Libraries/CoreNodeModels/Remember.cs

+            resultAst.Add(AstFactory.BuildAssignment(functionCallIndent, functionCall));
+
+            //Next add the first key value pair to the output port
+            var getFirstKey = AstFactory.BuildFunctionCall(BuiltinDictionaryTypeName, BuiltinDictionaryGet,


I think the ASTFactory somewhere has methods for building these calls easily.

Not that I am aware of or could find. The main thing we do here is that we need to pass the data from the inputPort and the cached string into and back out of the node every time. The dictionary is used so that we can only present to the ports data on the node and keep the cache data internal. I think we look long and hard at the time we wrote this and didn't have any out of the box options of dictionaries in ASTFactory.

look for:
DictionaryExpressionBuilder

https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/Parser/DictionaryBuilder.cs

Ahhh. Thanks. I did look at that. Interesting but not exactly what we need. The dictionary is already set up by the function we call Remember() as the return type. The AST construction we are doing is to look up specific keys later... More specifically DictionaryBuilder embeds the constuctor DesignScript.Builtin.Dictionary.ByKeysValues whereas we need the method DesignScript.Builtin.Dictionary.ValueAtKey. There could be a helper for us but not this one.

src/Libraries/CoreNodeModels/Remember.cs

# Conflicts: # src/Libraries/CoreNodeModels/CoreNodeModelsImages.resx # src/Libraries/CoreNodeModels/Properties/Resources.en-US.resx # src/Libraries/CoreNodeModels/Properties/Resources.resx # test/Tools/docGeneratorTestFiles/sampledictionarycontent/Dynamo_Nodes_Documentation.json

mjkkirschner

@saintentropy lets ship it.

saintentropy · 2023-11-13T22:02:58Z

Cool... Will merge when CICD confirms

saintentropy · 2023-11-14T04:45:17Z

OK Passed on https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1244/. Going to merge 😃

saintentropy added 7 commits June 7, 2023 12:44

Add Geometry support to the Data Parse / Stringify nodes

ea9afbe

Add converters for Color, Image, and Location

d3479fe

missing comma

0a12332

Add Remember Node

5179b47

Move serialization logic to LibG

ef5e9fc

fix order bug

05d5a89

Merge branch 'master' into DYN-5895

281ac3d

Fixed typo preventing Remember node from showing up in Dynamo library

22d624c

saintentropy added 9 commits September 25, 2023 11:50

Merge branch 'master' into DYN-5895

cdffedb

Merge branch 'master' into DYN-5895

128d133

Clean up

ae9ddbe

Temp remove ForgeUnits

3eede66

Merge branch 'master' into DYN-5895

4ba4038

Merge branch 'master' into DYN-5895

6fe96a1

Update tests

8e0ef8c

Add back in the Units support

1f43200

Fix tests

295af29

aparajit-pratap reviewed Oct 13, 2023

View reviewed changes

src/Libraries/CoreNodeModels/Remember.cs Outdated Show resolved Hide resolved

saintentropy added 9 commits October 14, 2023 01:32

Add icon

eafd245

Add new types for solids

233b4c8

Add initial tests

01454db

Update tests

6557cea

Add catch and resource strings

c7e5043

Add error check in test

9db3562

Fix typos

c8c3eb9

Add mesh support back to convertor

f092690

Add os guards for System.Drawing

d089075

mjkkirschner reviewed Nov 3, 2023

View reviewed changes

Merge branch 'master' into DYN-5895

76174bd

mjkkirschner reviewed Nov 10, 2023

View reviewed changes

src/Libraries/CoreNodeModels/Remember.cs Show resolved Hide resolved

mjkkirschner reviewed Nov 10, 2023

View reviewed changes

src/Libraries/CoreNodeModels/Remember.cs Outdated

{

switch (e.PropertyName)

{

case "State":

Copy link

Member

mjkkirschner Nov 10, 2023

Choose a reason for hiding this comment

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

nameof?

mjkkirschner reviewed Nov 10, 2023

View reviewed changes

test/DynamoCoreTests/DSCoreDataTests.cs

}

[Test]

[Category("Failure")]

Copy link

Member

mjkkirschner Nov 10, 2023

Choose a reason for hiding this comment

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

why does this fail? Can you add a TODO, task number etc?

mjkkirschner reviewed Nov 10, 2023

View reviewed changes

test/DynamoCoreTests/DSCoreDataTests.cs

}

[Test]

[Category("Failure")]

Copy link

Member

mjkkirschner Nov 10, 2023

Choose a reason for hiding this comment

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

why does this fail - etc

mjkkirschner reviewed Nov 10, 2023

View reviewed changes

src/Libraries/CoreNodeModels/Remember.cs Outdated Show resolved Hide resolved

saintentropy added 4 commits November 13, 2023 12:39

PR comments

72baac0

Update error handling and logging

2f787ee

Update bad merge conflict fix and update test

58f3a9f

mjkkirschner approved these changes Nov 13, 2023

View reviewed changes

saintentropy added 2 commits November 13, 2023 18:33

Merge branch 'master' into DYN-5895

43d4f3c

Merge branch 'master' into DYN-5895

cab7d39

saintentropy merged commit 8af516c into DynamoDS:master Nov 14, 2023
17 checks passed

		@@ -0,0 +1,4 @@
		## Remember - Documentation
		This documentation file is auto generated by NodeDocumentationMarkdownGenerator, Version=2.13.0.2212, Culture=neutral, PublicKeyToken=null.

Improve Core.Data Serialization and Add Remember Node #14111

Improve Core.Data Serialization and Add Remember Node #14111

Conversation

saintentropy commented Jun 26, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

mjkkirschner commented Jun 27, 2023

LongNguyenP commented Sep 22, 2023

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

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 Nov 10, 2023 • 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

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 left a comment

Choose a reason for hiding this comment

saintentropy commented Nov 13, 2023

saintentropy commented Nov 14, 2023

saintentropy commented Jun 26, 2023 •

edited

Loading

mjkkirschner Nov 10, 2023 •

edited

Loading