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

Compile all UI nodes to static methods #7178

Merged
merged 11 commits into from
Sep 21, 2016
Merged

Conversation

ke-yu
Copy link
Contributor

@ke-yu ke-yu commented Sep 21, 2016

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 to input.X and Curve.PointAtParameter will be compiled to input1.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 and Family.Translate() function should be finally executed. But if we connect a geometry to this node, as there is a Translate() member function defined in Geometry, 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 in xs 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 method Translate() defined on that type. Now suppose xs is {"foo", line, point}, as the type of first element "foo" is string and no method Translate() defined on string, method resolution fails and returns null. See github issue #7153.

This PR compiles all UI nodes to static methods.

  • The engine will generate a static version of getter for each property. For example, for Point.X there is a static getter Point.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.
  • The engine already generate a corresponding static method for each instance method. For example, 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:

untitled

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's direction input and a vector to Curve.Extrudes distance input, both nodes work (as unexpected):

untitled

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.
  • Follows Semantic Versioning standards (No change of public methods or types etc..)

FYIs

@mccrone, @dimven , @kronz @Racel @riteshchandawar @monikaprabhu

@ke-yu ke-yu merged commit 7eafb9a into DynamoDS:master Sep 21, 2016
@Racel
Copy link
Contributor

Racel commented Sep 21, 2016

@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

@mccrone
Copy link
Contributor

mccrone commented Sep 21, 2016

Wow, @ke-yu this is great work.

@sharadkjaiswal
Copy link
Contributor

@Racel, Node-to-code shouldn't use the static getter, because if this method is used in code block then the saved file won't work in older version of Dynamo. Ideally get_X() shouldn't be visible in autocompletion in the code block. @ke-yu thoughts?

@ke-yu
Copy link
Contributor Author

ke-yu commented Sep 21, 2016

@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?

@sharadkjaiswal
Copy link
Contributor

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.
The static method equivalent for instance method is already there since 1.0, there is no issue for instance methods.

@ke-yu ke-yu mentioned this pull request Sep 21, 2016
7 tasks
@ke-yu
Copy link
Contributor Author

ke-yu commented Sep 21, 2016

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.

@aparajit-pratap
Copy link
Contributor

@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 Surface.PointAtParameter? Can we discard replication guide <1> if the 'surface' input is a single object in the static method and compile it to Surface.PointAtParameter(surface, u<1>, v<2>) so that there is no array promotion for a single object?

@ke-yu
Copy link
Contributor Author

ke-yu commented Sep 22, 2016

@aparajit-pratap then again we break backward compatibility. Promoting a single object to an array if there is a replication guide is current behavior:

untitled

Changing this will break it.

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.

5 participants