Leave RenderPackage data intact when passed to AggregateRenderPackages by saintentropy · Pull Request #13437 · DynamoDS/Dynamo

saintentropy · 2022-10-26T21:12:43Z

Purpose

The purpose of this PR is to modify the behavior of the AggregateRenderPackages method. Current method is triggered based on the NodeModel.RenderPackagesUpdated event. Currently a the AggregateRenderPackages modify the RenderPackage object it is handed via the event when it is processing data to add to the view. The issue with this modification is that other consumers this event will receive a modified RenderPackage which does not contain the original data. The PR changes the the RenderPackage data is processed so that it is not modified.

In testing this PR I also noticed some scenarios today which Render incorrectly. Edge cases where a list is created combining objects that are rendered via instance data, objects with texture maps, and regular geometry.

Todo

Add tests for broken render scenarios
Add tests to validate no changes to the RenderPackage

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

Release Notes

Update API behavior for AggregateRenderPackages.
Fix regressions in background preview render for some list of objects.

Reviewers

@mjkkirschner @aparajit-pratap

FYIs

@twastvedt

mjkkirschner · 2022-10-27T15:13:10Z

Hi @saintentropy

please use the reviewers section in github when possible
It seems there are 2 failing tests here that are real, one is a UI test that is flaky. FYI @QilongTang
What release is this intended for?

mjkkirschner · 2022-10-27T15:41:51Z

@saintentropy to review this for correctness I think I'm going to need a more detailed description about what was changed/ how it works - at this point I have no memory of / bandwidth to get deep into this code.

mjkkirschner

it seems okay, a few questions, and I think it would be good if we could create a regression test for the failure - maybe 2 subscribers to the event?

src/DynamoCoreWpf/Rendering/HelixRenderPackage.cs

mjkkirschner · 2022-10-28T01:07:21Z

src/VisualizationTests/HelixWatch3DViewModelTests.cs

            ViewModel.RenderPackageFactoryViewModel.ShowEdges = true;
            //this graph displays a grid, cones, cone edges, cube instances, cube edge instances.
-            Assert.AreEqual(3, BackgroundPreviewGeometry.NumberOfVisibleCurves());
+            Assert.AreEqual(2, BackgroundPreviewGeometry.NumberOfVisibleCurves());


grid lines, cube edges and cone edges are not 3 visible curve objects?

Yeah this is where there was a error in the original test. NumbersOfVisibleCurves removes grids from the number it returns. So the expected behavior is 2. Cube edges and Cone Edges. Not sure what the third was before. Probably an empty item.

Same for the axes but I can't remember if that is curve or mesh geometry.

src/DynamoCoreWpf/Rendering/HelixRenderPackage.cs

mjkkirschner · 2022-10-29T02:59:22Z

src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs

                                var startIndex = range.Item1; //Start line vertex index
                                var count = range.Item2 - range.Item1 + 1; //Count of line vertices
-                                var uniqueId = baseId + ":" + j + LinesKey;
+                                var uniqueId = baseId + ":" + j + LinesKey + "_Instance";


whats this change about?

The current implementation has a collision on uniqueIds if a render package contains instanced mesh and multiple texture maps. This change just makes sure the unique Ids for each type of helix geometry is actually unique.

mjkkirschner · 2022-10-29T03:01:35Z

src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs

                            var startIndex = range.Item1; //Start mesh vertex index
                            var count = range.Item2 - range.Item1 + 1; //Count of mesh vertices
-                            var uniqueId = baseId + ":" + j + MeshKey;
+                            var uniqueId = baseId + ":" + j + MeshKey + "_Texture";


oh I see, just to make debugging it easier?

See above but yes

src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs

twastvedt · 2022-11-10T17:12:39Z

it seems okay, a few questions, and I think it would be good if we could create a regression test for the failure - maybe 2 subscribers to the event?

I'm looking into making this test. As far as I can tell I need to be able to modify the feature flags, as the issue only happens when the graphics-primitive-instancing flag is false, but in test mode that flag is set to true. I did a little looking around but didn't see another place where those flags are being modified. Does that sound right, and is that possible?

mjkkirschner · 2022-11-10T21:46:07Z

it seems okay, a few questions, and I think it would be good if we could create a regression test for the failure - maybe 2 subscribers to the event?

I'm looking into making this test. As far as I can tell I need to be able to modify the feature flags, as the issue only happens when the graphics-primitive-instancing flag is false, but in test mode that flag is set to true. I did a little looking around but didn't see another place where those flags are being modified. Does that sound right, and is that possible?

Hi @twastvedt - maybe @saintentropy can confirm - I thought this issue only happened when instancing was true!?

But if you need to modify the flags during your test for some reason - I think what you can do is make this property internal and set it:
https://github.com/DynamoDS/Dynamo/blob/master/src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs#L86
then set it back.

or maybe it's easier to just override this cache, since it's in the same process:

Dynamo/src/DynamoUtilities/DynamoFeatureFlagsManager.cs

Line 61 in a24d3c1

    
           AllFlagsCache = JsonConvert.DeserializeObject<Dictionary<string, object>>(dataFromCLI);

twastvedt · 2022-11-11T20:39:11Z

Hi @twastvedt - maybe @saintentropy can confirm - I thought this issue only happened when instancing was true!?

OK, found a couple things.

Now remembering that Craig tried to tell me that I can just test the multiple texture maps part of this, as that doesn't depend on a feature flag. Doing that!
But, for reference, it looks like AggregateRenderPackages only modifies the render package if the geometry uses instancing but the flag is set to false (https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs#L1868-L1869). Otherwise I think it's just sending the render package with instancing embedded directly to Helix. I'm fuzzy on the details still though.

At any rate, I added a test which I verified fails before the PR, now passes. Still looking at the rendering edge cases.

src/VisualizationTests/HelixWatch3DViewModelTests.cs

twastvedt · 2022-11-15T21:28:57Z

@mjkkirschner I think this is ready to go? Just wanted to give you the chance to look at the test I added, if you want to. It's pretty straightforward though. I added an image comparison test which checks for the render bug Craig discovered in the process of this PR. I verified that it renders something different (the bug) before this PR's changes.

mjkkirschner · 2022-11-15T21:46:44Z

thanks @twastvedt - I will merge now, can you please cherry pick the squashed commit to 2.17?

#13437) * Leave RP data intact * Fix tests * Remove unnessary code * Spelling typos and clean up * clean up * clean up * Add regression test for render package mutation. * Unsubscribe from event. * Add image comparison test for render bug. * Fix test image size. Co-authored-by: Craig Long <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]> (cherry picked from commit 104ac05)

#13437) (#13519) * Leave RP data intact * Fix tests * Remove unnessary code * Spelling typos and clean up * clean up * clean up * Add regression test for render package mutation. * Unsubscribe from event. * Add image comparison test for render bug. * Fix test image size. Co-authored-by: Craig Long <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]> (cherry picked from commit 104ac05) Co-authored-by: Craig Long <[email protected]>

#13437) * Leave RP data intact * Fix tests * Remove unnessary code * Spelling typos and clean up * clean up * clean up * Add regression test for render package mutation. * Unsubscribe from event. * Add image comparison test for render bug. * Fix test image size. Co-authored-by: Craig Long <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]>

* Fixing horizontal center (#13312) * Fix Trusted Message behavior (#13355) * Fix Trusted Message behavior * Extract Business Logic as a function * Moving API to Preferences * Adding unit test * Leave RenderPackage data intact when passed to AggregateRenderPackages (#13437) * Leave RP data intact * Fix tests * Remove unnessary code * Spelling typos and clean up * clean up * clean up * Add regression test for render package mutation. * Unsubscribe from event. * Add image comparison test for render bug. * Fix test image size. Co-authored-by: Craig Long <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]> * DYN-5437 Export image button shortcut (#13550) * Insert the export command in the shortcut view model * remove test method * Dyn 5317 check webview2 installed (#13464) * DYN-5317-Check-WebView2-Installed I added a validation that before starting the DynamoSandbox app we will check if the WebView2 Runtime is installed or not, if is not installed we will show a MessageBox with the link of the WebView2 installer and then after pressing the OK button close Dynamo. Also in the DynamoMessageBox I added a RichTextBox that will be visible under very specific circumstances, this RichTextBox allows to have a blue hyperlink as a part of the text so the user can click it and open the webview2 installer webpage. * DYN-5317-Check-WebView2-Installed removing extra comment * DYN-5674-CustomNode-Renamed (#13854) After the auto-save functionality is triggered when we are located in the Custom Node edit tab when the node in the Home Workspace is being renamed to "backup". Then after my fix first is checking if it has a name and file associated (that usually happen when you create/edit a custom node) if that is the case then we don't execute the rename process. Also I added a test that will validate that the Save functionality doesn't rename the CustomNode name. * clean up --------- Co-authored-by: jesusalvino <[email protected]> Co-authored-by: Craig Long <[email protected]> Co-authored-by: Craig Long <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]> Co-authored-by: filipeotero <[email protected]> Co-authored-by: Roberto T <[email protected]>

Leave RP data intact

6d645e1

saintentropy added the WIP label Oct 26, 2022

Fix tests

70cd16a

mjkkirschner added this to the 2.17.0 milestone Oct 28, 2022

mjkkirschner self-requested a review October 29, 2022 02:58

mjkkirschner approved these changes Oct 29, 2022

View reviewed changes

saintentropy and others added 6 commits October 31, 2022 11:30

Merge branch 'master' into DYN-5329

0e21699

Remove unnessary code

00647c6

Spelling typos and clean up

e9bdba7

clean up

b22f78e

clean up

a50575b

Merge remote-tracking branch 'origin/master' into DYN-5329

ed24fd9

Add regression test for render package mutation.

6d83355

mjkkirschner reviewed Nov 11, 2022

View reviewed changes

src/VisualizationTests/HelixWatch3DViewModelTests.cs Outdated Show resolved Hide resolved

twastvedt added 3 commits November 14, 2022 09:28

Unsubscribe from event.

0094aab

Add image comparison test for render bug.

7358100

Fix test image size.

bbfd5ef

twastvedt removed the WIP label Nov 15, 2022

mjkkirschner merged commit 104ac05 into DynamoDS:master Nov 15, 2022

twastvedt mentioned this pull request Nov 15, 2022

Leave RenderPackage data intact when passed to AggregateRenderPackage… #13519

Merged

QilongTang mentioned this pull request Jul 12, 2023

Dynamo 2.16.2 Hotfix #14157

Merged

9 tasks

Conversation

saintentropy commented Oct 26, 2022 • edited by twastvedt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

mjkkirschner commented Oct 27, 2022

Uh oh!

mjkkirschner commented Oct 27, 2022

Uh oh!

mjkkirschner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjkkirschner Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

saintentropy Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saintentropy Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjkkirschner Oct 29, 2022

Choose a reason for hiding this comment

Uh oh!

saintentropy Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Oct 29, 2022

Choose a reason for hiding this comment

Uh oh!

saintentropy Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twastvedt commented Nov 10, 2022

Uh oh!

mjkkirschner commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twastvedt commented Nov 11, 2022

Uh oh!

Uh oh!

twastvedt commented Nov 15, 2022

Uh oh!

mjkkirschner commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

saintentropy commented Oct 26, 2022 •

edited by twastvedt

Loading

saintentropy Nov 3, 2022 •

edited

Loading

saintentropy Nov 3, 2022 •

edited

Loading

mjkkirschner commented Nov 10, 2022 •

edited

Loading