-
Notifications
You must be signed in to change notification settings - Fork 635
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-6631 - Input/Output Node - part 2 (View customization) #15022
Conversation
- starting with the model and the test suite
- now types are contained inside an enum - added the basic primitives to the test structure, including lists checks - reworked the node to start getting the customization going (and make sense of the whole thing)
- created hierarchical container capable of tracking type inheritance - added all geometry tests
- finished all primitive date type tests
This reverts commit 1621855.
- completed tests checking inf inheritance works on individual and on list level
- removed the Enum, replaced directly with Type
- removed dictionary in favor of list of datatypes - renamed methods to better suit the specificity of the node functionality they were serving
- now returns the input as an output - not doing much with the result of the validation though, except returning to the first value of the drop-down
UI Smoke TestsTest: success. 2 passed, 0 failed. |
- public static method of retrieving the list of datatypes replaced by an internal static property DataNodeDynamoTypeList
[JsonProperty] | ||
public bool IsList { get; set; } | ||
public bool IsAutoMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dnenov @twastvedt @saintentropy when this node's data types do not match and errors are thrown, does this node return null?
Does it make sense to explore not returning anything and to stop all downstream computation? For example, this node could compile to an imperative AST like scopeIF.
Anyhow, that may be out of scope and we could do it later - it just seems like a large potential waste of compute if this node is used upstream of the entire graph in many cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does (should) return null/nothing. Makes sense to me. Is that (stopping downstream computation) something that other nodes do? If it's not complicated, perhaps we can look at it now. Things left for later tend not to get done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you point me to an existing implementation, I can take that on, up to you @mjkkirschner and @twastvedt !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@twastvedt no - returning null does not stop downstream computation - it passes null and likely leads to a bunch of slow exceptions for all downstream node calls - that's why I am suggesting rethinking that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look here - but it's not simple, if it's something vera team is interested in it probably warrants a meeting as it's a new scope/feature for sure.
[NodeName("ScopeIf"), NodeCategory(BuiltinNodeCategories.LOGIC), |
Purpose
This is a follow-up on the work done in #14987 for the creation of a new
DefineData
node in Dynamo. This PR introduces the beginning of the view customization and stops with the ability to 'validate' the input fed into the node, without going further.Using the 'unlocked' mode, passing data of one of the valid data types would cause the node to automatically assume the 'correct' data type, including single/list value.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Reviewers
@saintentropy
@twastvedt
FYIs
@mjkkirschner