Skip to content
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

Feature/bringing pack unpack to dynamo #9005

Closed

Conversation

Mabiro
Copy link

@Mabiro Mabiro commented Jul 20, 2018

Purpose

The purpose of this PR is to bring a new concept of Pack and UnPack nodes inside Dynamo.
The idea behind Packing nodes is to bring in type definitions (partially) inside Dynamo and have a way of building multi-level custom types with basic validations.

TypeDefinition

This feature was initially started with a specific type definition format (pset as JSON). While integrating PackUnPack inside Dynamo Core, it was quite obvious that this needed to be generalized. Without being able to find a dynamo-wide solution that made sense regarding global types, I decided to propose a TypeScript-like syntax to define types on Packing nodes. It uses a simple Parser made using Sprache. Packing nodes will expect a string that fits the Typescript-like syntax.

Syntax

The syntax is quite simple. The initial keyword is Type, which must be followed by a word representing the type name and then a pair of { } containing one or more properties defined as name, :, Type and seperated by a ,. Like in Typescript, [] can be added to a property Type to indicate a collection.

i.e:
image

Faulty Syntax

An error made in the syntax will be outlined by Sprache
image

Builder nodes

I've thought about adding builder nodes for TypeDefinitions, that is, a node with an UI that would allow to define properties and assign known or unknown types to them. I didn't jump in that yet and kept it as a "power-user" tool with the string definition, as I wasn't sure this TypeDefinition proposition would be accepted to begin with. Would be glad to get suggestions regarding this!

Pack

The purpose of the Pack node is to take in a TypeDefinition (currently from a string) and redefining its InPorts to fit with the given type. When running, some basic validation of types occurs.
Assuming no validation errors, it then outputs a dictionary (or list of dictionaries, as it does some lacing) corresponding to the input values.

i.e.:
image

UnPack

The purpose of the UnPack node is to complete the Pack node in a symmetric way. It takes in a TypeDefinition (currently from a string) and redefine its outports to show every property of that type definition. It then expects a dictionary (or list of dictionary) as an additional input and unpacks its content by mapping them to the different OutPorts.

i.e.:
image

Nested Types

It's possible to define types and re-use them in other Pack/UnPack nodes.
image

Notes

  • There are 1 TODO and 2 FIXME which I would like some input on
  • The DataBridge callbacks were used to handle the "dynamic" redefinition of ports + validation. It means that you must be running in Automatic mode or manually run the graph for the packing nodes to update and for type validations to be made. If anyone can bring a solution to that, I'd be glad to hear it!
  • Primitive Geo types were mapped by name, I am sure there will be better suggestions on this. Also, maybe some builtin types are missing and should be added?
  • TypeDefinition is a vague concept, hoping for suggestions/comments.
  • I am sorry for the huge initial commit. I actually implemented this in another repository, which explains why everything was squished into a single commit.
  • Lacing on the pack node was made manually. Due to the dynamic input aspect of the Pack node, I wasn't able to use the out of the box replication/lacing. The UnPack node, on the other hand, uses the out of the box feature.
  • Some tests were failing on my environment, but they weren't related to any of my changes. Can anyone double check that those aren't regressions, but maybe issues with my environment?

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

@saintentropy
@mjkkirschner

Mathieu Bilodeau-Roy added 7 commits July 20, 2018 13:44
-Added Bool and Int32 to known types.
-Exposed "typeid" in the pack node output
…e to force execution of the graph in non-automatic mode.
…king node to force execution of the graph in non-automatic mode."

This reverts commit 2d2a186.
@kronz
Copy link
Contributor

kronz commented Jul 30, 2018

The Pack/Unpack model seems to be pretty speculative at the moment, and needs user validation. This is even before we start talking about whether it is appropriate to be introduced as a general implementation in core (as opposed to a package). Before I close this request, is there any discussion that I am missing to say why this should be part of core?
cc @mccrone , @saintentropy

@Mabiro
Copy link
Author

Mabiro commented Aug 3, 2018

A proposal was made to bring those nodes as part of DynamoSamples and/or package.

Closing this PR.

@radumg
Copy link
Collaborator

radumg commented Aug 15, 2018

Chiming in with a bit of user validation :

  • not come across pack/unpack pattern before, but i like the implementation above. Terminology could be better/more descriptive though
  • on a more general note, i've needed to be able to define custom types/classes often when dealing with web APIs in Dynamo, mostly for de/serialisation purposes, so would re-inforce the need for that functionality.

Unfortunately, the quickest/easiest solutions are still JSON templates (in String node/code block) with find/replace or writing your own class in C#.

Now, there is a Dynamo-native alternative to this : DesignScript has the ability to define classes. But, for some reason, this is disabled in Code blocks completely. You can however define them in .ds files and import then.

It'd be great if DesignScript classes were brought in to code blocks, that would alleviate the need for a whole new type definition language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants