DynamoDS / DynamoRevit

Dynamo Libraries for Revit
https://dynamobim.org
336 stars 187 forks source link

Roof Nodes #2095

Open johnpierson opened 6 years ago

johnpierson commented 6 years ago

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

2.1.0

Revit version

2018.3

Operating system

Windows 10

What did you do?

Attempted to use the Roof.AddPoint node, Roof.Points and Roof.MovePoint nodes.

What did you expect to see?

The node add the given point to the roof, or retrieve the points.

What did you see instead?

An error (below). Seems the roofs aren't being cast as the correct element type? image image

andydandy74 commented 6 years ago

See #1790

johnpierson commented 6 years ago

Ah I see. Even the points node is failing on a roof that is fact shape edited. Did you have that submitted as well @andydandy74 ?

andydandy74 commented 6 years ago

No, so leave yours open, please.

dimven commented 6 years ago

Dynamo's ElementWrapper will try to wrap as many objects as it can to the predefined wrapper classes and classify everything else as (Unknown)Element. At that stage, any node that expects a specific sub-class of the Element wrapper would usually fail.

Then there's also the fact that we have multiple roof objects in the api and the wrapper might be targeting only one of those:

image

Then there's also the problem that both roofs and floors actually point to a SlabShapeEditor object for controlling point modifications.

Sidetracking a bit, this points to a bigger issue with the way object types are currently handled.

One example of the larger issue that comes to mind is rooms and spaces. They both inherit from Revit's SpatialElement and share most of their methods and properties, yet because there's no wrapper for 'Space', all of the room nodes fail for spaces.

image

You have a few options to fix this - copy the entire code for the room wrapper for spaces and end up with a ton of redundancy, mis-classify all Revit Space objects as wrapped Room objects, or rewrite the current wrapper class for the shared super-class.

Another example - structural framing nodes have a type and location node but under the hood they're all FamilyInstance. And hey, FamilyInstance nodes have their own nodes for the exact same functionality (both in a method and a property flavor for getting the FamilySymbol). In the end, they all inherit from Element and we've got some nodes in there that have you covered too ...

revit_2018-07-18_10-01-04

With the current library ideology, you end up with a ton of redundancy throughout the codebase that just adds noise and confusion. At the same time, people using the final nodes occasionally end up scratching their head when things don't work quite the way you'd expect:

image

It seems to me that the best thing going forward would be to re-write all Revit facing nodes's input types to the lowest common denominator (lowest shared super-class) and then any time you need specialized functionality, perform strict type casting inside the node's method. This way we wouldn't need as many wrapper classes that just muddy things up and would have a smaller code base that's easier to maintain.

On a side note, triplicating everything to be able to support multiple versions of Revit is kind of a drag too. Isn't there a smarter way to control this using build targets and conditionals during build time?

johnpierson commented 6 years ago

Coincidentally, that is what I do in my zero touch nodes. My node inputs generally expect Revit.Elements.Element, and the name of the node should tell the user what it works on. (Or at least I hope so)