DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state) #12150

saintentropy · 2021-10-15T20:08:07Z

Purpose

This PR covers https://jira.autodesk.com/browse/DYN-4224 and https://jira.autodesk.com/browse/DYN-4225

This PR updates the behavior of the Watch3d node in three ways.

Fix regression from 2.0 where the camera data that is saved to the DYN file is not used to set the nodes camera view on deserialization. Previously it would always reset to default. Now it will set the camera to the view that was saved to the file.
Fix regression from 2.0 where the node canvas size that is saved to the DYN file is not used to set the watch node size on deserialization. Previously it would always reset to the default 200 x 200. Now it will restore the watch size on canvas to the size that was saved to file.
Change the behavior for Watch3D to always show geometry from connected node regardless of the nodes preview state. In the past the Watch3D node had functionality that gave the user more control over what geometry was displayed within the node (ie visible upstream). Currently the node is only limited to showing a subset of geometry that is visible in the Background Preview. This limits the user ability to visualize subsets of geometry independently. This change will allow the user to visualize geometry in Watch3d nodes independent of the preview state set on the 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

Reviewers

TBD

FYIs

…review state

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

src/Libraries/Watch3DNodeModels/Watch3D.cs

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

src/DynamoCore/Scheduler/UpdateRenderPackageAsyncTask.cs

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

mjkkirschner · 2022-11-03T18:25:17Z

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

+                var connected = watchModel.InPorts.SelectMany(p => p.Connectors.Select(c => c.Start.Owner));
+                if (!connected.Contains(node))
+                {
+                    return;


when would this occur?

The DefaultWatch is wired up to every node.PropertyChanged event in the graph. In the case of the background preview you want all of them. For the Watch3D we want to only react to changes that are for the node that is directly connected

mjkkirschner · 2022-11-03T18:26:06Z

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

        protected override void PortDisconnectedHandler(PortModel obj)
        {
-            OnClear();
+            if (obj.PortType == PortType.Input  && watchModel == obj.Owner)


when could it occur that obj is not a portModel of this node?

In this case the PortConnected and PortDisconnected Handler are registered in the DefaultWatchViewModel as well. So we need to filter out port events unrelated to this node.

mjkkirschner · 2022-11-03T18:30:10Z

@saintentropy this PR needs tests.

mjkkirschner

a few questions and a request for at least the following tests:

calling RequestVisualUpdateAsync with ignoreIsVisible works correctly with true and with false inputs
deserialization test for watch3d node camera sets some properties correctly.

mjkkirschner · 2022-11-07T22:50:51Z

src/DynamoCoreWpf/ViewModels/Watch3D/DefaultWatch3DViewModel.cs

@@ -610,7 +611,8 @@ protected virtual void OnRenderPackagesUpdated(NodeModel node, RenderPackageCach
            // If there is no attached model update for all render packages
            if (watchModel == null)
            {
-                if(node.IsVisible == false)
+                //When Watch3D nodes are in canvis we need to ignore this even under certain situations. 


spelling (canvas) - also comment is not super clear to me?
ignore this even under certain situations
whats this,
what situations?

@mjkkirschner Does this make more sence now?

src/DynamoCore/Graph/Nodes/NodeModelExtensions.cs

…Watch3dUpdates

twastvedt · 2022-11-16T20:49:20Z

@mjkkirschner I believe this is ready to go, but have a look at the added tests when you get a chance.

twastvedt · 2022-11-16T20:51:12Z

Also, can we still cherry-pick this into 2.17? I notice the PR isn't marked 2.17, though the corresponding Jira task is.

mjkkirschner · 2022-11-16T20:52:56Z

@twastvedt yes, please cherry pick the squash commit.

mjkkirschner · 2022-11-16T21:32:21Z

@twastvedt while this looks good, there are 2 failing tests, please check them out before merging!
⚠️
WpfVisualizationTests.HelixImageComparisonTests.HelixWatch3DViewModelTests.Node_IgnoreIsVisible
WpfVisualizationTests.HelixWatch3DViewModelTests.Node_IgnoreIsVisible

I actually think this is 1 test just listed incorrectly as 2 by jenkins.

twastvedt · 2022-11-17T15:03:20Z

@twastvedt while this looks good, there are 2 failing tests, please check them out before merging! ⚠️ WpfVisualizationTests.HelixImageComparisonTests.HelixWatch3DViewModelTests.Node_IgnoreIsVisible WpfVisualizationTests.HelixWatch3DViewModelTests.Node_IgnoreIsVisible

I actually think this is 1 test just listed incorrectly as 2 by jenkins.

Ah, sorry about that. Ok, passing now!

…between saves / Ignore Node preview state) (#12150) * Use exisitng width, height, and camera during deserialization * Allow Watch3D to tessellate directly connected nodes independent of preview state * One more change for directly connected * Fix bug with port connection and disconnect events * pr comments * Ignore certain cases * PR comments * update comments * Update DefaultWatch3DViewModel.cs * Added deserialization test for HelixWatch3D node * Added the dyn file for the HelixWatch3D node deserialization test * Added testing for Width deserialization * Renamed test dyn file * Added test for ignoring IsVisible parameter by Watch3d node * Fixed wrong test geometry in dyn file * Added another Assert to the IgnoreIsVisible test * Fixed Assert statements * Simplified a test * Remove node_modules. * Fix test. * Simplify Co-authored-by: Craig Long <[email protected]> Co-authored-by: Long Nguyen <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]> (cherry picked from commit 903ca10)

…between saves / Ignore Node preview state) (#12150) (#13534) * Use exisitng width, height, and camera during deserialization * Allow Watch3D to tessellate directly connected nodes independent of preview state * One more change for directly connected * Fix bug with port connection and disconnect events * pr comments * Ignore certain cases * PR comments * update comments * Update DefaultWatch3DViewModel.cs * Added deserialization test for HelixWatch3D node * Added the dyn file for the HelixWatch3D node deserialization test * Added testing for Width deserialization * Renamed test dyn file * Added test for ignoring IsVisible parameter by Watch3d node * Fixed wrong test geometry in dyn file * Added another Assert to the IgnoreIsVisible test * Fixed Assert statements * Simplified a test * Remove node_modules. * Fix test. * Simplify Co-authored-by: Craig Long <[email protected]> Co-authored-by: Long Nguyen <[email protected]> Co-authored-by: Trygve Wastvedt <[email protected]> (cherry picked from commit 903ca10) Co-authored-by: Craig Long <[email protected]>

saintentropy added 5 commits August 1, 2021 21:56

Use exisitng width, height, and camera during deserialization

17bc6dc

Allow Watch3D to tessellate directly connected nodes independent of p…

c497756

…review state

One more change for directly connected

0583c85

Fix bug with port connection and disconnect events

d81582e

Merge remote-tracking branch 'upstream/master' into Watch3dUpdates

c564327

saintentropy changed the title ~~Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state)~~ DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state) Oct 18, 2021

mjkkirschner reviewed Oct 22, 2021

View reviewed changes

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Oct 22, 2021

View reviewed changes

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs Show resolved Hide resolved

mjkkirschner reviewed Oct 22, 2021

View reviewed changes

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Oct 22, 2021

View reviewed changes

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs Outdated Show resolved Hide resolved

pr comments

b073fc3

LongNguyenP reviewed Apr 5, 2022

View reviewed changes

src/Libraries/Watch3DNodeModels/Watch3D.cs Show resolved Hide resolved

mjkkirschner reviewed Nov 3, 2022

View reviewed changes

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Nov 3, 2022

View reviewed changes

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

mjkkirschner reviewed Nov 3, 2022

View reviewed changes

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Nov 3, 2022

View reviewed changes

mjkkirschner requested changes Nov 3, 2022

View reviewed changes

saintentropy added 4 commits November 4, 2022 08:04

Merge remote-tracking branch 'upstream/master' into Watch3dUpdates

f63a78f

Ignore certain cases

c1c9b17

PR comments

a6244c3

update comments

1d0c23a

mjkkirschner reviewed Nov 7, 2022

View reviewed changes

src/DynamoCore/Graph/Nodes/NodeModelExtensions.cs Show resolved Hide resolved

saintentropy and others added 5 commits November 9, 2022 08:59

Update DefaultWatch3DViewModel.cs

749beb7

Added deserialization test for HelixWatch3D node

71a4c28

Added the dyn file for the HelixWatch3D node deserialization test

03ceb2e

Added testing for Width deserialization

372284d

Renamed test dyn file

e8a003b

LongNguyenP and others added 8 commits November 14, 2022 20:13

Added test for ignoring IsVisible parameter by Watch3d node

77de095

Fixed wrong test geometry in dyn file

4165db0

Added another Assert to the IgnoreIsVisible test

5d64c4e

Fixed Assert statements

37f711c

Merge branch 'master' of https://github.com/saintentropy/Dynamo into …

ffc8edf

…Watch3dUpdates

Simplified a test

c61d09b

Merge branch 'master' into Watch3dUpdates

019e541

Remove node_modules.

256c1a9

twastvedt requested a review from mjkkirschner November 16, 2022 20:46

mjkkirschner added this to the 2.17.0 milestone Nov 16, 2022

mjkkirschner approved these changes Nov 16, 2022

View reviewed changes

twastvedt added 2 commits November 16, 2022 15:55

Fix test.

afc68a0

Simplify

f1ede34

twastvedt merged commit 903ca10 into DynamoDS:master Nov 17, 2022

twastvedt mentioned this pull request Nov 17, 2022

DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size … #13534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state) #12150

DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state) #12150

saintentropy commented Oct 15, 2021

mjkkirschner Nov 3, 2022

saintentropy Nov 4, 2022

mjkkirschner Nov 3, 2022

saintentropy Nov 4, 2022

mjkkirschner commented Nov 3, 2022

mjkkirschner left a comment

mjkkirschner Nov 7, 2022

saintentropy Nov 9, 2022

twastvedt commented Nov 16, 2022

twastvedt commented Nov 16, 2022

mjkkirschner commented Nov 16, 2022

mjkkirschner commented Nov 16, 2022 •

edited

Loading

twastvedt commented Nov 17, 2022

DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state) #12150

DYN-4224 / DYN-4225 Watch3d Updates (Persistent camera and node size between saves / Ignore Node preview state) #12150

Conversation

saintentropy commented Oct 15, 2021

Purpose

Declarations

Reviewers

FYIs

mjkkirschner Nov 3, 2022

Choose a reason for hiding this comment

saintentropy Nov 4, 2022

Choose a reason for hiding this comment

mjkkirschner Nov 3, 2022

Choose a reason for hiding this comment

saintentropy Nov 4, 2022

Choose a reason for hiding this comment

mjkkirschner commented Nov 3, 2022

mjkkirschner left a comment

Choose a reason for hiding this comment

mjkkirschner Nov 7, 2022

Choose a reason for hiding this comment

saintentropy Nov 9, 2022

Choose a reason for hiding this comment

twastvedt commented Nov 16, 2022

twastvedt commented Nov 16, 2022

mjkkirschner commented Nov 16, 2022

mjkkirschner commented Nov 16, 2022 • edited Loading

twastvedt commented Nov 17, 2022

mjkkirschner commented Nov 16, 2022 •

edited

Loading