Add RegisterForTrace attribute as alternate trigger in Json serialization #8595
Conversation
| ws.Nodes.Where( | ||
| n => n is DSFunction || n is DSVarArgFunction || n is CodeBlockNodeModel || n is Function) | ||
| n => n is DSFunction || n is DSVarArgFunction || n is CodeBlockNodeModel || n is Function || | ||
| n.GetType().GetCustomAttributes(typeof(DynamoServices.RegisterForTraceAttribute),false).Length>0 ) |
There was a problem hiding this comment.
maybe .Any() would be more clear to - also, I wouldn't mind if this was a helper function. - we may already have it around in utils.
There was a problem hiding this comment.
is there any performance implication to adding this check to every node in the graph on save? - how long does it take per node?
There was a problem hiding this comment.
Good points. I will look for a preexisting utility. As for perf I am not sure but I will check. I suspect it can't be too much more as we are selecting and short-circuiting on every zero touch node. There can't be too many nodes that don't fall under the first four conditions. Let me look though if there is a faster way to test for the attribute.
|
please add a test - I know the change is small, but the behavioral change is not small. We should have a test that checks serialization and deserialization works. |
|
@saintentropy maybe you could also document that the register for trace attribute is to be used only on nodeModel types. |
|
@saintentropy another thought. What happens for all our revit nodes where devs have been using the [registerForTrace] attr for no reason for 3 years. It was harmless before - does it cause any unwanted behavior now? |
|
I am working on the tests and I will also examine the implications for the Revit nodes. Still seems like we are adding additional cover to how serialization is aware of trace utilization within the node and should not have material impact. If anyone had ever made a |
|
@saintentropy I understand the intent here. But can you point the JIRA task and the Acceptance criteria? That would help someone in the future if there are any issues. |
|
@saintentropy I'm not really sure what your last comment means. My point is exactly that it appears NodeModel nodes never supported trace and so that is a large change in behavior. To make sure that this behavior is encoded somewhere a test is required. |
|
@benglin @ke-yu @aparajit-pratap Any reason that we specifically excluded NodeModel derived nodes from having tracing data serialized to the Dyn? |
|
@kronz @saintentropy I can't think of any specific reason why we left out NodeModel nodes from serializing trace data. I don't think there was any need until now. We have been adding support for trace to node types incrementally. At one point of time when we realized that element binding wasn't working for custom nodes, only then did we decide to add support for custom nodes. |
|
@saintentropy I have been staring at this and my PR for a long while. Let me get into the office and have a word with @ke-yu, hopefully I'll have a less random answer for you later. |
|
@saintentropy It has been a while, I don't remember all details...Tracing data is to do element binding, i.e., associating elements with the node that creates these elements so that if the node is re-executed, the original elements should be disposed properly. Tracing data is a dictionary updated by DesignScript virtual machine when the virtual machine is executing a function (tracing data format looks like "<unique_ids_to_that_function, element ids>"). Not all But, as most of other nodes only generate empty tracing data, I think you could enable tracing data on all nodes. |
|
@saintentropy @mjkkirschner Is there a JIRA task for this? |
|
@saintentropy Thanks |
|
Ok @mjkkirschner. Now what do you think? |
|
@saintentropy can it wait until friday? I don't see any major change coming, but in case... |
|
That is fine :-) |
|
@mjkkirschner @QilongTang @kronz Ping. Really need to get this into master |
|
@saintentropy LGTM. |
|
Ok I am merging now |
Purpose
This PR allows for Nodes that are derived from NodeModel and utilize tracing mechanism to have binding data serialized to the json file.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner
FYIs
@sm6srw @nonoesp