ConversionResult in HostObjectBuilder
#3437
Conversation
Core/Core/Models/ConversionResult.cs
Outdated
| public ConversionResult(object target, object result, string resultId) | ||
| { | ||
| Target = target; | ||
| Result = result; |
There was a problem hiding this comment.
This object might know whether the conversion is successful with fallback displayValue or not. I mean, it is "successful" with "but". TBD
There was a problem hiding this comment.
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
|
|
||
| results.AddRange(convertedObjects.Select(e => new ConversionResult(tc, e, e.Handle.Value.ToString()))); | ||
| } | ||
| catch (Exception ex) when (!ex.IsFatal()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'll take a look at Receive today, if a new class makes sense, lets do it.
| @@ -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); | |||
There was a problem hiding this comment.
This is where I can start to think interop
| .Where(obj => obj.Current is not Collection) | ||
| .Select(ctx => (GetLayerPath(ctx), ctx.Current)) | ||
| .ToArray(); | ||
| .TraverseWithProgress(rootObject, onOperationProgressed, cancellationToken) |
There was a problem hiding this comment.
Do we report progress on Traversing to UI?
There was a problem hiding this comment.
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.
| { | ||
| ReceiverModelCardResult res = | ||
| new() | ||
| { | ||
| ModelCardId = modelCardId, | ||
| ReceiveResult = new ReceiveResult() { BakedObjectIds = bakedObjectIds } | ||
| ReceiveResult = new ReceiveResult() |
There was a problem hiding this comment.
Need some changes on UI side to send proper result
|
|
||
| yield return tc; | ||
|
|
||
| onOperationProgressed?.Invoke("Converting", (double)++count / traversalGraph.Length); |
There was a problem hiding this comment.
Should we really report to UI progress when traversing? If so I believe it shouldn't be named with "Converting"?
There was a problem hiding this comment.
See other comment, this isn't traversal progress (because traversal is super quick)
This is the conversion progress (as the IEnumerable is being enumerated)
Let me know if you want me to explain this better over voice call.
| @@ -54,24 +52,13 @@ public object Convert(Base target) | |||
| return FallbackToDisplayValue(displayValue); | |||
There was a problem hiding this comment.
Need some thoughts on it -> How can we report fallback conversion, or should we?
There was a problem hiding this comment.
See other comment above, Perhaps we don't need to, since we will know the original and result type and that's the main thing users will care about.
But if we wanted to go further, and know the converter that was used, I have an idea, but I think we should wait till we can get some feedback from the team.
|
Changes will be included in #3448 |
Still WIP