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

DYN-4215 Forge Unit SDK Nodes #12126

Merged
merged 76 commits into from
Nov 17, 2021

Conversation

saintentropy
Copy link
Contributor

@saintentropy saintentropy commented Oct 8, 2021

Purpose

https://jira.autodesk.com/browse/DYN-4215

This PR includes new Dynamo Unit nodes based on the ForgeUnitsSDK. They surface the Units category as a top level item in the standard node library and add a set of subcategories which include Quantity, Unit, Symbol, and Utilities. For Quantity, Unit, and Symbols the nodes represent the basic underlying capabilities to defined and associated with these types. Utilities includes helper nodes and UI nodes to work with the basic types.

Description

Quantity Nodes: (ie Length, Mass, etc)

  • ByTypeID: Constructor, creates a Quantity from a forge typeId string (ie autodesk.unit.quantity:area-1.0.2)
  • Name: Property, the type's simple name (ie Area)
  • TypeID: Property, the type's forge typeId string (ie autodesk.unit.quantity:area-1.0.2)
  • Units: Property, array of Units associated with this type (ie square feet, square meters, etc)

image

Unit Nodes: (ie feet, meters, grams, etc)

  • ByTypeID: Constructor, creates a Unit from a forge typeid string (ie autodesk.unit.unit:feet-1.0.1)
  • AreUnitsConvertible: Action, indicates if two Units are convertible (ie feet vs inches => true, feet vs grams => false)
  • ConvertibleUnits: Property, array of Units which are convertible from the input Unit.
  • Name: Property, the type's simple name (ie feet)
  • TypeID: Property, the type's forge typeId string (ie autodesk.unit.quantity:feet-1.0.1)
  • QuantitiesContainingUnit: Property, array of Quantities that contain this Unit (ie feet => [Length])

image

Symbol Nodes (ie in, ", etc)

  • ByTypeID: Constructor, creates a Symbol from a forge typeId string (ie autodesk.unit.symbol:in-1.0.1)
  • Text: Property, the string value of the symbol (ie in)
  • TypeID: Property, the type's forge typeId string (ie autodesk.unit.symbol:in-1.0.1)
  • Unit: Property, the Unit associated with this Symbol (ie Inch)
  • SymbolsByUnit: Action, array of Symbols which are associated with the input Unit (ie inch => [in, "])
  • StringifyDecimal: Action, a string formatted to display the input value in a decimal format with the specified Symbol
  • StringifyFraction: Action, a string formatted to display the input value in a fraction format with the specified Symbol

image

Utilities Nodes

  • Quantities: UI DropDown node with all Quantity types available

  • Units: UI DropDown node with all Unit types available

  • Symbols: UI DropDown node with all Symbol types available
    image

  • Convert Units: Updated UI node that allows specification of a value conversion from one Unit type to another. The UI is the same as the existing which allows specification of a Quantity and Units via drop down but with more supported types.

  • ConvertByUnitIDs: Zero touch node which allows conversion of a value from one Unit to another (ie 12in to 1ft). The input type is forge typeId string.

  • ConvertByUnits: Zero touch node which allows conversion of a value from one Unit to another (ie 12in to 1ft). The input type is Unit.
    image

  • Parse Unit Input: UI Node to allow for string input in the node's text input field which is parsed to a specified double value for a specific Unit (ie 25.4mm string input is converted to 1 with the node set to the Unit feet).

  • Parse Expression: Zero touch Node to allow parsing of string input math expression to a value (ie "12*2+1" => 25)

  • Parse ExpressionByUnit: Zero touch Node to allow parsing of string input math expression to a specific unit value (ie "12"*2+1ft" => 3 with the node set to unit feet).
    image

Implementation details... WIP

Todo

  • Bugs
  • Add Nuget for Unit Schemas once publiced

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

Reviewers

FYIs

@Amoursol @QilongTang @jasonstratton @mjkkirschner

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

@saintentropy it looks good, but I still have some concerns -

  1. tests - apologies I just can't remember where we landed on this - but I don't see any tests for the new nodes in this PR?
    2. Where are the schema files? I think there should be documentation in this repo regarding them - maybe a readme in the DynamoUnits folder? figured this out, that they are copied from the nuget.

@mjkkirschner
Copy link
Member

mjkkirschner commented Nov 12, 2021

@saintentropy more thoughts:
* Should we add the forgeUnitCLR reference to the about box?
We'll need to add it to the internal tracking sheet for dependencies
* Is there special licensing that needs to go in the about box?

this is resolved.

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

@saintentropy a few more comments and some questions - I think it's pretty close, ZT stuff looks pretty good, just questions on nodeModel stuff now.

src/Libraries/UnitsUI/NodeViewCustomizations.cs Outdated Show resolved Hide resolved
src/Libraries/UnitsUI/NodeViewCustomizations.cs Outdated Show resolved Hide resolved
src/Libraries/DynamoUnits/Symbol.cs Outdated Show resolved Hide resolved
src/Libraries/DynamoUnits/Symbol.cs Show resolved Hide resolved
src/Libraries/UnitsUI/UnitsUI.cs Outdated Show resolved Hide resolved
src/Libraries/UnitsUI/UnitsUI.cs Outdated Show resolved Hide resolved
src/Libraries/UnitsUI/ViewModelCustomization.cs Outdated Show resolved Hide resolved
src/Libraries/UnitsUI/ViewModelCustomization.cs Outdated Show resolved Hide resolved
@mjkkirschner
Copy link
Member

@saintentropy - thanks for fixing the tests - at the moment my only remaining concern is about the NodeType -
it appears that we are not checking it during deserialization or serialization, and we instead use the .net concrete type...
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Graph/Workspaces/SerializationConverters.cs#L270

I guess thats fine for now, but it starts to break down the entire idea of the schema meaning that each nodeType mapped to a very specific set of properties that other clients could count on. Do these nodes have specific properties that are serialized that are consistent with their type?

@mjkkirschner
Copy link
Member

@saintentropy - last 3 things I think:

  1. updated precision comment
  2. NodeType - I still think ExtensionNode is a better choice unless you feel the new names here are general enough to one day belong as schema types
  3. I think this PR Improve Precision of Units Conversion Node Area #12287 is going to bring in a conflict that will need to be fixed.

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

thanks @saintentropy - after tests pass LGTM. I will repoint my test PR to master instead of your fork.

@mjkkirschner
Copy link
Member

@mjkkirschner
Copy link
Member

@saintentropy there is one failure on master-15

12) Ignored : DynamoCoreWpfTests.RunSettingsTests.RunSettingsControllerSavesAndLoads

Errors, Failures and Warnings

1) Failed : Dynamo.Tests.UnitsOfMeasureTests.Extensions
  Expected string length 7 but was 6. Strings differ at index 5.
  Expected: "5.0000m"
  But was:  "5.000m"
  ----------------^
   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at Dynamo.Tests.UnitsOfMeasureTests.Extensions()

@saintentropy saintentropy merged commit 102bd97 into DynamoDS:master Nov 17, 2021
@QilongTang QilongTang mentioned this pull request Sep 26, 2024
9 tasks
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.

6 participants