QNTM-4278 DynamoWPFCLI should use CoGs geometry format #8887

sm6srw · 2018-05-24T15:42:06Z

Purpose

This PR changes the graphics format used by the /g option in DynamoWPFCLI to use the same format as CoGs is using.

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
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.

Reviewers

@mjkkirschner

FYIs

@QilongTang @saintentropy

…ivate.

sm6srw · 2018-05-24T18:38:33Z

This is now ready for review. @mjkkirschner: When you have a moment. No rush...

mjkkirschner · 2018-05-24T18:42:35Z

src/DynamoWPFCLI/GeometryHolder.cs

+    /// The class that represents data for drawing a graphic primitive 
+    /// </summary>
+
+    internal class GraphicPrimitives


@sm6srw what is the difference between this and

Dynamo/src/NodeServices/GraphicsDataInterfaces.cs

Line 10 in dec6240

public interface IRenderPackage

It is a version of the data in the IRenderPackage but the types and property names are different. I borrowed this class from Reach and then updated it to follow the naming Convention used by CoGs for easy serialization to JSON. Do you think it is worth doing this differently? Add a ToJson method to iRenderPackage?

Not sure if it would work, but you could inherit from renderPackage and then use JsonProperty attributes to serialize into the format.

Or could add a new interface backing this object which is the same as the format.

I think keeping it internal for now is fine but we would want to expose creation methods for it eventually, and not just through the CLI but as part of some kind of export from dynamoCore or as part of DynamoServices I think using the render package for this keeps it simpler, but not sure if that works for you or if you think it's better to have some indirection between the output and render packages. Just curious on your approach.

Thanks, I will take a look at both options.

sm6srw · 2018-05-30T14:57:56Z

@mjkkirschner This is a starting point that I think is in the right direction. Everything is internal for now and should not break the API as I understands it. I think the create function in the end should be a get method on the IRenderPackage interface but it could also live on a utility interface (if we have one suitable). The latter is more flexible I think as it doesn't ties it to the IRenderPackage implementation. Take a look when you have a moment. No rush...

mjkkirschner · 2018-06-06T15:33:44Z

src/DynamoCore/DynamoCore.csproj

      <HintPath>..\packages\Unofficial.Ionic.Zip.1.9.1.8\lib\Ionic.Zip.dll</HintPath>
      <Private>True</Private>
    </Reference>
+    <Reference Include="Microsoft.CSharp" />


any memory of why this is required?

Needed when 'dynamic' is used.

@sm6srw hmm, can you verify this does not break the mono build?

I think the Mono build is fine but I will check again.

mjkkirschner · 2018-06-06T15:34:52Z

src/DynamoCore/Visualization/DefaultGraphicPrimitives.cs

+                if (typeof(T) == typeof(double))
+                {
+                    foreach (T value in coordinates)
+                        writer.Write(Convert.ToSingle(value));


whats this about? CoGS format doesn't support doubles?

The 'Cogs' format is storing doubles as floats as I read it. I think that is fine and not something that can be changed without breaking the format, right? This method is borrowed from reach.

CoGS is fine with double precision.

float in Swagger just means floating point, doesn't imply single precision.

This is for the arrays that are encoded as base64 strings. So they are strings in swagger. Is Cogs encoding these as double? Or as float the same way reach is doing it? I have been assuming the latter.

Ah, yes, those are 32 bit.

mjkkirschner · 2018-06-06T15:36:16Z

src/DynamoWPFCLI/GeometryHolder.cs

+                return;
+            }
+
+            var renderPackages = packages.ToList();


is it necessary to do this iteration?

Probably not, thanks...

mjkkirschner · 2018-06-06T15:37:30Z

src/NodeServices/GraphicsDataInterfaces.cs

    }
+
+    /// <summary>
+    /// The class that represents json data for drawing a graphic primitive 


the interface

mjkkirschner · 2018-06-06T15:37:52Z

@sm6srw looks good to me, only a few comments then LGTM.

mjkkirschner · 2018-06-06T15:41:31Z

@sm6srw thinking a bit more about this I'm curious if the dynamo core dep on dynamic keyword breaks the mono build?

Some more thoughts - if it is not supported in 3.8 I think thats fine, as we should be okay to move on to a newer version of mono if need a cross platform dynamo core, so I think as long as it's supported in newer mono versions we should be okay.

mjkkirschner · 2018-06-06T18:26:11Z

LGTM not withstanding some testing on mono solution - looks like travis and appVeyor checks have stopped running :(.

sm6srw · 2018-06-07T14:22:10Z

Bummer, I was hoping the mono build would happen. Anyway, I checked manually: dynamic is supported by 3.8 and we are using it already in for example ProtoCore that is part of the mono solution. So I think we are good. I am running all the tests one final time and will go ahead and merge this as soon as they are done. Thanks!

mjkkirschner · 2018-06-07T15:32:52Z

src/DynamoCore/Visualization/DefaultGraphicPrimitives.cs

+                else
+                {
+                    foreach (T value in coordinates)
+                        writer.Write(value as dynamic);


@sm6srw just a bit curious - is casting to dynamic actually required here?

Would not compile without it: error CS1503: Argument 1: cannot convert from 'T' to 'bool'

sm6srw · 2018-06-07T18:11:15Z

All tests are passing. I will merge this now.

* 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.

* 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 30 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

Remove comment.

bc4a09d

Don't extend CommandLineRunner.

2274c49

Use var when possible.

968ab9d

Remove commented out code.

e63d8b9

Use DynamoViewModel in CLI.

1fe74e1

Use DefaultWatch3DViewModel for CLI.

f680cd9

Add new project DynamoWPFCLI.

0bee032

Let CommandLineRunnerWPF inherit from CommandLineRunner.

1b1d904

Fix review comments.

fb51053

Fix unstable unit tests.

00f644c

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

2536a47

Move comment.

ff2d90a

sm6srw added WIP and removed WIP labels May 24, 2018

mjkkirschner reviewed May 24, 2018

View reviewed changes

sm6srw added 7 commits May 25, 2018 12:37

Only store the geometry array.

6da9cfb

Refactor.

9337746

Add interface

f629cec

Make interface internal

1ee4020

Rename implmentation.

f3a2dcd

Allow internal access for CLI

9e1c853

Move default implementation to core.

bea0d0b

Make setters private

ae5c6bc

mjkkirschner reviewed Jun 6, 2018

View reviewed changes

Address review comments.

fe7f7ce

mjkkirschner reviewed Jun 7, 2018

View reviewed changes

sm6srw merged commit 8471315 into DynamoDS:master Jun 7, 2018

sm6srw deleted the QNTM-4278 branch June 7, 2018 18:11

saintentropy mentioned this pull request Oct 22, 2018

Cherry Picks for Refinery support in Dynamo 2.0.2 #9198

Merged

7 tasks

QNTM-4278 DynamoWPFCLI should use CoGs geometry format #8887

QNTM-4278 DynamoWPFCLI should use CoGs geometry format #8887

Uh oh!

Conversation

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

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

sm6srw commented May 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw May 24, 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!

sm6srw commented May 30, 2018

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 Jun 6, 2018

Uh oh!

mjkkirschner commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Jun 6, 2018

Uh oh!

sm6srw commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw commented Jun 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sm6srw commented May 24, 2018 •

edited

Loading

sm6srw May 24, 2018 •

edited

Loading

mjkkirschner commented Jun 6, 2018 •

edited

Loading