-
Notifications
You must be signed in to change notification settings - Fork 632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MAGN-9744 Studio 1.0.0.775 partially renders graphics on first try #6356
Conversation
b9972a0
to
6139b28
Compare
if (Camera != null) | ||
{ | ||
values.Sort(new Model3DComparer(Camera.Position)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM |
@@ -149,14 +149,14 @@ public void Node_RenderingUpToDate() | |||
p1.UpdateValue(new UpdateValueParams("IsVisible", "false")); | |||
|
|||
Assert.True(BackgroundPreviewGeometry.HasNumberOfPointsCurvesAndMeshes(7, 6, 0)); | |||
Assert.AreEqual(6, BackgroundPreviewGeometry.NumberOfInvisiblePoints()); | |||
Assert.AreEqual(1, BackgroundPreviewGeometry.NumberOfInvisiblePoints()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marimano Are we just fixing the test to match the behavior here, or have the changes you made actually result in there being less objects in the scene?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikeough these changes in tests are done to match new behavior which is presented in this PR. All points of a node are stored in a single geomentry instead of storing each array item of the node separately. So, here are 6 points which are stored in 1 PointGeometryModel3D
from now
@marimano Thanks very much for this fix. The numbers look good! I had one question about the tests. |
@marimano BTW: where you say "Competition" we usually refer to that process as "Benchmark" |
Purpose
First of all, task name is not accurate because geometry is fully rendered after some time.
When there is a lot of geometry, Dynamo computes and renders it quite slowly. The PR adds some optimization to partly overcome the issue:
SceneItems
is constantly being called, so each time it generates new list and sorts it.In this PR
SceneItems
is computed only after changes in its source;In this PR node array geometry items are stored together as it had used to be and instead label positions are stored in a separate place, so when a node requests displaying label for its array item we know where to place the label.
Analysis
I made a competition between Dynamo without my changes and Dynamo with them. It was used
lotsofstuff.dyn
from the task.Render time for each node means time to generate geometry objects from render packages for the node.
setting properties and rendering time
means time to render already generated geometry. Render packages for each node are coming separately and between receiving packages, Helix renders what it already has.sum
time means time passed from receiving the very first render packages to finishing render of all geometry.So, it this case we have 9 seconds of gain in time.
In this case designations are the same, code block just gives much more array items, we have 1 minute and 19 seconds of gain in time.
These time values are got from output window in Visual Studio. Also, I noted the time on my smartphone starting at pressing Run button and stopping at all geometry appears. The results are next:
By the way computing render packages for a node with many array items is time-consuming operation as well. The PR does not consider this component of the delay in rendering
Declarations
*.resx
filesReviewers
@mjkkirschner
FYIs
@jnealb @kronz @ikeough