QNTM-3841 Add geometry creation option to Dynamo CLI by sm6srw · Pull Request #8775 · DynamoDS/Dynamo

sm6srw · 2018-04-19T17:57:01Z

Purpose

This PR:

Add a new command line tool, DynamoWPFCLI, that uses DynamoViewModel.
Add a new command line option, /G, to Dynamo WPF CLI that generates a file in JSON format with the geometry.
Updates the list of libraries that is pre-loaded for the CLI. We now pre-load the same libraries in the CLI as we do in the Sandbox. I can see no reason why the CLI should behave differently compared to Sandbox. It is very confusing that a graph that works in sandbox doesn't work in the CLI (and there is no feedback at all from the CLI when it happens) and the performance impact from adding a couple of more libraries is minimal.
The geometry format chosen is right now work in progress and will most likely change in the next couple of sprints.
Unit test added.

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
N/A User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
N/A Snapshot of UI changes, if any.
N/A Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @QilongTang

FYIs

@saintentropy @Racel

…ivate.

QilongTang · 2018-04-19T19:41:47Z

src/DynamoApplications/PathResolvers.cs

                "ProtoGeometry.dll",
                "DesignScriptBuiltin.dll",
                "DSCoreNodes.dll",
+                "DSOffice.dll",


If we are sure about the performance impact, I see no harm adding these dlls and looks like the comment at line:92 needs to be removed 😉

this was done initially for optimization workflows that benefited from the slightly faster startup time of ignoring these libraries. It's worth profiling it I think before making this change unless they are simply required for your work @sm6srw.

mjkkirschner · 2018-04-19T19:54:47Z

src/DynamoApplications/StartupUtils.cs

                "File will have the .json extension and be located in the same directory as the original file.", x => convertFile = x != null)
-                .Add("h|H|help", "Get some help", h => showHelp = h != null);
+                .Add("h|H|help", "Get some help", h => showHelp = h != null)
+                .Add("g=|G=|geometry", "Geometry, Instruct Dynamo to output geometry from all evaluations to a json file at this path", g => geometryFilePath = g);


could you update the dynamo CLI wiki page with an example of this format?

Sure, I will make sure the wiki gets updated.

mjkkirschner · 2018-04-19T19:56:11Z

src/DynamoCLI/CommandLineRunner.cs

    /// the results from invoking dynamo from the command line are useable.
    /// </summary>
-    public class CommandLineRunner
+    public partial class CommandLineRunner


is this necessary?

mjkkirschner · 2018-04-19T19:56:53Z

src/DynamoCLI/CommandLineRunner.cs

+
+                if (!String.IsNullOrEmpty(cmdLineArgs.GeometryFilePath))
+                {
+                    IRenderPackageFactory renderPackageFactory = new DefaultRenderPackageFactory();


should the geometry option take some settings for render package tessellation?

mjkkirschner · 2018-04-19T19:57:15Z

src/DynamoCLI/CommandLineRunner.cs

+                    var jsonFilename = cmdLineArgs.GeometryFilePath;
+                    using (StreamWriter jsonFile = new StreamWriter(jsonFilename))
+                    {
+                        List<Object> geometry = new List<object>();


mjkkirschner · 2018-04-19T19:57:20Z

src/DynamoCLI/CommandLineRunner.cs

+                if (!String.IsNullOrEmpty(cmdLineArgs.GeometryFilePath))
+                {
+                    IRenderPackageFactory renderPackageFactory = new DefaultRenderPackageFactory();
+                    List<GeometryHolder> nodeGeometries = new List<GeometryHolder>();


QilongTang · 2018-04-19T20:01:25Z

src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

+                                matchingNode.IsSetAsInput = updateIsSetAsInput;
+                                matchingNode.IsSetAsOutput = updateIsSetAsOutput;
+                                matchingNode.IsFrozen = updateIsFrozen;
+                                matchingNode.UpdateValue(new UpdateValueParams("IsVisible", updateIsVisible.ToString()));


I understand the Frozen aspect of CLI usage but not about IsVisible, can you expand a bit here?

I think we need to have a discussion around this... I feel this muddies our API separation and while useful could also be solved by CLI creating a viewModel.

mjkkirschner · 2018-04-19T20:01:31Z

src/DynamoCLI/GeometryHolder.cs

+                        }
+                    }
+
+                    groupData.Add("name", nodeModel.GUID.ToString());


hmm, couldn't this dataStructure just be a function that takes a renderPackage and serializes it?

Is the count > 0 due to custom nodes?

@saintentropy Not really, it is more about making sure that we are 'Done' and 'HasGeometry' is false if it for whatever reason happens.

QilongTang · 2018-04-19T20:03:15Z

test/Libraries/CommandLineTests/CommandLineTests.cs

+            //            var output = new XmlDocument();
+            //            output.Load(newpath);
+            //            AssertOutputValuesForGuid("36c30251-c867-4d73-9a3b-24f3e9ab00e5", new List<Tuple<int, string>> { Tuple.Create(0, "{e : 6, d : {4, 5}, a : 1, c : {bar : 999, foo : 99}, b : 2}") }, output);
+            //            AssertOutputValuesForGuid("6a5bcff0-ce40-4773-aee6-88d99104b4a7", new List<Tuple<int, string>> { Tuple.Create(0, "{a,b,c,d,e}"), Tuple.Create(1, "{1,2,{bar : 999, foo : 99},{4,5},6}") }, output);


Debugging code?

sm6srw · 2018-04-30T12:49:50Z

@mjkkirschner Thanks, I always forget those. PTAL when you have a moment.

sm6srw · 2018-05-02T12:57:27Z

@mjkkirschner Friendly ping!

mjkkirschner · 2018-05-03T00:53:42Z

src/DynamoWPFCLI/CommandLineRunnerWPF.cs

+                    }
+                }
+                string json = JsonConvert.SerializeObject(geometry);
+                jsonFile.Write(json);


what if this fails, since it's a CLI utility I guess a crash is not that bad.

It will be catched and logged in Programs.cs. I think that is good enough for a CLI.

mjkkirschner · 2018-05-03T00:54:30Z

src/DynamoWPFCLI/CommandLineRunnerWPF.cs

+            File.WriteAllText(newFilePath, json);
+        }
+
+        public new void Run(StartupUtils.CommandLineArguments args)


can you add a summary to this public method.

mjkkirschner · 2018-05-03T00:55:42Z

src/DynamoWPFCLI/DynamoWPFCLI.csproj

+  <ItemGroup>
+    <Reference Include="Microsoft.Practices.Prism, Version=4.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />
+    <Reference Include="Newtonsoft.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">
+      <HintPath>..\packages\Newtonsoft.Json.8.0.3\lib\net45\Newtonsoft.Json.dll</HintPath>


does this version of newtonsoft.JSON match the rest of the repo - I think we might be on 8.02 - @alfarok did you plan to update core to 8.03?

As far as I can tell, all projects are already on 8.03.

@mjkkirschner I think @sm6srw is correct, I see 8.03 in Core. We were discussing updating NUnit to a newer version but I don't think there were plans to update json.net

mjkkirschner · 2018-05-03T01:00:12Z

src/DynamoWPFCLI/GeometryHolder.cs

+            }
+        }
+
+        public void Dispose()


do you ever call this?

I don't, thanks!

mjkkirschner · 2018-05-03T01:00:44Z

src/DynamoWPFCLI/GeometryHolder.cs

+        {
+            node = nodeModel;
+            nodeModel.RenderPackagesUpdated += NodeRenderPackagesUpdated;
+            if (!nodeModel.RequestVisualUpdateAsync(model.Scheduler, model.EngineController, factory, true))


what does this do? Can you add a comment?

mjkkirschner · 2018-05-03T01:01:52Z

src/DynamoWPFCLI/GeometryHolder.cs

+            node.RenderPackagesUpdated -= NodeRenderPackagesUpdated;
+        }
+
+        private void NodeRenderPackagesUpdated(NodeModel nodeModel, RenderPackageCache renderPackages)


I guess I'm a little confused why this function can't just be a method that takes a list of renderPackages and serializes them. Is it to use JSON.net directly to make serialization more automatic?

Well, this will change in the next sprint or two when we make our final decision of what format to use. The answer could be a direct serialization of the renderPackages or something else. I leave it "as is" for now because it happens to work with the testing tools I have.

mjkkirschner · 2018-05-03T01:02:17Z

src/DynamoWPFCLI/Program.cs

+                try
+                {
+                    DynamoModel.IsCrashing = true;
+                    Dynamo.Logging.Analytics.TrackException(e, true);


hmm, do we want to log crashes for the CLI?

We already do it in the other CLI. I just follow the same pattern. Do we want to change it?

nope, lets keep it then.

mjkkirschner · 2018-05-03T01:02:49Z

src/DynamoWPFCLI/packages.config

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="Newtonsoft.Json" version="8.0.3" targetFramework="net45" />


@alfarok lets synchronize here.

mjkkirschner · 2018-05-03T01:03:12Z

test/Libraries/CommandLineTests/CommandLineTests.csproj

  </PropertyGroup>
  <ItemGroup>
+    <Reference Include="Microsoft.Practices.Prism, Version=4.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" />
+    <Reference Include="Newtonsoft.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">


mjkkirschner · 2018-05-03T01:03:25Z

test/Libraries/CommandLineTests/packages.config

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="Newtonsoft.Json" version="8.0.3" targetFramework="net45" />


mjkkirschner · 2018-05-03T01:03:54Z

@sm6srw looks good to me, a few last questions though.

sm6srw · 2018-05-03T15:22:50Z

@mjkkirschner Please take a quick final look. Many thanks!

mjkkirschner · 2018-05-03T15:29:25Z

@sm6srw looks good to me.
@QilongTang is there any reason to hold on on bringing this in now while we prep the other branches or shouldn't make a difference?

QilongTang · 2018-05-03T15:32:02Z

@mjkkirschner I see no reason, master is already 2.1.0 from now on. Safe to merge

sm6srw · 2018-05-03T15:40:06Z

Thanks a lot!

* POC * Cleanup code. * Add command line option -g for generating geometry json file. * Remove filtering * Fix node state flags at load time. * Use UpdateValue for setting isVisible on node model. Revert set to private. * Remove filtering. * Clean up holder code. * Cleanup converter code. Use node model defaults as starting point. * Preload the same libraries for CLI as for Sandbox. * Add color support * Simplify json * Update file format. * Remove property expressions. * Restore comment. * Restore Cef Settings * Add unit test for CLI geometry option. * Remove comment. * Don't extend CommandLineRunner. * Use var when possible. * Remove commented out code. * Use DynamoViewModel in CLI. * Use DefaultWatch3DViewModel for CLI. * Add new project DynamoWPFCLI. * Let CommandLineRunnerWPF inherit from CommandLineRunner. * Fix review comments. * Fix unstable unit tests. * Remove Dispose method. * Add comments

* Add RegisterFroTrace attribute as a trigger in Json serialization of trace * PR comment * [ 2.0 Release Cut ] Bump Up Master Patch Version (#8647) * Bump Up Patch Version * Add version * Update comments and version baseline * Address Comments * CubicMillimeterToCubicMeter conversion * Revert "CubicMillimeter To CubicMeter conversion" * Fix the units bug * Remove thread sleep * missed the test * Fix static property compilation and crash (#8649) * fix crash, compilation of static property * code cleanup * add test * add test graph * QNTM-3646: Fix creation of unwanted input port for list index-assignment (#8648) * fix for additional input port * fixes for output ports * update tests * added test, code cleanup * address review comments * QNTM-3279: Fix for crash when trying to directly call properties on class name (#8661) * fix for compiling property method as function pointer * code cleanup * add tests * Cherry-pick #8663 (#8664) * Update failing tests (#8663) * update tests * Merge LibG binaries with Geometry.Approximate API (#8666) * merge LibG binaries with Geometry.Approximate() * Update license * scroll bar fix in incanvas search * QNTM-3641: Prevent ColorRange node from crashing with old style List syntax (#8670) (#8674) * fix crash with ColorRange1D node * add test * add test set unsaved changes false when initializing json custom node * Update changes from librarie.js to fix QNTM-3710 (#8678) After investigating the errors in the EngOps check it was thought that these changes could not have caused the current failures. Other PRs have also shown the same issues. Given all of that, this PR is being merged. * address comments * Update LibG binaries for ASM224 branch (#8681) * update LibG binaries for ASM224 branch * PR comment * QNTM-3841 Add geometry creation option to Dynamo CLI (#8775) * POC * Cleanup code. * Add command line option -g for generating geometry json file. * Remove filtering * Fix node state flags at load time. * Use UpdateValue for setting isVisible on node model. Revert set to private. * Remove filtering. * Clean up holder code. * Cleanup converter code. Use node model defaults as starting point. * Preload the same libraries for CLI as for Sandbox. * Add color support * Simplify json * Update file format. * Remove property expressions. * Restore comment. * Restore Cef Settings * Add unit test for CLI geometry option. * Remove comment. * Don't extend CommandLineRunner. * Use var when possible. * Remove commented out code. * Use DynamoViewModel in CLI. * Use DefaultWatch3DViewModel for CLI. * Add new project DynamoWPFCLI. * Let CommandLineRunnerWPF inherit from CommandLineRunner. * Fix review comments. * Fix unstable unit tests. * Remove Dispose method. * Add comments * Fix type check * Apply same fix to NodeInputData * Add serialization tests * Fix typo in Extansion. * QNTM-4278 DynamoWPFCLI should use CoGs geometry format (#8887) * POC * Cleanup code. * Add command line option -g for generating geometry json file. * Remove filtering * Fix node state flags at load time. * Use UpdateValue for setting isVisible on node model. Revert set to private. * Remove filtering. * Clean up holder code. * Cleanup converter code. Use node model defaults as starting point. * Preload the same libraries for CLI as for Sandbox. * Add color support * Simplify json * Update file format. * Remove property expressions. * Restore comment. * Restore Cef Settings * Add unit test for CLI geometry option. * Remove comment. * Don't extend CommandLineRunner. * Use var when possible. * Remove commented out code. * Use DynamoViewModel in CLI. * Use DefaultWatch3DViewModel for CLI. * Add new project DynamoWPFCLI. * Let CommandLineRunnerWPF inherit from CommandLineRunner. * Fix review comments. * Fix unstable unit tests. * Remove Dispose method. * Add comments * First pass * Formatting fixes. * Remove XML stuff. * Some final adjustments. * Add more comments. * Move comment. * Only store the geometry array. * Refactor. * Add interface * Make interface internal * Rename implmentation. * Allow internal access for CLI * Move default implementation to core. * Make setters private * Address review comments. * match Revit CEF intitializtion settings * match Revit CEF intitializtion settings * match Revit CEF intitializtion settings * QNTM-5580 - Correct Typo in the Dynamo json serialization to match CoGS API (#9170) * Fix typo * Obsolete the existing property * Update test * remove expression body syntax * Add serialization test with typo * AVP NumberInputNode deserialization * add testing * QNTM-5568 Refinery needs to support culture invariance for number fmt, part Deux (#9178) * Don't culture format value strings. * Assume value strings are formated with `InvariantCulture` * Add tests * More work on tests * Clarify switching of cultures

sm6srw added 19 commits March 22, 2018 10:02

POC

595f4cc

Cleanup code.

ff387c9

Merge remote-tracking branch 'upstream/master' into QNTM-3670

911c72e

Add command line option -g for generating geometry json file.

b3c242f

Remove filtering

0394288

Fix node state flags at load time.

8b718fd

Use UpdateValue for setting isVisible on node model. Revert set to pr…

9098ffb

…ivate.

Remove filtering.

da09d56

Clean up holder code.

b186729

Cleanup converter code. Use node model defaults as starting point.

d694c3b

Preload the same libraries for CLI as for Sandbox.

4c9534b

Add color support

2ed4b0e

Simplify json

6cad914

Update file format.

de26758

Merge remote-tracking branch 'upstream/master' into QNTM-3841

23798ed

Remove property expressions.

d9945f7

Restore comment.

a2082bf

Restore Cef Settings

a4a6786

Add unit test for CLI geometry option.

9e73ada

sm6srw requested a review from mjkkirschner April 19, 2018 17:58

QilongTang reviewed Apr 19, 2018

View reviewed changes

mjkkirschner reviewed Apr 19, 2018

View reviewed changes

QilongTang reviewed Apr 19, 2018

View reviewed changes

mjkkirschner reviewed Apr 19, 2018

View reviewed changes

QilongTang reviewed Apr 19, 2018

View reviewed changes

Remove comment.

bc4a09d

Fix review comments.

fb51053

Fix unstable unit tests.

00f644c

sm6srw added the PTAL Please Take A Look 👀 label May 2, 2018

mjkkirschner reviewed May 3, 2018

View reviewed changes

sm6srw added 3 commits May 3, 2018 08:33

Merge remote-tracking branch 'upstream/master' into QNTM-3841

2536a47

Remove Dispose method.

d5fe2b1

Add comments

2dcb93a

mjkkirschner added LGTM and removed PTAL Please Take A Look 👀 labels May 3, 2018

sm6srw merged commit d4e2d0d into DynamoDS:master May 3, 2018

sm6srw deleted the QNTM-3841 branch May 3, 2018 17:21

saintentropy mentioned this pull request Oct 22, 2018

Cherry Picks for Refinery support in Dynamo 2.0.2 #9198

Merged

7 tasks

Conversation

sm6srw commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw commented Apr 30, 2018

Uh oh!

sm6srw commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented May 3, 2018

Uh oh!

sm6srw commented May 3, 2018

Uh oh!

sm6srw commented Apr 19, 2018 •

edited

Loading

sm6srw May 3, 2018 •

edited

Loading