DYN-6052 Add node info event and API under DynamoServices package for ProtoCore #14234

QilongTang · 2023-08-07T21:13:31Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Add node info event and API under DynamoServices package for ProtoCore so libG could raise info message instead of warning message.
Add mechanism for graph run to consume runtime infos and connect each of them with node.

Declarations

Check these if you believe they are true

The codebase 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.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

pinzart90 · 2023-08-07T22:07:17Z

src/Engine/ProtoCore/RuntimeStatus.cs

+            public Guid GraphNodeGuid;
+            public int AstID;
+            public string Filename;
+        }


I wish we didn’t have to expose more APIs from the engine level

I did look at it but unfortunately because the current public interface ILiveRunner setup, the function involved needs to be public and also the return object as well. VS does throw errors if trying to make this internal. I have made as much as I can internal or private

Is this really a good candidate for a struct?

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/choosing-between-class-and-struct

@mjkkirschner I am not sure but maybe we could break these changes in 3.0. Right now I am just following the previous design for all these engine related code

QilongTang · 2023-08-08T20:18:02Z

src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs

+                    continue;
+                using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.ToolTipText), nameof(NodeModel.Infos), nameof(NodeModel.State)))
+                {
+                    node.Info(string.Join(Environment.NewLine, info.Value.Select(w => w.Message)));


This is the place where we would display node info returned from libG

QilongTang · 2023-08-08T20:21:27Z

src/Engine/ProtoCore/RuntimeStatus.cs

@@ -99,6 +139,7 @@ public void ClearWarningForExpression(int expressionID)
            using (rwl.CreateWriteLock())
            {
                warnings.RemoveAll(w => w.ExpressionID == expressionID);
+                infos.RemoveAll(w => w.ExpressionID == expressionID);


@aparajit-pratap I am not 100% sure about the usage of these function or if infos need to be reset. But I guess we want to clear the runtime infos every time graph runs. Please correct me if I was wrong.

This could be tricky. In the case of geometry scale info messages, we would want it to be associated with node execution, while in other contexts, it may just be a static info message that has nothing to do with the node execution, in which case this may not be applicable.

QilongTang · 2023-08-08T20:27:03Z

src/NodeServices/MessageEvents.cs

+    public static class LogInfoMessageEvents
+    {
+        public static event LogInfoMessageEventHandler LogInfoMessage;
+        public static void OnLogInfoMessage(string message)


This is the new API ProtoGeometry will call to raise info instead of warning

src/Engine/ProtoCore/RuntimeStatus.cs

aparajit-pratap

@QilongTang I tried an alternative approach using VMDataBridge in the referenced PR, please check it out. It avoids having to make these many changes at the VM level.

aparajit-pratap

@QilongTang I'm okay with going ahead with this PR instead of mine as I wasn't able to figure out how to clear Info messages using the VM Databridge approach. If you could address review comments made so far, that would be great.

QilongTang · 2023-08-14T20:48:30Z

Addressed the comments, the unit test with an example of a large number node triggering info cant be tested without the change to libG side. So I may merge this first and have a follow-up PR to add unit test.

src/Engine/ProtoCore/BuildStatus.cs

src/NodeServices/MessageEvents.cs

aparajit-pratap

Few more comments.

QilongTang · 2023-08-15T04:45:05Z

Since we do not have 3.0 build working yet, I will have to merge to master -> cherry-pick to 2.19 branch and then use a 2.19 beta nuget to test. Merging.

… ProtoCore (#14234) * Add node info event and API * Update * Update * clean up * remove line and column from InfoEntry * comments * clean up * clean up

* DYN-6052 Add node info event and API under DynamoServices package for ProtoCore (#14234) * Add node info event and API * Update * Update * clean up * remove line and column from InfoEntry * comments * clean up * clean up * update

Add node info event and API

cadd563

pinzart90 reviewed Aug 7, 2023

View reviewed changes

QilongTang added 2 commits August 8, 2023 16:03

Update

83e8940

Update

bd490ac

QilongTang commented Aug 8, 2023

View reviewed changes

clean up

e242e03

QilongTang commented Aug 8, 2023

View reviewed changes

src/Engine/ProtoCore/RuntimeStatus.cs Outdated Show resolved Hide resolved

QilongTang changed the title ~~[WIP] Add node info event and API under DynamoServices package for ProtoCore~~ DYN-6052 Add node info event and API under DynamoServices package for ProtoCore Aug 8, 2023

QilongTang added this to the 2.19.0 milestone Aug 8, 2023

QilongTang marked this pull request as ready for review August 8, 2023 20:29

aparajit-pratap reviewed Aug 8, 2023

View reviewed changes

src/Engine/ProtoCore/RuntimeStatus.cs Outdated Show resolved Hide resolved

aparajit-pratap mentioned this pull request Aug 9, 2023

Log Info on ZT nodes using VM Databridge #14241

Closed

9 tasks

aparajit-pratap reviewed Aug 9, 2023

View reviewed changes

QilongTang mentioned this pull request Aug 9, 2023

Geometry Scaling Warning in D4C3D #14143

Closed

aparajit-pratap reviewed Aug 14, 2023

View reviewed changes

remove line and column from InfoEntry

40cdf23

aparajit-pratap reviewed Aug 15, 2023

View reviewed changes

src/Engine/ProtoCore/BuildStatus.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Aug 15, 2023

View reviewed changes

src/NodeServices/MessageEvents.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Aug 15, 2023

View reviewed changes

QilongTang added 3 commits August 15, 2023 00:22

comments

489fa44

clean up

626c0c1

clean up

8fd7999

QilongTang merged commit c15e215 into master Aug 15, 2023

QilongTang deleted the NodeInfoAPIUnderNodeServices branch August 15, 2023 04:45

QilongTang mentioned this pull request Aug 15, 2023

Cherry-Pick DYN-6052 #14234 #14260

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6052 Add node info event and API under DynamoServices package for ProtoCore #14234

DYN-6052 Add node info event and API under DynamoServices package for ProtoCore #14234

QilongTang commented Aug 7, 2023 •

edited

Loading

pinzart90 Aug 7, 2023

QilongTang Aug 8, 2023 •

edited

Loading

mjkkirschner Aug 8, 2023

mjkkirschner Aug 8, 2023

QilongTang Aug 8, 2023

QilongTang Aug 8, 2023

QilongTang Aug 8, 2023

aparajit-pratap Aug 8, 2023

QilongTang Aug 8, 2023

aparajit-pratap left a comment •

edited

Loading

aparajit-pratap left a comment

QilongTang commented Aug 14, 2023

aparajit-pratap left a comment

QilongTang commented Aug 15, 2023

DYN-6052 Add node info event and API under DynamoServices package for ProtoCore #14234

DYN-6052 Add node info event and API under DynamoServices package for ProtoCore #14234

Conversation

QilongTang commented Aug 7, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

Choose a reason for hiding this comment

QilongTang Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap left a comment • edited Loading

Choose a reason for hiding this comment

aparajit-pratap left a comment

Choose a reason for hiding this comment

QilongTang commented Aug 14, 2023

aparajit-pratap left a comment

Choose a reason for hiding this comment

QilongTang commented Aug 15, 2023

QilongTang commented Aug 7, 2023 •

edited

Loading

QilongTang Aug 8, 2023 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading