-
Notifications
You must be signed in to change notification settings - Fork 638
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
Compile all UI nodes to static methods #7178
Conversation
@ke-yu - I think this will make everything much clearer for the user. You mentioned in the PR that we should use the static getter in code block nodes like Point.get_X() instead of point.X. What does this mean for "node to code" if anything? Also, will we need to change the UI so that the static getters appear as the node names? fyi @kronz |
Wow, @ke-yu this is great work. |
@sharadkjaiswal @Racel this is a tricky... my initial thought is node to code should use get_X(), but just as Sharad pointed out, it won't work in older version of Dynamo. On the other hand, if we still use .X in node to code, node to code may have different result. We probably should use .X/.foo() in node to code until 2.0. @kronz @riteshchandawar what's your thought? |
good point about consistency @ke-yu, maybe we can just document this issue when users see method resolution error for get_xxxx methods in older version of Dynamo, they will know how to fix it. |
This PR causes some regressions. Previously, for a instance method node, say Surface.PointAtParameter, if its lacing is cross-product, the node will be compiled to code surface<1>.PointAtParameters(us<2>, vs<3>). As PointAtParameter(u:double, v:double) is an instance method, the replication guide <1> after input surface will simply be discarded, so the result will be a 2D list if us and vs both are 1D list and surface is a single object. As now node is compiled to static method, that is, Surface.PointAtParameter(s:surface, u:double, v:double), the replication guide <1> will be used. According to replication guide behavior, it forces to promote input s to a list, therefore the result will be a 3D list. I believe the old behavior is incorrect. But this PR will be reverted to keep the backward compatibility till 2.0. |
@ke-yu why do you say the old behaviour of returning a 2D list is wrong? It seems logical to get a 2D grid of points by doing a cross product with only one surface input to |
@aparajit-pratap then again we break backward compatibility. Promoting a single object to an array if there is a replication guide is current behavior: Changing this will break it. |
Purpose
Previously, instance method nodes and instance property nodes are compiled to the corresponding instance method calls and instance properties. For example,
Point.X
will be compiled toinput.X
andCurve.PointAtParameter
will be compiled toinput1.PointAtParameter(input2)
.But there are two issues.
The first issue is, the function that represented by UI node may not be the function that will be executed at run-time. For example, creating a UI node
Family.Translate
means we expect the input of this node should be a family instance andFamily.Translate()
function should be finally executed. But if we connect a geometry to this node, as there is aTranslate()
member function defined inGeometry
, the node will be executed without any failure.The second issue is, node doesn't work with heterogeneous input. At run-time, the execution engine needs to find out which function should be dispatched to. If the input is an array, say
xs.Translate(...)
, as it is expensive to go through each element inxs
to find out all types of all elements, method resolution logic will assume the target type is same as the type of first element, and tries to look for methodTranslate()
defined on that type. Now supposexs
is{"foo", line, point}
, as the type of first element"foo"
isstring
and no methodTranslate()
defined onstring
, method resolution fails and returnsnull
. See github issue #7153.This PR compiles all UI nodes to static methods.
Point.X
there is a static getterPoint.get_X()
.pt.X
still works, but UI instance property will be compiled to static getter, and we strongly suggest to use static getter in code block node.Curve.PointAtParameter(thisObject: curve, parameter:double)
. Again,curve.PointAtParameter(parameter)
still works, but UI instance method node will be compiled to static version.As node is compiled to static method, method resolution will know that exact function should be dispatched to, plus the enhancement of method resolution in #7175, now we are able to connect heterogeneous input to instance method and property:
Declarations
Note: becuase Dynamo supports function overloads, it may still get confused if there is the other overloaded function with same amount of parameters. For example, in the following graph we connect a numeric value to
Curve.Extrude
'sdirection
input and a vector toCurve.Extrude
sdistance
input, both nodes work (as unexpected):Check these if you believe they are true
*.resx
filesFYIs
@mccrone, @dimven , @kronz @Racel @riteshchandawar @monikaprabhu