Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIKE] DYN-2473: Use light-weight geometry primitives lib for direct manipulation geometry #14973

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+354 −314
Open

[SPIKE] DYN-2473: Use light-weight geometry primitives lib for direct manipulation geometry #14973

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
282216b
port protogeometry types to geometryprimitives in dynamomanipulation …
aparajit-pratap Feb 20, 2024
25182a1
Merge branch 'master' of github.com:DynamoDS/Dynamo into dyn-2473
aparajit-pratap Feb 22, 2024
316b198
port Protogeometry usage to geometry primitives lib
aparajit-pratap Feb 26, 2024
8209717
cleanup
aparajit-pratap Feb 26, 2024
73786b0
update
aparajit-pratap Feb 26, 2024
8153ac9
review comments
aparajit-pratap Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/DynamoManipulation/DynamoManipulation.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
<ItemGroup>
<PackageReference Include="DynamoVisualProgramming.LibG_230_0_0" Version="3.0.0.4074" />
</ItemGroup>
<ItemGroup>
<Reference Include="$(PkgDynamoVisualProgramming_LibG_230_0_0)\tools\netstandard2.0\windows\LibG_230_0_0\Autodesk.GeometryPrimitives.Dynamo.dll">
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
</Reference>
</ItemGroup>
<ItemGroup>
<Compile Update="Properties\Resources.Designer.cs">
<AutoGen>True</AutoGen>
Expand Down
50 changes: 21 additions & 29 deletions src/DynamoManipulation/Gizmo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
using System;
using System;
using System.Windows;
using System.Windows.Media.Media3D;
using Dynamo.Visualization;
using Dynamo.Wpf.ViewModels.Watch3D;
using Point = Autodesk.DesignScript.Geometry.Point;
using Vector = Autodesk.DesignScript.Geometry.Vector;
using Point = Autodesk.GeometryPrimitives.Dynamo.Geometry.Point;
using Vector = Autodesk.GeometryPrimitives.Dynamo.Math.Vector3d;

namespace Dynamo.Manipulation
{
Expand Down Expand Up @@ -90,10 +90,7 @@ protected IRenderPackageFactory RenderPackageFactory
/// <summary>
/// Physical location of the manipulator lying on the geometry being manipulated
/// </summary>
protected Point ManipulatorOrigin
{
get { return manipulator.Origin; }
}
protected Point ManipulatorOrigin => manipulator.Origin;

private string name = "gizmo";
public string Name
Expand All @@ -117,29 +114,26 @@ protected Point Origin
{
get
{
if(origin != null) origin.Dispose();
var cameraPos = cameraPosition != null
? new Point(cameraPosition.Value.X, cameraPosition.Value.Y, cameraPosition.Value.Z)
: null;

using (var cameraPos = cameraPosition != null
? Point.ByCoordinates(cameraPosition.Value.X, cameraPosition.Value.Y, cameraPosition.Value.Z)
: null)
if (cameraPos == null)
{

if (cameraPos == null)
{
// cameraPos will be null if HelixWatch3DViewModel is not initialized
// this happens on an out of memory exception in SharpDX probably due to
// DynamoEffectsManager not being disposed off promptly.
// TODO: revisit to fix this properly later
// For the time being return a default position instead of throwing an exception
// to the effect that camerPos should not be null
return Point.ByCoordinates(ManipulatorOrigin.X, ManipulatorOrigin.Y, ManipulatorOrigin.Z);
}

using (var vec = Vector.ByTwoPoints(cameraPos, ManipulatorOrigin).Normalized())
{
origin = cameraPos.Add(vec.Scale(zDepth));
}
// cameraPos will be null if HelixWatch3DViewModel is not initialized
// this happens on an out of memory exception in SharpDX probably due to
// DynamoEffectsManager not being disposed off promptly.
// TODO: revisit to fix this properly later
// For the time being return a default position instead of throwing an exception
// to the effect that camerPos should not be null
return new Point(ManipulatorOrigin.Position.X, ManipulatorOrigin.Position.Y,
ManipulatorOrigin.Position.Z);
}

var vec = (ManipulatorOrigin.Position - cameraPos.Position).Unit;
vec.Scale(zDepth);
origin = new Point(cameraPos.Position + vec);

return origin;
}
}
Expand Down Expand Up @@ -212,8 +206,6 @@ public void Dispose()
{
Dispose(true);

if(origin != null) origin.Dispose();

BackgroundPreviewViewModel.ViewCameraChanged -= OnViewCameraChanged;
}
}
Expand Down
82 changes: 36 additions & 46 deletions src/DynamoManipulation/MousePointManipulator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
using System;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using Autodesk.DesignScript.Geometry;
using Dynamo.Graph.Nodes;
using Dynamo.Graph.Nodes.ZeroTouch;
using Point = Autodesk.DesignScript.Geometry.Point;
using Point = Autodesk.GeometryPrimitives.Dynamo.Geometry.Point;
using Vector = Autodesk.GeometryPrimitives.Dynamo.Math.Vector3d;
using Plane = Autodesk.GeometryPrimitives.Dynamo.Geometry.Plane;

namespace Dynamo.Manipulation
{
Expand All @@ -20,13 +21,14 @@ public INodeManipulator Create(NodeModel node, DynamoManipulationExtension manip
public class MousePointManipulator : NodeManipulator
{
private Point origin;
internal override Point Origin { get { return origin; } }
internal override Point Origin => origin;

private TranslationGizmo gizmo;

// Holds manipulator axis and input node pair for each input port.
// This collection is accessed from multiple threads
private ConcurrentDictionary<int, Tuple<Vector, NodeModel>> indexedAxisNodePairs = new ConcurrentDictionary<int, Tuple<Vector, NodeModel>>();
private readonly ConcurrentDictionary<int, Tuple<Vector, NodeModel>> indexedAxisNodePairs
= new ConcurrentDictionary<int, Tuple<Vector, NodeModel>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general which threads access this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while but as far as I remember this could be accessed by scheduler and UI threads.


internal MousePointManipulator(DSFunction node, DynamoManipulationExtension manipulatorContext)
: base(node, manipulatorContext)
Expand All @@ -38,15 +40,14 @@ internal MousePointManipulator(DSFunction node, DynamoManipulationExtension mani
protected override void AssignInputNodes()
{
//Default axes
var axes = new Vector[] { Vector.XAxis(), Vector.YAxis(), Vector.ZAxis() };
var axes = new Vector[] { Vector.XAxis, Vector.YAxis, Vector.ZAxis };

indexedAxisNodePairs.Clear();

for (int i = 0; i < 3; i++)
{
//First find out if the input can be manipulated.
NodeModel node = null;
if (!CanManipulateInputNode(i, out node))
if (!CanManipulateInputNode(i, out var node))
{
continue;
}
Expand All @@ -69,7 +70,7 @@ protected override void AssignInputNodes()
{
//Combine old axis with this axis
axis = item.Value.Item1;
axis = axis.Add(axes[i]);
axis = axis + axes[i];
idx = item.Key;
break;
}
Expand All @@ -87,10 +88,9 @@ protected override void AssignInputNodes()
// Normalize all axes in indexedAxisNodePairs
for (int i = 0; i < 3; i++)
{
Tuple<Vector, NodeModel> pair;
if (indexedAxisNodePairs.TryGetValue(i, out pair))
if (indexedAxisNodePairs.TryGetValue(i, out var pair))
{
indexedAxisNodePairs[i] = Tuple.Create(pair.Item1.Normalized(), pair.Item2);
indexedAxisNodePairs[i] = Tuple.Create(pair.Item1.Unit, pair.Item2);
}
}
}
Expand All @@ -109,7 +109,7 @@ protected override IEnumerable<IGizmo> GetGizmos(bool createOrUpdate)
yield break;

//No axis data, so no gizmo.
if (!indexedAxisNodePairs.Any())
if (indexedAxisNodePairs.IsEmpty)
yield break;

if (createOrUpdate)
Expand All @@ -134,11 +134,10 @@ protected override IEnumerable<NodeModel> OnGizmoClick(IGizmo gizmoInAction, obj
if(axis1 == null)
{
//Hit object is a plane, two axes will be updated simultaneously.
var plane = hitObject as Plane;
if(plane != null)
if(hitObject is Plane plane)
{
axis1 = plane.XAxis;
axis2 = plane.YAxis;
axis1 = plane.UAxis;
axis2 = plane.Normal * plane.UAxis;
}
}

Expand All @@ -149,11 +148,8 @@ protected override IEnumerable<NodeModel> OnGizmoClick(IGizmo gizmoInAction, obj
var node = item.Value.Item2;
if (v.Equals(axis1) || v.Equals(axis2))
{
if (node == null)
{
node = CreateAndConnectInputNode(0, item.Key);
}

node ??= CreateAndConnectInputNode(0, item.Key);

nodes.Add(item.Key, node);
}
}
Expand All @@ -178,9 +174,8 @@ protected override IEnumerable<NodeModel> OnGizmoClick(IGizmo gizmoInAction, obj
/// <param name="offset">Offset by which the gizmo has moved.</param>
protected override void OnGizmoMoved(IGizmo gizmoInAction, Vector offset)
{
var offsetPos = origin.Add(offset);
origin.Dispose();
origin = offsetPos;
var offsetPos = origin.Position + offset;
origin = new Point(offsetPos);
}

protected override List<(NodeModel inputNode, double amount)> InputNodesToUpdateAfterMove(Vector offset)
Expand All @@ -192,15 +187,13 @@ protected override void OnGizmoMoved(IGizmo gizmoInAction, Vector offset)

// When more than one input is connected to the same slider, this
// method will decompose the axis corresponding to each input.
using (var v = GetFirstAxisComponent(item.Value.Item1))
{
var amount = offset.Dot(v);
var v = GetFirstAxisComponent(item.Value.Item1);
var amount = offset % v;

aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
if (Math.Abs(amount) > MIN_OFFSET_VAL)
{
dynamic uiNode = item.Value.Item2;
inputNodes.Add((uiNode, uiNode.Value + amount));
}
if (Math.Abs(amount) > MIN_OFFSET_VAL)
{
dynamic uiNode = item.Value.Item2;
inputNodes.Add((uiNode, uiNode.Value + amount));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is existing code, but do we really need dynamic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was dynamic since it was originally intended to be applicable to different types of input nodes, not just sliders. In the next line, it uses the Value property, which isn't defined on the NodeModel class, but which is the common base class for all nodes (input nodes in this case).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it sounds like you don't need dynamic you need to use the common base class or an interface etc?

I think using the dlr has significant performance implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could introduce a new interface with a single Value member.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that @twastvedt wanted a similar interface for accessing input nodes in player without referencing the actual NodeModel types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aparajit-pratap - see https://jira.autodesk.com/browse/DYN-6114

}
}
return inputNodes;
Expand All @@ -213,18 +206,15 @@ protected override bool UpdatePosition()
{
if (Node == null || !indexedAxisNodePairs.Any()) return false;

if (origin == null)
{
origin = Point.Origin(); //First time initialization
}
origin ??= new Point(0, 0, 0);

//Node output could be a collection, consider the first item as origin.
Point pt = GetFirstValueFromNode(Node) as Point;
var pt = GetFirstValueFromNode(Node) as Autodesk.DesignScript.Geometry.Point;
if (null == pt) return false; //The node output is not Point, could be a function object.

//Don't cache pt directly here, we need to create a copy, because
//pt may be GC'ed by VM.
origin = Point.ByCoordinates(pt.X, pt.Y, pt.Z);
origin = new Point(pt.X, pt.Y, pt.Z);

return true;
}
Expand Down Expand Up @@ -266,15 +256,15 @@ private void UpdateGizmo()
private Vector GetFirstAxisComponent(Vector vector)
{
var tol = 0.0001;
var v1 = Vector.ByCoordinates(vector.X, 0, 0);
if (v1.Length > tol)
return v1.Normalized();
var v1 = new Vector(vector.X, 0, 0);
if (v1.Magnitude > tol)
return v1.Unit;

var v2 = Vector.ByCoordinates(0, vector.Y, 0);
if (v2.Length > tol)
return v2.Normalized();
var v2 = new Vector(0, vector.Y, 0);
if (v2.Magnitude > tol)
return v2.Unit;

return vector.Normalized();
return vector.Unit;
}

#endregion
Expand Down
Loading
Loading