DYN-6489 Dynamo Zoom Extents behavior has changed #14674

saintentropy · 2023-12-02T22:03:52Z

Purpose

Helix-Toolkit made an adjustment in the ZoomExtents() algorithm in the 2.24.0 Release. Specifically, the change was introduced here -> helix-toolkit/helix-toolkit#1921 and was included in the 2.24.0 release. Previously the ZoomExtents() algorithm was never very accurate or predictable in that results would vary depending on the size and height/width ratio of background preview view window. This change however makes the calculated ZoomExtents appear fully incorrect for some cases. Subsequent to this code change, the Dev branch in helix-toolkit now has a dramatically improved algorithm for ZoomExtents. The PR is found here -> helix-toolkit/helix-toolkit#2003. I assume this new algorithm will be available in the 2.25.0 release and is testable currently with the helix-toolkit daily builds. This PR pulls the relevant implementation into Dynamo for use until the release of 2.25.0 when we can presumably call the updated methods directly. In testing, ZoomExtents is now fully predictable regardless of background preview view window size or height/width ratio.

This PR also takes the opportunity to utilize the native helix-toolkit bounding box calculation for scene objects. This was not available 8 years ago when we added our own implementation. By using the native methods, we also solve the issue where instance geometry was not correctly considered for ZoomExtents calculations.

Also for some reason we have always been clipping the top 20 pixels off of the background preview 🤷. Now you notice it so I removed that margin.

Improved Result

Previous Behavior

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
This PR contains no files larger than 50 MB

Release Notes

Zoom Extents for the background preview is now more accurate and predictable regardless of the size of the Dynamo application window.

Reviewers

@aparajit-pratap @mjkkirschner

FYIs

@jnealb @twastvedt

jnealb · 2023-12-02T22:16:34Z

+1

saintentropy · 2023-12-02T23:24:39Z

Old clipping

vs New

src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs

src/DynamoCoreWpf/Views/Preview/Watch3DView.xaml.cs

mjkkirschner

A couple questions, but it looks good otherwise, I think we should add an image comparison test if one doesn't exist already.

We likely also need a followup if one already exists to see if we can make the system more robust.

I think whoever takes the current task can add that test or file that followup task.

mjkkirschner · 2023-12-11T23:17:15Z

@saintentropy I've pushed to this branch and will merge / cherry pick after the tests pass.

github-actions · 2023-12-11T23:34:46Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-11T23:37:42Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

aparajit-pratap · 2023-12-12T15:54:28Z

src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs


-            if (geom.Geometry.Positions.Count > 1)
+            if(Math.Abs(bounds.Size.LengthSquared()) < defaultBoundsSize * defaultBoundsSize * 3)


A square of any number will always be greater than or equal to zero, why do we need Math.Abs?

Can you explain this formula, where do you get 3 from?

If I remember @saintentropy 's explanation correctly, he was just trying to set a good minimum zoom size. If we go too small the UI becomes difficult to navigate/pan etc. This value is larger than what we had before but it seems to work well in practice even for just a point.

I don't see a reason for the abs call either. I can remove it.

aparajit-pratap · 2023-12-12T16:28:32Z

src/DynamoCoreWpf/Views/Preview/Watch3DView.xaml.cs

-            if(watch_view.Camera.Position.ToVector3().IsUndefined() || 
-                watch_view.Camera.LookDirection.ToVector3().IsUndefined() || 
-                watch_view.Camera.LookDirection.Length == 0)
+            if (watch_view.Camera is HelixToolkit.Wpf.SharpDX.PerspectiveCamera perspectiveCam)


Do we need to account for the case where the camera is Orthographic if at all possible?

hmm, let me look into if that was supported before.

yeah, your're right @aparajit-pratap that would have been supported before, I'll move in the code for ortho cameras as well from helix.

Wait, how do we change the camera from perspective to orthographic? Is there a UI option?

it can be done from a view extension - theres a property on the helix viewport object

github-actions · 2023-12-12T20:06:15Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-12T20:06:15Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

github-actions · 2023-12-12T20:13:35Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2023-12-12T20:22:53Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net6.0

* Adopt updated helix ZoomExtents algorithm * use Helix native bounds implementation * Why was this ever this way * changes made * handle zoom for ortho * review comments --------- Co-authored-by: Craig Long <[email protected]> Co-authored-by: mjk.kirschner <[email protected]> (cherry picked from commit 6d50740)

saintentropy added 2 commits December 2, 2023 16:37

Adopt updated helix ZoomExtents algorithm

6b840d9

use Helix native bounds implementation

6b748f0

jnealb closed this Dec 2, 2023

Why was this ever this way

4e0871e

saintentropy reopened this Dec 2, 2023

mjkkirschner reviewed Dec 5, 2023

View reviewed changes

src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 5, 2023

View reviewed changes

src/DynamoCoreWpf/Views/Preview/Watch3DView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 5, 2023

View reviewed changes

src/DynamoCoreWpf/Views/Preview/Watch3DView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 5, 2023

View reviewed changes

src/DynamoCoreWpf/Views/Preview/Watch3DView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner approved these changes Dec 5, 2023

View reviewed changes

mjkkirschner added 2 commits December 11, 2023 18:03

changes made

1203d10

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into zoom

c884bf4

aparajit-pratap reviewed Dec 12, 2023

View reviewed changes

mjkkirschner added 3 commits December 12, 2023 13:47

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into zoom

ab9eebe

handle zoom for ortho

034b760

review comments

222739d

aparajit-pratap approved these changes Dec 13, 2023

View reviewed changes

mjkkirschner merged commit 6d50740 into DynamoDS:master Dec 13, 2023
22 checks passed

saintentropy mentioned this pull request Dec 14, 2023

Cherry Pick CP DYN-6489 Dynamo Zoom Extents behavior has changed (#14674) #14757

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6489 Dynamo Zoom Extents behavior has changed #14674

DYN-6489 Dynamo Zoom Extents behavior has changed #14674

saintentropy commented Dec 2, 2023 •

edited

Loading

jnealb commented Dec 2, 2023

saintentropy commented Dec 2, 2023

mjkkirschner left a comment

mjkkirschner commented Dec 11, 2023

github-actions bot commented Dec 11, 2023

github-actions bot commented Dec 11, 2023

aparajit-pratap Dec 12, 2023 •

edited

Loading

aparajit-pratap Dec 12, 2023

mjkkirschner Dec 12, 2023 •

edited

Loading

aparajit-pratap Dec 12, 2023 •

edited

Loading

mjkkirschner Dec 12, 2023 •

edited

Loading

mjkkirschner Dec 12, 2023

mjkkirschner Dec 12, 2023

aparajit-pratap Dec 13, 2023

mjkkirschner Dec 13, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023

github-actions bot commented Dec 12, 2023

github-actions bot commented Dec 12, 2023

github-actions bot commented Dec 12, 2023


		if (geom.Geometry.Positions.Count > 1)
		if(Math.Abs(bounds.Size.LengthSquared()) < defaultBoundsSize * defaultBoundsSize * 3)

DYN-6489 Dynamo Zoom Extents behavior has changed #14674

DYN-6489 Dynamo Zoom Extents behavior has changed #14674

Conversation

saintentropy commented Dec 2, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

jnealb commented Dec 2, 2023

saintentropy commented Dec 2, 2023

mjkkirschner left a comment

Choose a reason for hiding this comment

mjkkirschner commented Dec 11, 2023

github-actions bot commented Dec 11, 2023

UI Smoke Tests

github-actions bot commented Dec 11, 2023

UI Smoke Tests

aparajit-pratap Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Dec 12, 2023

Choose a reason for hiding this comment

mjkkirschner Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Dec 12, 2023

Choose a reason for hiding this comment

mjkkirschner Dec 12, 2023

Choose a reason for hiding this comment

aparajit-pratap Dec 13, 2023

Choose a reason for hiding this comment

mjkkirschner Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2023

UI Smoke Tests

github-actions bot commented Dec 12, 2023

UI Smoke Tests

github-actions bot commented Dec 12, 2023

UI Smoke Tests

github-actions bot commented Dec 12, 2023

UI Smoke Tests

saintentropy commented Dec 2, 2023 •

edited

Loading

aparajit-pratap Dec 12, 2023 •

edited

Loading

mjkkirschner Dec 12, 2023 •

edited

Loading

aparajit-pratap Dec 12, 2023 •

edited

Loading

mjkkirschner Dec 12, 2023 •

edited

Loading

mjkkirschner Dec 13, 2023 •

edited

Loading