DYN-2538 Generate single RenderPackage per Node #11395

saintentropy · 2021-01-11T05:19:57Z

Purpose

Note: I was cleaning up my Dynamo git stash over the holiday break and this was too good to leave on the shelf. This builds on some work that @LongNguyenP, @Dewb, @aparajit-pratap, and I looked at a while back. I picked up and fixed the regressions we noted previously.

The purpose of this PR is a performance and memory optimization for the render pipeline. The main architectural change is modifying the assumption that we generate a single RenderPackage object per every individual return object for a node. Instead we aggregate all node tessellation data into a single RenderPackage recursively. This improves tessellation performance as the HelixRenderPackage is a large .NET object with a non-trivial init time in addition to a large saving in memory; specifically one render package with all the tessellation data is much smaller then many render packages with partial tessellation data. This approach also creates performance benefit and memory savings when the tessellated data is added to the Helix scene. Processing one RenderPackage per AggregateRenderPackages call is dramatically more efficient than multiple render packages as there are numerous collection interactions and temporary allocation. In code, this change was enabled by generating the RenderPackage first in GetRenderPackagesFromMirrorData before the recursive loop to fill the render package data. Previously it was generated within the loop.

There are a few things that require changes to support features which assumed a single item = a single RenderPackage.

Labels (ie the labels that appear in the scene context by selecting items in the node preview). The existing implementation encoded label information in the Description property of the RenderPackage. That encoding was not well documented making it nearly impossible to add labels to the your own tessellation method overrides. Here I introduce a new interface, IRenderLabels, which provides methods on the render package to add label data and placement. The new underlying storage implementation matches the existing LabelPlaces data structure in the HelixWatch3DViewModel so that adding labels requires much less string parsing and processing.
Texture maps. The original texture map implementation in the Colors and ColorStride property assumed only one texture map per the mesh geometry in the RenderPackage. To support aggregation of multiple mesh geometries within a single RenderPackage with unique textures, I introduce a new interface, IRenderPackageSupplement, which supports adding lists of Colors and ColorStrides along with their associated mesh vertex ranges. The Colors and ColorStride property as well as the SetColors method are marked as obsolete with notes to use the new methods in the IRenderPackageSupplement.
Color adding methods on IRenderPackage. While methods to add vertex color like AddPointVertexColor still work fine with this change, when utilized in tessellation calls, methods like ApplyPointVertexColors overwrite the previous data vs appending. That can cause data loss as they are currently used. I introduce new methods to IRenderPackageSupplement like InsertPointVertexColorRange and AppendPointVertexColorRange to allow additions without overwriting existing data. The old methods are also marked as obsolete with notes to use the new methods in the IRenderPackageSupplement. The GeometryColor nodes are updated to use the new methods.

These new features are being introduced with a minor release and not Dynamo 3.0. While we can update the tessellation implementation in Dynamo and Dynamo integrations, Third Party tessellation functions also need to stay working. For each of the Obsolete methods above I introduced an Exception to allow the GetRenderPackagesFromMirrorData implementation to detect their usage. Specifically within IRenderPackageSupplement there is a boolean AllowLegacyColorOperations that can be set to false which will cause the LegacyRenderPackageMethodException to be thrown when the obsolete methods are called. In that case, we roll back any changes to the RenderPackage that occurred within the Tesselate call and generate a new RenderPackage with AllowLegacyColorOperations set to true and re-Tesselate. This individual render package is added to the RenderPackageCache and the outer loop resumes. In the transition to Dynamo 3.0, the obsolete methods can be removed as well as this fallback.

Todo

Add tests for the new functionality
Update Tessellate implementation in DynamoRevit, other integrations, samples, or internal packages. This is not strictly required due to the fallback mechanism.
Fix the Label.ByPointAndString node to work with the new label mechanism so that the node actually work.
ITransformable needs to be refactored similarly to the Texture Map implementation. This may be covered under a separate PR as we can possibly take advantage of the refactor to introduce instancing mechanism.

Examples

PointsNew.mp4

PointsOld.mp4

LinesNew.mp4

LinesOld.mp4

For most point and curve tessellation the speed and memory improvement will be linear based on the number of tessellated items. However for meshes the speed and memory improvement is dramatic. The original implementation would take exponentially longer to add mesh geometry to the scene depending on the number of items. This refactor restores performance to a linear relationship

# Meshes	Now (s)	Memory Alloc	Before (s)	Memory Alloc
~10000	7.95s	334mb	17.12s	668mb
~30000	17s	1.1gb	86s	2.1gb
~68000	38s	2.6gb	Crash	NA

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

Reviewers

@mjkkirschner @aparajit-pratap @QilongTang

…r package

…ng the description field

… render package per node vs per node return obj

…labels vs the cryptic use of Description.

…lation calls

… public prop with Helix types

…usage of the color methods in IRenderPackage

…or methods in tessellation calls. If Detected, revert changes to render package and generate a new render package with legacy methods per IGraphicItem.

mjkkirschner · 2021-01-12T22:57:48Z

@aparajit-pratap one downside of marking this PR as a draft is that the tests do not run - we'd have to modify the code that runs on the self serve pull job - and I think it makes use of https://github.com/microsoft/PowerShellForGitHub ... I gave up once I got that far.

mjkkirschner · 2021-01-13T16:21:30Z

@saintentropy it would be good to run the wpf visualization test jobs - there are versions of that job that run with warp and another that runs on a real GPU - these do image comparisons so should be useful.

src/DynamoCoreWpf/Rendering/HelixRenderPackage.cs

src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs

aparajit-pratap · 2021-04-23T14:42:50Z

src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs

+            }
+        }
+
+        private void GetTessellationDataFromGraphicItem(Guid outputPortId, IGraphicItem graphicItem, IRenderPackage package, string labelKey)


I think package should be passed as a ref parameter as it can be assigned a new object inside the function?

Definitely! Good catch

src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs

aparajit-pratap · 2021-04-26T02:08:02Z

@saintentropy thanks for addressing the review comments and adding inline documentation. Also thanks for clearer names for the new APIs. I have just two remaining questions, then LGTM.

saintentropy · 2021-04-26T13:45:17Z

Hi @aparajit-pratap Added some of fixes from the comments and a lot more tests

saintentropy · 2021-04-26T13:50:17Z

I also added the single item label fix. The regression was introduced in this PR -> #10989. That PR also added the non-functional DisplayLabel node back into core so this PR fixes both the node and the label regression.

aparajit-pratap · 2021-04-26T14:36:46Z

@saintentropy when I saw your visualization tests I remembered you could also add image comparison tests that @mjkkirschner introduced here https://github.com/DynamoDS/Dynamo/blob/master/src/VisualizationTests/HelixImageComparisonTests.cs. This could be especially useful for the texture map graph you showed with the 3 surfaces. It doesn't have to be in this PR, can be a separate one as we discussed.

aparajit-pratap · 2021-04-26T16:27:14Z

src/VisualizationTests/HelixRenderPackageTests.cs

+            try
+            {
+                p.AppendMeshVertexColorRange(testColors);
+            }
+            catch(Exception e)
+            {
+                Assert.AreEqual(e.GetType(), typeof(Exception));
+            }


Assert.ThrowsException<Exception>(() => p.AppendMeshVertexColorRange(testColors));

How are these exceptions dealt with in Dynamo in a real situation?

All the exceptions (my new ones and the old ones) within the RenderPacakge implementation are caught here -> https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs#L303. The net affect is that the node does not display geometry in the background preview.

The only difference in the new implementation is the specific handling of the LegacyRenderPackageMethodException

aparajit-pratap · 2021-04-26T16:31:28Z

src/VisualizationTests/HelixRenderPackageTests.cs

+            {
+                p.AppendLineVertexColorRange(testColors);
+            }
+            catch (Exception e)


Can we make these exceptions thrown be more specific? In the implementation I mean.

I think I can cover these in the next PR as I add more test coverage.

saintentropy added 15 commits January 5, 2021 11:46

Add additional interfaces to support adding colors in chunks to rende…

73d2244

…r package

Add interfaces for adding label data to a render package vs overloadi…

f512c4a

…ng the description field

Add implementation of new Interfaces to HelixRenderPackage

59d653e

Change GetRenderPackagesFromMirrorData implementation to only use one…

8aefd74

… render package per node vs per node return obj

Modify AggregateRenderPackages to utilize the IRenderLabels data for …

8f6d369

…labels vs the cryptic use of Description.

Modify mesh creation to support multiple texture maps per render package

0a36ee9

Update GeometryColor nodes to utilize the new Interface API's

4aefed2

Extract plane tessellation method

703b7b5

Validate vertex lists and vertexcolor lists stay in sync after tessel…

aca3964

…lation calls

Fix Test failures

91a1149

clean up internal method for LabelPlaces. Match same pattern as other…

2b3b2db

… public prop with Helix types

Add obsolete attributes to IRenderPackage methods

2c060f9

Add property and exception to allow detection and handling of legacy …

e7840ba

…usage of the color methods in IRenderPackage

Modify GetRenderPackageFromMirrorData to support detecting legacy col…

f49d8e6

…or methods in tessellation calls. If Detected, revert changes to render package and generate a new render package with legacy methods per IGraphicItem.

Clean up comment and simplify implementation.

2bffdad

saintentropy added the WIP label Jan 11, 2021

saintentropy requested review from QilongTang, mjkkirschner and aparajit-pratap January 11, 2021 06:04

saintentropy added 4 commits January 11, 2021 01:26

Fix crash

76b58a0

Don't push empty render packages onto the cache

82f035a

Add IRenderPackageSupplement to DefaultRenderPackage for DynamoWPFCLI

c6242f9

Fix label node and tests

ffe80dd

aparajit-pratap marked this pull request as draft January 12, 2021 21:08

aparajit-pratap marked this pull request as ready for review January 13, 2021 03:33

aparajit-pratap added the DNM Do not merge. For PRs. label Jan 13, 2021

saintentropy removed the WIP label Jan 15, 2021

saintentropy changed the title ~~WIP DYN-2538 Generate single RenderPackage per Node~~ DYN-2538 Generate single RenderPackage per Node Jan 15, 2021

aparajit-pratap reviewed Apr 21, 2021

View reviewed changes

src/DynamoCoreWpf/Rendering/HelixRenderPackage.cs Show resolved Hide resolved

aparajit-pratap reviewed Apr 21, 2021

View reviewed changes

src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs Outdated Show resolved Hide resolved

saintentropy added 3 commits April 23, 2021 07:29

Update method name and add comments

cdc90b0

Update comments on plane tessellation

54ae3cd

Dispose temp geometry in plane tessellation

d299072

aparajit-pratap reviewed Apr 23, 2021

View reviewed changes

src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs Show resolved Hide resolved

saintentropy added 5 commits April 24, 2021 13:46

Update the Legacy mechanism with clear comments for the use of count

a547190

Update method descriptions and method names for interfaces

e4a73f4

Update per PR comments

b7f5652

remove field references from instance methods

657002f

Fix label regeression introduced in Dyn 3026

d5dbb6b

saintentropy added 6 commits April 26, 2021 09:03

pash package by ref

c652d30

Rename Insert color ranges to Update to match validation behavior

ef33e81

Fix bug in AppendMethod validation

207247f

Add visualization tests and dyn files

55758b6

Add Helix Render package tests

8627930

PR comment

11ca407

aparajit-pratap reviewed Apr 26, 2021

View reviewed changes

Merge branch 'master' into SingleRenderPackagePerNode

ea432c1

saintentropy removed the DNM Do not merge. For PRs. label Apr 27, 2021

aparajit-pratap approved these changes Apr 27, 2021

View reviewed changes

aparajit-pratap merged commit 9740b24 into DynamoDS:master Apr 28, 2021

saintentropy mentioned this pull request Apr 30, 2021

Update Helix Toolkit from 2.11 to 2.15 #11658

Merged

8 tasks

saintentropy mentioned this pull request Aug 5, 2021

DYN-3896 Add support for geometry instancing in render pipeline #11914

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-2538 Generate single RenderPackage per Node #11395

DYN-2538 Generate single RenderPackage per Node #11395

saintentropy commented Jan 11, 2021 •

edited

Loading

mjkkirschner commented Jan 12, 2021

mjkkirschner commented Jan 13, 2021 •

edited

Loading

aparajit-pratap Apr 23, 2021 •

edited

Loading

saintentropy Apr 26, 2021

aparajit-pratap commented Apr 26, 2021

saintentropy commented Apr 26, 2021

saintentropy commented Apr 26, 2021

aparajit-pratap commented Apr 26, 2021

aparajit-pratap Apr 26, 2021

aparajit-pratap Apr 26, 2021

saintentropy Apr 27, 2021

saintentropy Apr 27, 2021 •

edited

Loading

aparajit-pratap Apr 26, 2021 •

edited

Loading

saintentropy Apr 28, 2021

DYN-2538 Generate single RenderPackage per Node #11395

DYN-2538 Generate single RenderPackage per Node #11395

Conversation

saintentropy commented Jan 11, 2021 • edited Loading

Purpose

Declarations

Reviewers

mjkkirschner commented Jan 12, 2021

mjkkirschner commented Jan 13, 2021 • edited Loading

aparajit-pratap Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

saintentropy Apr 26, 2021

Choose a reason for hiding this comment

aparajit-pratap commented Apr 26, 2021

saintentropy commented Apr 26, 2021

saintentropy commented Apr 26, 2021

aparajit-pratap commented Apr 26, 2021

aparajit-pratap Apr 26, 2021

Choose a reason for hiding this comment

aparajit-pratap Apr 26, 2021

Choose a reason for hiding this comment

saintentropy Apr 27, 2021

Choose a reason for hiding this comment

saintentropy Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

saintentropy Apr 28, 2021

Choose a reason for hiding this comment

saintentropy commented Jan 11, 2021 •

edited

Loading

mjkkirschner commented Jan 13, 2021 •

edited

Loading

aparajit-pratap Apr 23, 2021 •

edited

Loading

saintentropy Apr 27, 2021 •

edited

Loading

aparajit-pratap Apr 26, 2021 •

edited

Loading