Toggling geometry edge preview setting should not require re-execution of the graph

[alfarok]

Purpose

DYN-1025

The PR ensures RegenerateAllPackages is only called on HelixWatch3DViewModel or a view model that inherits from HelixWatch3DViewModel, thus preventing RegenerateAllPackages from being called on DefaultWatch3DViewModel. This doesn't seem to cause any disturbance to the behaviour of the RevitBackgroundPreview which implements DefaultWatch3DViewModel.

RegenerateAllPackages is only called in 2 places, when toggling edge visibility and when toggling the active state of the specified view. This change should result in the same current behavior for DefaultWatch3DViewModel which will require re-execuction of the graph to generate edges the render packages.

Concerns

When looking into existing unit tests that appeared to be covering this functionality I noticed none of the WpfVisualizationTests are running on the self-serve CI or during the complete install tests. Running them locally 50 of the 75 are failing with several exceptions I have outlined in DYN-1462. This being said it makes it difficult to determine if any regressions are occuring.

I don't think this is the best fix as more investigation is required into the deeper issues outlined in the spike above. For example, if several places we are attempting to parse DefaultWatch3DViewModel to HelixWatch3DViewModel which should never work given the inheritance chain. This bug has been in existence since Dynamo 1.0 (2015) it's hard to determine when this behavior was originally introduced or if it ever worked after the RevitBackgroundPreview was introduced. See 5431 for more info.

Background

Before

After

Declarations

Check these if you believe they are true

• The code base 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.

Reviewers

@mjkkirschner @QilongTang @aparajit-pratap

FYIs

@Racel

[Racel]

LGTM. But @mjkkirschner and @alfarok what do you think the risk is of this causing other regressions is since a bunch of tests are failing? Would you recommend putting this into the 2.1 RC?

[mjkkirschner]

I'm not sure if this fix should be merged. I don't understand why regenerating default packages would cause an exception to be thrown.

I feel thats what should be addressed /investigated here - @alfarok did you ever determine the exact cause of the exception?

DefaultRenderPackages should still be generated / regenerated on platforms where helix doesn't exist, suppose we are running dynamo core on mac and using default render packages to hold tessellated data before passing it to some rendering extension (like yours) - I would still want regneration to be called when the view was toggled on and off.

I think the change in showEdges is okay because it's isolated - but the Active toggle is worrisome to me because it effects all DefaultRenderPackages as far as I can tell.

[alfarok]

@Racel @mjkkirschner I don't think this should be merged either. This task was originally story pointed a 2 because we were under the impression it was probably simple fix. I have already exceeded this and haven't been able to get to the source. It appears to me that it's more complicated and the failing tests should be investigated/resolved before making any WPF visualization changes to monitor regressions. I can add my notes and add the task back to the backlog which can be revisited when the tests are resolved.

[aparajit-pratap]

In such cases where you have derived classes needing to override behavior in the base class, it's better design to override the method/property in the derived class and let polymorphism do its job. So in this case it would be overriding Active in HelixWatch3DViewModel.

[alfarok]

@mjkkirschner Do we have any examples of scenarios in which nodes have multiple render packages per node? Revit is the only case I can think of that does this today because there are multiple 3DViewModels. Calling regenerate calls node.RequestVisualUpdateAsync so for every node this gets called twice. I need to double check if this method updates all the render packages at once. If this is the case this operation is redundant and make sense why everything continues to work with this fix, however if the only render package is a DefaultRenderPackage this will break down (such as while using the CLI). It is also possible that this scenario is already not working and requires the graph to be re-executed but no one has encountered it. Maybe the black team while originally working with the CLI?

I am having difficulty understanding if this ever worked properly. If you check out the PR from Ian in 2015 HelixWatch3DViewmodel and DefaultWatch3DViewModel were pretty much overhauled. Which is around the same time the RevitBackgroundPreview was introduced and the last time the edges toggle was reported to be working properly.

HelixWatch3DViewmodel : DefaultWatch3DViewModel : NotificationObject, IWatch3DViewModel, IDisposable
DefaultRenderPackage : IRenderPackage
HelixRenderPackage : IRenderPackage, ITransformable

Several places in the code we try to dynamically cast a DefaultRenderPackage to HelixRenderPackage which given the inheritance chain above should not work.