DynamoDS / Dynamo

Open Source Graphical Programming for Design
https://dynamobim.org
Other
1.74k stars 634 forks source link

Many Functions fail for all inputs if the first input is not expected data type #7153

Closed mccrone closed 4 years ago

mccrone commented 8 years ago

If this issue is with Dynamo for Revit, please post your issue on the Dynamo for Revit Issues page.

If this issue is not a bug report or improvement request, please check the Dynamo forum, and start a thread there to discuss your issue.

Dynamo version

1.2.0 release candidate

(Which version of Dynamo are you using? Go to Help > About if you're not sure.)

Operating system

Windows 7

What did you do?

What did you expect to see?

What did you see instead?

Arc.CenterPoint fails for all inputs if the first input is not an arc. Expected behavior is that it should return null for non-arcs regardless of the order in which they were supplied. In the image example, reversing the list of the inputs to Arc.CenterPoint should reverse the outputs as well, not cause the entire node to fail.

arcenterpoint

ke-yu commented 8 years ago

@mccrone it is a known issue that replication doesn't work properly for jagged and/or heterogeneous array.

There are two difficulties to support jagged and/or heterogeneous array.

The first one is performance. Suppose we have two overloaded functions:

def foo(cat: Cat);
def foo(dog: Dog);

And call foo() with {cat, dog, cat, dog, ...}. To dispatch to different foo() for each element in the input list, we have to iterate through the whole input and do method resolution for each element. Note method resolution is an extremely expensive operation. To call non-member function, internally the VM will do some statistics of element types and find the best-matched one; to call member function, the VM currently just takes the type of first item as the target type.

The second is replication would be much complicated. For example, two overloaded functions

def foo(c: Cat, d: Dog[])
def foo(c: Cat[], d: Dog)

Then we call foo() with { {{cat}, {dog}}, {{cat}, {dog}} }. Which foo() should be called? Or both?

Not to mention to call member functions we need to go through the inheritance chain.

How to fix it?

A lot of complexity is related to overloaded functions. Besides these issues, a more serious one that caused by overloaded functions in Dynamo is, a node on the UI might not be the "node" that user wants to execute. Again, consider overloaded functions:

def foo(cat: Cat);
def foo(dog: Dog);

There will be two foo nodes in library. Placing foo(cat: Cat) and connecting a dog to it will eventually execute foo(dog: Dog) instead of foo(cat: Cat) on the UI. This kind of user's intention and run-time execution detachment is confusing.

So one way to fix it is to disable function overload. It definitely will cause some trouble in importing overloaded functions from C# library, but these overloaded functions already don't work properly.

The other way is to resolve function statically, similar to function specialization in C++ template programming. This way needs language support, for example, calling foo() in this way: foo<cat>(c).

@kronz @Benglin @riteshchandawar @Racel FYI

mccrone commented 8 years ago

Thanks for this thorough response, @ke-yu. I understand better where the complexity lies here.

I can also tell now that this issue is much broader than Arc.CenterPoint. Perhaps we should rename this issue to something more descriptive. Here is a simpler example I will reference:

non-reversible

In this case, I start with messy data: three Points and a string. I used the node Point.X from the Autodesk Geometry library to find the X-value of each point I give it. As a user, I would expect the result on the bottom, where null results from a bad input, but a double results from a Point input.

From your comment, I understand that the reason the bottom scenario worked is that the overload to find the property X with a Point as the input was chosen. On the top, a string does not have a property X, so the node fails and does not replicate.

So in the case of an overload, like Point.X and Vector.X, with mixed data, the method chosen for all inputs is based on the type of the first argument. That causes a disturbing potential result that the data changes based on the order of the inputs:

overload-choosing

The same problem does not occur for users of Grasshopper, perhaps because of automatic casting or because methods are chosen based in the item (I don't know which):

gh

I made a workaround for the Arc.CenterPoint problem that started this thread. For my purposes, I needed there to be null values where the input was not an arc and a point where the input was an arc. I created a custom node that split the inputs into groups based on data type, found the center point for all of the arcs, then stitched the data back together in the same order in which it was supplied. This seems cumbersome, but doable. As a user, I would want this type of logic to work for all nodes when I supply messy data--for every input item, find an appropriate overload, and return null per item when it can't. I do understand that the computation could get heavy there. But preserving the mental model for the user is also important.

workaround

kronz commented 8 years ago

@ke-yu thanks for the explaination. One more question here though, it looks like this happens even in situations when there isn't an overload. For instance, 2016-09-15_2052 In these cases, the input is set to ONLY take single items, and ONLY a curve. So, shouldn't there be an unambiguous datatype and replication that can fail on the first element and then move on?

ke-yu commented 8 years ago

@kronz for this case we could use static method instead of instance method. That is, for node Curve.PointAtParameter, instead of comping node to

curve.PointAtParameter(param);  // 1

We compile it to

Curve.PointAtParameter(curve, param) // 2

The difference is, for //1, we look for PointAtParameter() on curve, so if curve is null, we cannot find PointAtParameter(); for //2, we know the method is a static function from Curve, so it only fail for null.

We already create static method for every instance method (so that user could use in code block node). After made this change locally, this is the new result which looks better:

untitled

But if Curve.PointAtParameter is overloaded, the issue will be back again, though not so serious.

Let me make this change soon.

dimven commented 8 years ago

I guess this is related to the old issue here: https://github.com/DynamoDS/Dynamo/issues/5238

The issue is limited purely to when the first item in the list fails. Replication works just fine if the first item passes:

dynamosandbox_2016-09-17_12-18-03

ke-yu commented 8 years ago

Hi all, PR created https://github.com/DynamoDS/Dynamo/pull/7178

johnpierson commented 6 years ago

image Working as requested in 2.0.x releases.

Amoursol commented 4 years ago

Thank you for the submission of this Issue above - We very much do appreciate your time taken to look to improve Dynamo and help it grow and evolve into the future.

In order to better serve the active Dynamo user base and the evolving nature of Dynamo, the Dynamo team has made the decision to close any issue that hasn't had activity since 1st January 2019. This doesn't mean that these issues will not be addressed, but just that they are not being actively worked on as they do not align with our current Dynamo Public Roadmap.

If this issue is still relevant to you and your workflows, please do re-submit in a new Github Issue and link to this closed issue for historical context.

Many thanks, The Dynamo Team