fix(revit): CNX-394 better reference point cache invalidation (#213)

* adds reference point transform to xyz, and refactors

* spelling fixes

* adds reference point caclulation to connector instead of converter

* disables xyz test for now due to complications mocking the reference point converter

* moves reference point transform method to separate class

* extracts to speckle settings into manager singleton

* removes  unecessary usings

* Pragma the send function sorry to not sorry

* optimizes mesh conversions

* removes unnessary meshconversion method and adds reference point transform to meshlist conversion

* better reference point invalidation logic

* Update MeshByMaterialDictionaryToSpeckle.cs

* fixes id evictions for objects that need to be unpacked

* removes circular dependency

---------

Co-authored-by: oguzhankoral <oguzhankoral@gmail.com>
Co-authored-by: Oğuzhan Koral <45078678+oguzhankoral@users.noreply.github.com>

Connectors/Revit/Speckle.Connectors.RevitShared/Bindings/RevitSendBinding.cs

@@ -11,6 +11,7 @@
 using Speckle.Connectors.DUI.Models.Card;
 using Speckle.Connectors.DUI.Models.Card.SendFilter;
 using Speckle.Connectors.DUI.Settings;
+using Speckle.Connectors.Revit.HostApp;
 using Speckle.Connectors.Revit.Operations.Send.Settings;
 using Speckle.Connectors.Revit.Plugin;
 using Speckle.Connectors.Utils.Caching;
@@ -32,6 +33,7 @@ internal sealed class RevitSendBinding : RevitBaseBinding, ISendBinding
   private readonly IOperationProgressManager _operationProgressManager;
   private readonly ToSpeckleSettingsManager _toSpeckleSettingsManager;
   private readonly ILogger<RevitSendBinding> _logger;
+  private readonly ElementUnpacker _elementUnpacker;
 
   /// <summary>
   /// Used internally to aggregate the changed objects' id. Note we're using a concurrent dictionary here as the expiry check method is not thread safe, and this was causing problems. See:
@@ -51,7 +53,8 @@ public RevitSendBinding(
     ISendConversionCache sendConversionCache,
     IOperationProgressManager operationProgressManager,
     ToSpeckleSettingsManager toSpeckleSettingsManager,
-    ILogger<RevitSendBinding> logger
+    ILogger<RevitSendBinding> logger,
+    ElementUnpacker elementUnpacker
   )
     : base("sendBinding", store, bridge, revitContext)
   {
@@ -62,6 +65,7 @@ ILogger<RevitSendBinding> logger
     _operationProgressManager = operationProgressManager;
     _toSpeckleSettingsManager = toSpeckleSettingsManager;
     _logger = logger;
+    _elementUnpacker = elementUnpacker;
     var topLevelExceptionHandler = Parent.TopLevelExceptionHandler;
 
     Commands = new SendBindingUICommands(bridge);
@@ -186,7 +190,8 @@ private void DocChangeHandler(Autodesk.Revit.DB.Events.DocumentChangedEventArgs
     if (HaveUnitsChanged(e.GetDocument()))
     {
       var objectIds = Store.GetSenders().SelectMany(s => s.SendFilter != null ? s.SendFilter.GetObjectIds() : []);
-      _sendConversionCache.EvictObjects(objectIds);
+      var unpackedObjectIds = _elementUnpacker.GetUnpackedElementIds(objectIds.ToList());
+      _sendConversionCache.EvictObjects(unpackedObjectIds);
     }
     _idleManager.SubscribeToIdle(nameof(RevitSendBinding), RunExpirationChecks);
   }

Connectors/Revit/Speckle.Connectors.RevitShared/HostApp/ElementUnpacker.cs

@@ -9,11 +9,11 @@ namespace Speckle.Connectors.Revit.HostApp;
 /// </summary>
 public class ElementUnpacker
 {
-  private readonly IRevitConversionContextStack _contextStack;
+  private readonly RevitContext _revitContext;
 
-  public ElementUnpacker(IRevitConversionContextStack contextStack)
+  public ElementUnpacker(RevitContext revitContext)
   {
-    _contextStack = contextStack;
+    _revitContext = revitContext;
   }
 
   /// <summary>
@@ -34,26 +34,39 @@ public IEnumerable<Element> UnpackSelectionForConversion(IEnumerable<Element> se
     return PackCurtainWallElements(atomicObjects);
   }
 
+  /// <summary>
+  /// Unpacks input element ids into their subelements, eg groups and nested family instances
+  /// </summary>
+  /// <param name="objectIds"></param>
+  /// <returns></returns>
+  /// <remarks>
+  /// This is used to invalidate object ids in the send conversion cache when the selected object id is only the parent element id
+  /// </remarks>
+  public IEnumerable<string> GetUnpackedElementIds(List<string> objectIds)
+  {
+    var doc = _revitContext.UIApplication?.ActiveUIDocument.Document!;
+    var docElements = doc.GetElements(objectIds);
+    return UnpackSelectionForConversion(docElements).Select(o => o.UniqueId).ToList();
+  }
+
   private List<Element> UnpackElements(IEnumerable<Element> elements)
   {
     var unpackedElements = new List<Element>(); // note: could be a hashset/map so we prevent duplicates (?)
+    var doc = _revitContext.UIApplication?.ActiveUIDocument.Document!;
 
     foreach (var element in elements)
     {
       // UNPACK: Groups
       if (element is Group g)
       {
         // POC: this might screw up generating hosting rel generation here, because nested families in groups get flattened out by GetMemberIds().
-        var groupElements = g.GetMemberIds().Select(_contextStack.Current.Document.GetElement);
+        var groupElements = g.GetMemberIds().Select(doc.GetElement);
         unpackedElements.AddRange(UnpackElements(groupElements));
       }
       // UNPACK: Family instances (as they potentially have nested families inside)
       else if (element is FamilyInstance familyInstance)
       {
-        var familyElements = familyInstance
-          .GetSubComponentIds()
-          .Select(_contextStack.Current.Document.GetElement)
-          .ToArray();
+        var familyElements = familyInstance.GetSubComponentIds().Select(doc.GetElement).ToArray();
 
         if (familyElements.Length != 0)
         {
@@ -64,7 +77,7 @@ private List<Element> UnpackElements(IEnumerable<Element> elements)
       }
       else if (element is MultistoryStairs multistoryStairs)
       {
-        var stairs = multistoryStairs.GetAllStairsIds().Select(_contextStack.Current.Document.GetElement);
+        var stairs = multistoryStairs.GetAllStairsIds().Select(doc.GetElement);
         unpackedElements.AddRange(UnpackElements(stairs));
       }
       else

Connectors/Revit/Speckle.Connectors.RevitShared/Operations/Send/Settings/ToSpeckleSettingsManager.cs

@@ -1,6 +1,7 @@
 using Autodesk.Revit.DB;
 using Autodesk.Revit.UI;
 using Speckle.Connectors.DUI.Models.Card;
+using Speckle.Connectors.Revit.HostApp;
 using Speckle.Connectors.Utils.Caching;
 using Speckle.Converters.RevitShared.Helpers;
 using Speckle.Converters.RevitShared.Settings;
@@ -12,14 +13,20 @@ public class ToSpeckleSettingsManager
 {
   private readonly RevitContext _revitContext;
   private readonly ISendConversionCache _sendConversionCache;
+  private readonly ElementUnpacker _elementUnpacker;
 
   // cache invalidation process run with ModelCardId since the settings are model specific
   private readonly Dictionary<string, DetailLevelType> _detailLevelCache = new();
-  private readonly Dictionary<string, ReferencePointType> _referencePointCache = new();
+  private readonly Dictionary<string, Transform?> _referencePointCache = new();
 
-  public ToSpeckleSettingsManager(RevitContext revitContext, ISendConversionCache sendConversionCache)
+  public ToSpeckleSettingsManager(
+    RevitContext revitContext,
+    ISendConversionCache sendConversionCache,
+    ElementUnpacker elementUnpacker
+  )
   {
     _revitContext = revitContext;
+    _elementUnpacker = elementUnpacker;
     _sendConversionCache = sendConversionCache;
   }
 
@@ -44,7 +51,8 @@ fidelityString is not null
         if (previousType != fidelity)
         {
           var objectIds = modelCard.SendFilter != null ? modelCard.SendFilter.GetObjectIds() : [];
-          _sendConversionCache.EvictObjects(objectIds);
+          var unpackedObjectIds = _elementUnpacker.GetUnpackedElementIds(objectIds);
+          _sendConversionCache.EvictObjects(unpackedObjectIds);
         }
       }
       _detailLevelCache[modelCard.ModelCardId.NotNull()] = fidelity;
@@ -65,17 +73,23 @@ out ReferencePointType referencePoint
       )
     )
     {
-      if (_referencePointCache.TryGetValue(modelCard.ModelCardId.NotNull(), out ReferencePointType previousType))
+      // get the current transform from setting first
+      // we are doing this because we can't track if reference points were changed between send operations.
+      Transform? currentTransform = GetTransform(_revitContext, referencePoint);
+
+      if (_referencePointCache.TryGetValue(modelCard.ModelCardId.NotNull(), out Transform? previousTransform))
       {
-        if (previousType != referencePoint)
+        // invalidate conversion cache if the transform has changed
+        if (previousTransform != currentTransform)
         {
           var objectIds = modelCard.SendFilter != null ? modelCard.SendFilter.GetObjectIds() : [];
-          _sendConversionCache.EvictObjects(objectIds);
+          var unpackedObjectIds = _elementUnpacker.GetUnpackedElementIds(objectIds);
+          _sendConversionCache.EvictObjects(unpackedObjectIds);
         }
       }
 
-      _referencePointCache[modelCard.ModelCardId.NotNull()] = referencePoint;
-      return GetTransform(_revitContext, referencePoint);
+      _referencePointCache[modelCard.ModelCardId.NotNull()] = currentTransform;
+      return currentTransform;
     }
 
     throw new ArgumentException($"Invalid reference point value: {referencePointString}");

Converters/Revit/Speckle.Converters.RevitShared/ToSpeckle/Raw/Geometry/MeshByMaterialDictionaryToSpeckle.cs

@@ -60,7 +60,6 @@ RevitMaterialCacheSingleton materialCacheSingleton
 
       // use the meshlist converter to convert the mesh values into a single speckle mesh
       SOG.Mesh speckleMesh = _meshListConverter.Convert(meshes);
-      speckleMesh.units = _contextStack.Current.SpeckleUnits;
       speckleMesh.applicationId = Guid.NewGuid().ToString(); // NOTE: as we are composing meshes out of multiple ones for the same material, we need to generate our own application id. c'est la vie.
 
       // get the render material if any

Sdk/Speckle.Connectors.Utils/Caching/ISendConversionCache.cs

@@ -19,7 +19,6 @@ public interface ISendConversionCache
   /// </summary>
   /// <param name="objectIds"></param>
   public void EvictObjects(IEnumerable<string> objectIds);
-
   public void ClearCache();
   bool TryGetValue(string projectId, string applicationId, out ObjectReference objectReference);
 }