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.

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

ConversionResult in HostObjectBuilder #3437

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
34 changes: 34 additions & 0 deletions Core/Core/Models/ConversionResult.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using System;

namespace Speckle.Core.Models;

public sealed class ConversionResult
{
public string? ResultId { get; }
public object? Result { get; }
public Exception? Error { get; }
public object Target { get; }

//[MemberNotNullWhen(true, nameof(Result))]
//[MemberNotNullWhen(true, nameof(ResultId))]
//[MemberNotNullWhen(false, nameof(Error))]
public bool IsSuccessful => Result is not null;

public ConversionResult(object target, object result, string resultId)
{
Target = target;
Result = result;
Copy link
Member

Choose a reason for hiding this comment

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

This object might know whether the conversion is successful with fallback displayValue or not. I mean, it is "successful" with "but". TBD

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially simply the result type will be enough for us to communicate this if we wish.
"Brep" was converted as "Mesh" communicates exactly the same thing as "with dispalyValues", but imo will be more intuitive to the user.

If we wanted more than this, I could see some utility in knowing what converter was used to convert the object.
But this would require some more restructuring of our "...WithFallBack" service, because right now, the converter is abstracted and the connector doesn't see it.

I think we could do something nice, where the "...WithFallBack" simply returns the converter (without calling it), instead of right now, where it both finds and immediately calls it

ResultId = resultId;
}

public ConversionResult(object target, Exception error)
{
Target = target;
Error = error;
}

public override string ToString() =>
IsSuccessful
? $"Successfully converted {Target} to {Result}"
: $"Failed to convert {Target}: {Error!.GetType()}: {Error!.Message}";
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,17 @@
using Speckle.Connectors.DUI.Models;
using Speckle.Connectors.Utils.Cancellation;
using Speckle.Connectors.DUI.Models.Card;
using Speckle.Connectors.Utils;
using Speckle.Connectors.Utils.Operations;
using Speckle.Converters.Common;
using Speckle.Core.Logging;
using Speckle.Core.Models;
using ICancelable = System.Reactive.Disposables.ICancelable;

namespace Speckle.Connectors.Autocad.Bindings;

public sealed class AutocadReceiveBinding : IReceiveBinding, ICancelable
{
public string Name { get; } = "receiveBinding";
public string Name => "receiveBinding";
public IBridge Parent { get; }

private readonly DocumentModelStore _store;
Expand Down Expand Up @@ -53,7 +54,7 @@ public async Task Receive(string modelCardId)
}

// Receive host objects
IEnumerable<string> receivedObjectIds = await unitOfWork.Service
IReadOnlyList<ConversionResult> conversionResults = await unitOfWork.Service
.Execute(
modelCard.AccountId.NotNull(), // POC: I hear -you are saying why we're passing them separately. Not sure pass the DUI3-> Connectors.DUI project dependency to the SDK-> Connector.Utils
modelCard.ProjectId.NotNull(),
Expand All @@ -65,7 +66,7 @@ public async Task Receive(string modelCardId)
)
.ConfigureAwait(false);

Commands.SetModelReceiveResult(modelCardId, receivedObjectIds.ToList());
Commands.SetModelReceiveResult(modelCardId, conversionResults);
}
catch (Exception e) when (!e.IsFatal()) // All exceptions should be handled here if possible, otherwise we enter "crashing the host app" territory.
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public static class EntityExtensions
/// <param name="entity">Entity to add into database.</param>
/// <param name="layer"> Layer to append object.</param>
/// <exception cref="InvalidOperationException">Throws when there is no top transaction in the document.</exception>
public static ObjectId Append(this Entity entity, string? layer = null)
public static ObjectId AppendToDb(this Entity entity, string? layer = null)
{
// POC: Will be addressed to move it into AutocadContext!
var db = entity.Database ?? Application.DocumentManager.MdiActiveDocument.Database;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
using System.Diagnostics;
using Autodesk.AutoCAD.DatabaseServices;
using Speckle.Connectors.Autocad.HostApp;
using Speckle.Connectors.Autocad.HostApp.Extensions;
Expand All @@ -12,22 +11,22 @@ namespace Speckle.Connectors.Autocad.Operations.Receive;

public class AutocadHostObjectBuilder : IHostObjectBuilder
{
private readonly IRootToHostConverter _converter;
private readonly AutocadLayerManager _autocadLayerManager;
private readonly IRootToHostConverter _converter;
private readonly GraphTraversal _traversalFunction;

public AutocadHostObjectBuilder(
IRootToHostConverter converter,
AutocadLayerManager autocadLayerManager,
GraphTraversal traversalFunction
GraphTraversal traversalFunction,
AutocadLayerManager autocadLayerManager
)
{
_converter = converter;
_autocadLayerManager = autocadLayerManager;
_traversalFunction = traversalFunction;
_autocadLayerManager = autocadLayerManager;
}

public IEnumerable<string> Build(
public IReadOnlyList<ConversionResult> Build(
Base rootObject,
string projectName,
string modelName,
Expand All @@ -40,64 +39,65 @@ CancellationToken cancellationToken

// Layer filter for received commit with project and model name
_autocadLayerManager.CreateLayerFilter(projectName, modelName);
var traversalGraph = _traversalFunction.Traverse(rootObject).ToArray();

//TODO: make the layerManager handle \/ ?
string baseLayerPrefix = $"SPK-{projectName}-{modelName}-";

HashSet<string> uniqueLayerNames = new();
List<string> handleValues = new();
int count = 0;

// POC: Will be addressed to move it into AutocadContext!
using (TransactionContext.StartTransaction(Application.DocumentManager.MdiActiveDocument))
List<ConversionResult> results = new();
foreach (var tc in _traversalFunction.TraverseWithProgress(rootObject, onOperationProgressed, cancellationToken))
{
foreach (TraversalContext tc in traversalGraph)
try
{
var convertedObjects = ConvertObject(tc, baseLayerPrefix, uniqueLayerNames);

results.AddRange(convertedObjects.Select(e => new ConversionResult(tc, e, e.Handle.Value.ToString())));
}
catch (Exception ex) when (!ex.IsFatal())
Copy link
Member

Choose a reason for hiding this comment

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

hmm, just have general Qs:

  • Will we continue conversion if some bad error happens that might affect the state of the app?
  • Will we have a revert operation mechanism if smth bad happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, I've gone for "continue conversion of the next object" for all non-fatal exceptions.
This was the behaviour of DUI2 and was (I seem to remember) agreed with Dim.

I don't think we ever saw any problems with this approach.

Although, when we do logging, we will be logging "expected" exceptions (like SpeckleException subtypes) differently from unexpected (e.g. nullreference)

We do have the flexibility here to pick and choose if we wanted to pivot to something else.
But I think this will strike a good balance for now.

We'll take a look at transactions/reverting for cancellation, probably in a separate PR.

{
cancellationToken.ThrowIfCancellationRequested();

try
{
string layerFullName = GetLayerPath(tc, baseLayerPrefix);

if (uniqueLayerNames.Add(layerFullName))
{
_autocadLayerManager.CreateLayerOrPurge(layerFullName);
}

//POC: this transaction used to be called in the converter, We've moved it here to unify converter implementation
//POC: Is this transaction 100% needed? we are already inside a transaction?
object converted;
using (var tr = Application.DocumentManager.CurrentDocument.Database.TransactionManager.StartTransaction())
{
converted = _converter.Convert(tc.Current);
tr.Commit();
}

List<object> flattened = Utilities.FlattenToHostConversionResult(converted);

foreach (Entity conversionResult in flattened.Cast<Entity>())
{
if (conversionResult == null)
{
// POC: This needed to be double checked why we check null and continue
continue;
}

conversionResult.Append(layerFullName);

handleValues.Add(conversionResult.Handle.Value.ToString());
}

onOperationProgressed?.Invoke("Converting", (double)++count / traversalGraph.Length);
}
catch (Exception e) when (!e.IsFatal()) // DO NOT CATCH SPECIFIC STUFF, conversion errors should be recoverable
{
// POC: report, etc.
Debug.WriteLine("conversion error happened.");
}
results.Add(new(tc, ex));
}
}
return handleValues;

return results;
}

private IEnumerable<Entity> ConvertObject(TraversalContext tc, string baseLayerPrefix, ISet<string> uniqueLayerNames)
{
using TransactionContext transactionContext = TransactionContext.StartTransaction(
Application.DocumentManager.MdiActiveDocument
);

string layerFullName = GetLayerPath(tc, baseLayerPrefix);

if (uniqueLayerNames.Add(layerFullName))
{
_autocadLayerManager.CreateLayerOrPurge(layerFullName);
}

//POC: this transaction used to be called in the converter, We've moved it here to unify converter implementation
//POC: Is this transaction 100% needed? we are already inside a transaction?
object converted;
using (var tr = Application.DocumentManager.CurrentDocument.Database.TransactionManager.StartTransaction())
{
converted = _converter.Convert(tc.Current);
tr.Commit();
}

IEnumerable<Entity?> flattened = Utilities.FlattenToHostConversionResult(converted).Cast<Entity>();

foreach (Entity? conversionResult in flattened)
{
if (conversionResult == null)
{
// POC: This needed to be double checked why we check null and continue
continue;
}

conversionResult.AppendToDb(layerFullName);

yield return conversionResult;
}
}

private string GetLayerPath(TraversalContext context, string baseLayerPrefix)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Speckle.Connectors.Utils.Cancellation;
using Speckle.Connectors.Utils.Operations;
using Speckle.Core.Logging;
using Speckle.Core.Models;

namespace Speckle.Connectors.Rhino7.Bindings;

Expand Down Expand Up @@ -51,7 +52,7 @@ public async Task Receive(string modelCardId)
}

// Receive host objects
IEnumerable<string> receivedObjectIds = await unitOfWork.Service
IReadOnlyList<ConversionResult> conversionResults = await unitOfWork.Service
Copy link
Member

Choose a reason for hiding this comment

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

Probably, we will have a different structure on send and receive since we also return ObjectReferences on send, so maybe it would be nice to name class explicitly as ReceiveConversionResult

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look at Receive today, if a new class makes sense, lets do it.

.Execute(
modelCard.AccountId.NotNull(), // POC: I hear -you are saying why we're passing them separately. Not sure pass the DUI3-> Connectors.DUI project dependency to the SDK-> Connector.Utils
modelCard.ProjectId.NotNull(),
Expand All @@ -63,8 +64,7 @@ public async Task Receive(string modelCardId)
)
.ConfigureAwait(false);

// POC: Here we can't set receive result if ReceiveOperation throws an error.
Commands.SetModelReceiveResult(modelCardId, receivedObjectIds.ToList());
Commands.SetModelReceiveResult(modelCardId, conversionResults);
Copy link
Member

Choose a reason for hiding this comment

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

This is where I can start to think interop

}
catch (Exception e) when (!e.IsFatal()) // All exceptions should be handled here if possible, otherwise we enter "crashing the host app" territory.
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ GraphTraversal traverseFunction
_traverseFunction = traverseFunction;
}

public IEnumerable<string> Build(
public IReadOnlyList<ConversionResult> Build(
Base rootObject,
string projectName,
string modelName,
Expand All @@ -38,25 +38,18 @@ CancellationToken cancellationToken
var baseLayerName = $"Project {projectName}: Model {modelName}";

var objectsToConvert = _traverseFunction
.Traverse(rootObject)
.Where(obj => obj.Current is not Collection)
.Select(ctx => (GetLayerPath(ctx), ctx.Current))
.ToArray();
.TraverseWithProgress(rootObject, onOperationProgressed, cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

Do we report progress on Traversing to UI?

Copy link
Member Author

Choose a reason for hiding this comment

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

The actually traversal step, does not get reported with progress because its super quick and only happens once.

TraverseWithProgress, since it returns a lazily enumerated IEnumerable reports the actual conversion progress (as the enumeration of objects occurs)

This was simply my hope to be DRY with the progress reporting, but I'm not tied to it if we find a better way.

.Where(obj => obj.Current is not Collection);

var convertedIds = BakeObjects(objectsToConvert, baseLayerName, onOperationProgressed, cancellationToken);
var conversionResults = BakeObjects(objectsToConvert, baseLayerName);

_contextStack.Current.Document.Views.Redraw();

return convertedIds;
return conversionResults;
}

// POC: Potentially refactor out into an IObjectBaker.
private List<string> BakeObjects(
IReadOnlyCollection<(string[], Base)> objects,
string baseLayerName,
Action<string, double?>? onOperationProgressed,
CancellationToken cancellationToken
)
private IReadOnlyList<ConversionResult> BakeObjects(IEnumerable<TraversalContext> objectsGraph, string baseLayerName)
{
RhinoDoc doc = _contextStack.Current.Document;
var rootLayerIndex = _contextStack.Current.Document.Layers.Find(Guid.Empty, baseLayerName, RhinoMath.UnsetIntIndex);
Expand Down Expand Up @@ -85,48 +78,36 @@ CancellationToken cancellationToken
rootLayerIndex = doc.Layers.Add(new Layer { Name = baseLayerName });
cache.Add(baseLayerName, rootLayerIndex);

var newObjectIds = new List<string>();
var count = 0;

// POC: We delay throwing conversion exceptions until the end of the conversion loop, then throw all within an aggregate exception if something happened.
var conversionExceptions = new List<Exception>();

using var noDraw = new DisableRedrawScope(doc.Views);

foreach ((string[] path, Base baseObj) in objects)
var conversionResults = new List<ConversionResult>();

foreach (TraversalContext tc in objectsGraph)
{
try
{
cancellationToken.ThrowIfCancellationRequested();
var path = GetLayerPath(tc);

var fullLayerName = string.Join(Layer.PathSeparator, path);
var layerIndex = cache.TryGetValue(fullLayerName, out int value)
? value
: GetAndCreateLayerFromPath(path, baseLayerName, cache);

onOperationProgressed?.Invoke("Converting & creating objects", (double)++count / objects.Count);
var result = _converter.Convert(tc.Current);

var result = _converter.Convert(baseObj);

var conversionIds = HandleConversionResult(result, baseObj, layerIndex);
newObjectIds.AddRange(conversionIds);
}
catch (OperationCanceledException)
{
throw;
var conversionIds = HandleConversionResult(result, tc.Current, layerIndex);
foreach (var r in conversionIds)
{
conversionResults.Add(new(tc.Current, result, r));
}
}
catch (Exception e) when (!e.IsFatal())
catch (Exception ex) when (!ex.IsFatal())
{
conversionExceptions.Add(e);
conversionResults.Add(new(tc.Current, ex));
}
}

if (conversionExceptions.Count != 0)
{
throw new AggregateException("Conversion failed for some objects.", conversionExceptions);
}

return newObjectIds;
return conversionResults;
}

private IReadOnlyList<string> HandleConversionResult(object conversionResult, Base originalObject, int layerIndex)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using Speckle.Connectors.DUI.Bridge;
using Speckle.Connectors.DUI.Models.Card;
using Speckle.Core.Models;

namespace Speckle.Connectors.DUI.Bindings;

Expand All @@ -11,13 +12,16 @@ public class ReceiveBindingUICommands : BasicConnectorBindingCommands
public ReceiveBindingUICommands(IBridge bridge)
: base(bridge) { }

public void SetModelReceiveResult(string modelCardId, List<string> bakedObjectIds)
public void SetModelReceiveResult(string modelCardId, IReadOnlyList<ConversionResult> conversionResults)
{
ReceiverModelCardResult res =
new()
{
ModelCardId = modelCardId,
ReceiveResult = new ReceiveResult() { BakedObjectIds = bakedObjectIds }
ReceiveResult = new ReceiveResult()
Copy link
Member

Choose a reason for hiding this comment

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

Need some changes on UI side to send proper result

{
BakedObjectIds = conversionResults.Where(x => x.IsSuccessful).Select(x => x.ResultId!).ToList()
}
};
Bridge.Send(SET_MODEL_RECEIVE_RESULT_UI_COMMAND_NAME, res);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public interface IHostObjectBuilder
/// <returns> List of application ids.</returns> // POC: Where we will return these ids will matter later when we target to also cache received application ids.
/// <remarks>Project and model name are needed for now to construct host app objects into related layers or filters.
/// POC: we might consider later to have HostObjectBuilderContext? that might hold all possible data we will need.</remarks>
IEnumerable<string> Build(
IReadOnlyList<ConversionResult> Build(
Base rootObject,
string projectName,
string modelName,
Expand Down
Loading