Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DYN-6631 - Input/Output Node - part 2 (View customization) #15022
Changes from 21 commits
3785210
93c4741
9c07a73
2bc7903
1621855
33a522c
b92a8d0
1d843d8
49130b6
8cb3a41
6761fe4
cfd197e
a2b023a
0dd95d3
9e01e51
3ff098c
244e975
f689c16
a25b667
b0c46e4
19c565b
3d029c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Dynamo/src/Libraries/CoreNodeModels/Logic/ScopedIf.cs
Line 15 in 5fe7dac