DynamoDS / Dynamo

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

Namespace Collisions with 3rd Party Packages (e.g. Rhynamo) #3208

Closed andydandy74 closed 9 years ago

andydandy74 commented 9 years ago

I noticed this when wanting to create points. Autocomplete behaviour before installing Rhynamo: ac1 And after: ac2 So Point now needs to be called as Autodesk.DesignScript.Geometry.Point. Which is not good at all because it means existing scripts won't execute: acwarning I find it odd (and actually a bit disconcerting) that third party DLLs can have that kind of effect on Dynamo core functionality. CC @archinate

sharadkjaiswal commented 9 years ago

Thanks for reporting this issue. We are aware of this problem and working on user story MAGN-5380 to resolve this. When there are multiple types with the same name, we need to display all the types and to distinguish we start showing them with their complete namespace.

archinate commented 9 years ago

After doing some digging, in this case I believe the issue actually stems from Dynamo getting confused by the OpenNURBS library (Rhino3dmIO.dll) dependency and is introducing conflicting types. This is very odd because OpenNURBS is being referenced by Rhynamo and should not be referenced by Dynamo directly.

I agree that a third party DLL should NOT impact Dynamo core functionality in this manner... but it appears that it does.

At the most basic level, Rhynamo converts between Rhino types and Dynamo types (ProtoCore). When coding Rhynamo, the type "Point" or "NurbsSurface" or "Curve" are all ambiguous and I need to be explicit within my own code...

For example Autodesk.DesignScript.Geometry.Point vs. Rhino.Geometry.Point

Again... I have no idea why Dynamo, itself, would get confused since OpenNURBS is a Rhynamo dependency and should not be referenced by Dynamo.

aparajit-pratap commented 9 years ago

@archinate I see that Rhynamo has nodes that convert between types such as Rhino.Geometry.Point and Autodesk.DesignScript.Geometry.Point. Even though the Rhino.Geometry.Point is not explicitly imported into Dynamo, it needs to be aware of this type as it's being passed around by other Rhynamo nodes. In order to support this, the Zero Touch system in Dynamo creates empty (dummy) types by the name of Rhino.Geometry.Point etc., which then begin to conflict with native types in Dynamo. While this maybe an answer to your question, I agree it's not a solution. We are working on that.

andydandy74 commented 9 years ago

Here's a snippet from the 0.7.5 release notes:

Existing Code Block Nodes are no longer affected by name collisions with functions that come from installed packages. 
For instance, Point.ByCoordinates was affected by a collision with a Point. operation in the popular Rhynamo package and would throw an error saying 
“Warning: Dereferencing a non-pointer. Dereferencing a non-pointer.”
Existing Code Block Nodes and new code block nodes now use unique names for each class.

The autocomplete bug is indeed gone, but this is what happens with Rhynamo installed in 0.7.5 RC: capture The warning is still about dereferencing a non-pointer. Without Rhynamo installed, the Point.ByCoordinates command in the CBN computes just fine.

andydandy74 commented 9 years ago

The same seems to happen with the Document type when DynaWorks is installed.

aparajit-pratap commented 9 years ago

The fix for this issue is still pending design review and is not completely implemented yet. However on importing a new library that has namespace conflicts with classes used in existing code block nodes, you will now see these nodes emit warnings saying the classes used in them now have conflicts. For example, if you import Rhynamo, you will see an existing CBN using 'Point' class now emit a warning saying: "Warning: Multiple definitions for 'Point' are found as Autodesk.DesignScript.Geometry.Point, Rhino.Geometry.Point" Again you won't see this if "Run Automatically" is turned on as the node is executed even before this warning can be displayed and it results in a "Dereferening a non-pointer" runtime warning instead.

Agree that the release notes are not quite accurate in this case.

andydandy74 commented 9 years ago

Then how about listing this as a known issue for 0.7.5 - assuming this fix will not make it into the 0.7.5 release?

andydandy74 commented 9 years ago

Glad to see that this is partially fixed in 0.8 RC. Here's a description of the new behaviour:

  1. Entering Point in a CBN for the first time will still yield the following result in autocomplete: point-autocomplete (So at that point it still looks broken...)
  2. Entering Point. will make Dynamo autocomplete this to PointAnalysisData. - not quite what I was after.
  3. Manually correcting this again to Point. will bring up the following message in the console: Failed to perform code block autocomplete with exception: Value cannot be null. Parameter name: source at System.Linq.Enumerable.Select[TSource,TResult](IEnumerable1 source, Func2 selector) at Dynamo.DSEngine.CodeCompletion.CodeCompletionServices.GetCompletionsOnType(String code, String stringToComplete, ElementResolver resolver) at Dynamo.UI.Controls.CodeBlockEditor.GetCompletionData(String code, String stringToComplete) at Dynamo.UI.Controls.CodeBlockEditor.OnTextAreaTextEntered(Object sender, TextCompositionEventArgs e)
  4. I can manually complete the statement like so: Point.ByCoordinates(0,0,0); and it will execute. Nice!
  5. And now it gets really strange: After having typed in the command manually once, it will appear in the autocomplete suggestions without Autodesk.DesignScript.Geometry. in front of it: point-autocomplete2

Expected behaviour: Autocomplete for Point (and not Autodesk.DesignScript.Geometry.Point) should be available right away without having to jump through these hoops.

aparajit-pratap commented 9 years ago

@andydandy74 thanks for pointing out this discrepancy. The first auto-complete list where all the conflicting namespaces are displayed on typing Point is expected. The system cannot automatically differentiate between classes with name Point that occur in different libraries (in this case Dynamo itself and Rhino both have classes Point). Autocomplete therefore has to display the fully qualified names to the user so that the user can explicitly specify which of these Point classes he intends to use when he simply says Point.

Secondly if autocomplete displays multiple options for Point, it means that the code block cannot resolve just Point as a class name. It needs to know additionally which namespace it comes from - Autodesk or Rhino. Therefore, I agree with you that the fact that Point.ByCoordinates(0,0,0); executed successfully, is actually confusing because in reality it should have failed. There is a discrepancy in the engine core, which is being tracked as a bug here and will be resolved soon: http://adsk-oss.myjetbrains.com/youtrack/issue/MAGN-6838

aparajit-pratap commented 9 years ago

Also conflicting namespaces are now shortened into shortest names that can be uniquely resolved. For example, Autodesk.DesignScript.Geometry.Point and Rhino.Geometry.Point will now be shortened and displayed as Autodesk.Point and Rhino.Point resp. You may see this in the latest daily build: https://github.com/DynamoDS/Dynamo/pull/4060

andydandy74 commented 9 years ago

From a user perspective (or you might as well call it my personal bias) I would say that it would make a lot of sense to find a solution where Dynamo's own library always takes precedence. Imagine what would happen if somebody made a package for, lets say geometry conversion to another software / file format that had a lot of commands with names of generic geometric entities like Point, Vector, Curve, Surface etc. In that case I as a user would have to choose between: a) Explicitly calling the Dynamo methods each time I use a code block, b) not using code blocks any more for operations like that because they've become a nuisance or c) uninstalling the 3rd party package. I know that I, for one, would not pick option a). @aparajit-pratap - If I understood you correctly, you're saying that after the resolution of the "discrepancy" in the engine, a statement like Point.ByCoordinates(0,0,0) would not execute any longer in the scenario I described in my previous post. Please consider the following scenario and its ramifications: Person A uses code blocks in custom nodes without explicitly calling Autodesk.DesignScript.Geometry.Point (because he has no "interfering" 3rd party package installed). He then publishes it to package X. Person B installs package X and also another package like Rhynamo that adds a second Point class. This would effectively render the affected nodes in package X useless.

aparajit-pratap commented 9 years ago

Yes that's right, resolution of the engine issue will no longer execute the statement Point.ByCoordinates(); in the above scenario. You will have to specify Autodesk.Point.ByCoordinates(); or the fully qualified Autodesk.DesignScript.Geometry.Point.ByCoordinates(); for it to work. We did consider giving a preference to all Dynamo native classes without having to specify namespaces for them however decided not to take that route for various reasons. Even in the case where you have class conflicts and have to use native Dynamo classes, I don't see why you would need to "explicitly" call them as autcomplete will still display them, but only with a prefix of Autodesk. which you will have to pick. As for your concern about the following scenario:

"Person A uses code blocks in custom nodes without explicitly calling Autodesk.DesignScript.Geometry.Point (because he has no "interfering" 3rd party package installed). He then publishes it to package X. Person B installs package X and also another package like Rhynamo that adds a second Point class. This would effectively render the affected nodes in package X useless."

... this issue has already been taken care of: https://github.com/DynamoDS/Dynamo/pull/3255

You can try it out just as you said: Open a clean Dynamo version without any 3rd party packages /libraries installed. You should be able to use Point as is in a code block node. Save the file (DYN/DYF) with the CBN using Point. Import libraries that introduce conflicts with Point such as Rhynamo. Open the file. Your code block using Point should continue to work as before as Point has been internally mapped to Autodesk.DesignScript.Geometry.Point. Now if you type Point in a new code block you will simply see Point and Rhino.Point in autocomplete as the system now knows how to resolve Point and needs just one additional qualifier to distinguish it from Rhino.Point.

andydandy74 commented 9 years ago

@aparajit-pratap - thanks for clearing that up. One less thing to worry about. :-)