Merge pull request #1245 from specklesystems/main-backmerge

main backmerge

Connectors/Navisworks/Speckle.Connectors.NavisworksShared/Operations/Send/NavisworksRootObjectBuilder.cs

@@ -30,15 +30,13 @@ public class NavisworksRootObjectBuilder(
   NavisworksColorUnpacker colorUnpacker,
   Speckle.Converter.Navisworks.Constants.Registers.IInstanceFragmentRegistry instanceRegistry,
   IElementSelectionService elementSelectionService,
-  IUiUnitsCache uiUnitsCache,
-  bool disableGroupingForInstanceTesting,
-  bool skipNodeMerging
+  IUiUnitsCache uiUnitsCache
 ) : IRootObjectBuilder<NAV.ModelItem>
 {
 #pragma warning disable CA1823
 #pragma warning restore CA1823
-  private bool SkipNodeMerging { get; } = skipNodeMerging;
-  private bool DisableGroupingForInstanceTesting { get; } = disableGroupingForInstanceTesting;
+  private bool SkipNodeMerging { get; set; }
+  private bool DisableGroupingForInstanceTesting { get; set; }
 
   public async Task<RootObjectBuilderResult> Build(
     IReadOnlyList<NAV.ModelItem> navisworksModelItems,

Connectors/Revit/Speckle.Connectors.RevitShared/Operations/Receive/RevitHostObjectBuilder.cs

@@ -21,6 +21,7 @@ using Speckle.Sdk.Common;
 using Speckle.Sdk.Common.Exceptions;
 using Speckle.Sdk.Logging;
 using Speckle.Sdk.Models;
+using Speckle.Sdk.Models.Instances;
 using Transform = Speckle.Objects.Other.Transform;
 
 namespace Speckle.Connectors.Revit.Operations.Receive;
@@ -38,12 +39,15 @@ public sealed class RevitHostObjectBuilder(
   IThreadContext threadContext,
   RevitToHostCacheSingleton revitToHostCacheSingleton,
   ITypedConverter<
-    (Base atomicObject, IReadOnlyCollection<Matrix4x4> matrix),
+    (Base atomicObject, IReadOnlyCollection<Matrix4x4> matrix, DataObject? parentDataObject),
     DirectShape
   > localToGlobalDirectShapeConverter,
   IReceiveConversionHandler conversionHandler
 ) : IHostObjectBuilder, IDisposable
 {
+  // Maps atomic object applicationId -> parent DataObject
+  private readonly Dictionary<string, DataObject> _atomicObjectToParentDataObject = new();
+
   public Task<HostObjectBuilderResult> Build(
     Base rootObject,
     string projectName,
@@ -102,6 +106,9 @@ public sealed class RevitHostObjectBuilder(
       unpackedRoot.ObjectsToConvert.ToList()
     );
 
+    // Register DataObjects with InstanceProxy displayValues
+    RegisterDataObjectsWithInstanceProxies(unpackedRoot);
+
     // NOTE: below is 💩... https://github.com/specklesystems/speckle-sharp-connectors/pull/813 broke sketchup to revit workflow
     // ids were modified to fix receiving instances [CNX-1707](https://linear.app/speckle/issue/CNX-1707/revit-curves-and-meshes-in-blocks-come-as-duplicated)
     // but we then broke sketchup to revit because applicationIds in proxies didn't match modified application ids which cam from #813 hack
@@ -176,6 +183,9 @@ public sealed class RevitHostObjectBuilder(
       }
     }
 
+    // Update DataObject lookup IDs
+    UpdateAtomicObjectLookupWithModifiedIds(originalToModifiedIds);
+
     // 2 - Bake materials (now with the updated IDs)
     if (unpackedRoot.RenderMaterialProxies != null)
     {
@@ -234,6 +244,87 @@ public sealed class RevitHostObjectBuilder(
     return conversionResults.builderResult;
   }
 
+  /// <summary>
+  /// Registers DataObjects that have InstanceProxy displayValues and builds the lookup.
+  /// </summary>
+  private void RegisterDataObjectsWithInstanceProxies(RootObjectUnpackerResult unpackedRoot)
+  {
+    var definitionToDataObject = new Dictionary<string, DataObject>();
+
+    foreach (var tc in unpackedRoot.ObjectsToConvert)
+    {
+      if (tc.Current is DataObject dataObject)
+      {
+        var instanceProxies = dataObject.displayValue.OfType<InstanceProxy>().ToList();
+        if (instanceProxies.Count > 0)
+        {
+          foreach (var ip in instanceProxies)
+          {
+            definitionToDataObject[ip.definitionId] = dataObject;
+          }
+        }
+      }
+    }
+
+    // Build lookup: definition object applicationId -> parent DataObject
+    _atomicObjectToParentDataObject.Clear();
+    if (unpackedRoot.DefinitionProxies is not null)
+    {
+      foreach (var defProxy in unpackedRoot.DefinitionProxies)
+      {
+        if (
+          defProxy.applicationId is not null
+          && definitionToDataObject.TryGetValue(defProxy.applicationId, out var parentDataObject)
+        )
+        {
+          foreach (var objectId in defProxy.objects)
+          {
+            _atomicObjectToParentDataObject[objectId] = parentDataObject;
+          }
+        }
+        else
+        {
+          logger.LogError(
+            "Could not find parent DataObject for DefinitionProxy {ApplicationId}",
+            defProxy.applicationId
+          );
+        }
+      }
+    }
+  }
+
+  /// <summary>
+  /// Updates the atomic object lookup with modified IDs
+  /// </summary>
+  private void UpdateAtomicObjectLookupWithModifiedIds(Dictionary<string, List<string>> originalToModifiedIds)
+  {
+    // Build updated entries first to avoid modifying collection during iteration
+    var entriesToAdd = new List<KeyValuePair<string, DataObject>>();
+    var keysToRemove = new List<string>();
+
+    foreach (var kvp in _atomicObjectToParentDataObject)
+    {
+      if (originalToModifiedIds.TryGetValue(kvp.Key, out var modifiedIds))
+      {
+        keysToRemove.Add(kvp.Key);
+        foreach (var modifiedId in modifiedIds)
+        {
+          entriesToAdd.Add(new(modifiedId, kvp.Value));
+        }
+      }
+    }
+
+    foreach (var key in keysToRemove)
+    {
+      _atomicObjectToParentDataObject.Remove(key);
+    }
+
+    foreach (var entry in entriesToAdd)
+    {
+      _atomicObjectToParentDataObject[entry.Key] = entry.Value;
+    }
+  }
+
   private Autodesk.Revit.DB.Transform? CalculateNewTransform(
     Autodesk.Revit.DB.Transform? receiveTransform,
     Autodesk.Revit.DB.Transform? rootTransform
@@ -278,9 +369,17 @@ public sealed class RevitHostObjectBuilder(
         onOperationProgressed.Report(new("Converting", (double)++count / localToGlobalMaps.Count));
         if (result is DirectShapeDefinitionWrapper)
         {
+          // Look up parent DataObject for this atomic object (handles InstanceProxy displayValue)
+          var atomicId = localToGlobalMap.AtomicObject.applicationId;
+          DataObject? parentDataObject = null;
+          if (atomicId is not null)
+          {
+            _atomicObjectToParentDataObject.TryGetValue(atomicId, out parentDataObject);
+          }
+
           // direct shape creation happens here
           DirectShape directShapes = localToGlobalDirectShapeConverter.Convert(
-            (localToGlobalMap.AtomicObject, localToGlobalMap.Matrix)
+            (localToGlobalMap.AtomicObject, localToGlobalMap.Matrix, parentDataObject)
           );
 
           bakedObjectIds.Add(directShapes.UniqueId);
@@ -351,6 +450,7 @@ public sealed class RevitHostObjectBuilder(
     DirectShapeLibrary.GetDirectShapeLibrary(converterSettings.Current.Document).Reset(); // Note: this needs to be cleared, as it is being used in the converter
 
     revitToHostCacheSingleton.MaterialsByObjectId.Clear(); // Massive hack!
+    _atomicObjectToParentDataObject.Clear();
     groupManager.PurgeGroups(baseGroupName);
     materialBaker.PurgeMaterials(baseGroupName);
   }

Converters/Revit/Speckle.Converters.RevitShared/ToHost/Raw/LocalToGlobalToDirectShapeConverter.cs

@@ -2,6 +2,7 @@ using Speckle.Converters.Common;
 using Speckle.Converters.Common.Objects;
 using Speckle.Converters.RevitShared.Settings;
 using Speckle.DoubleNumerics;
+using Speckle.Objects.Data;
 using Speckle.Sdk.Common;
 using Speckle.Sdk.Models;
 using Speckle.Sdk.Models.Extensions;
@@ -10,11 +11,14 @@ namespace Speckle.Converters.RevitShared.ToSpeckle;
 
 /// <summary>
 /// Converts local to global maps to direct shapes.
-/// Spirit of the LocalToGlobalMap, we can't pass that object directly here bc it lives in Connectors.Common which I (ogu) don't want to bother with it.
-/// All this is  poc that should be burned, once we enable proper block support to revit.
+/// When atomicObject comes from an InstanceProxy displayValue, parentDataObject
+/// provides the original DataObject's metadata (category, name) for semantic preservation.
 /// </summary>
 public class LocalToGlobalToDirectShapeConverter
-  : ITypedConverter<(Base atomicObject, IReadOnlyCollection<Matrix4x4> matrix), DB.DirectShape>
+  : ITypedConverter<
+    (Base atomicObject, IReadOnlyCollection<Matrix4x4> matrix, DataObject? parentDataObject),
+    DB.DirectShape
+  >
 {
   private readonly IConverterSettingsStore<RevitConversionSettings> _converterSettings;
   private readonly ITypedConverter<(Matrix4x4 matrix, string units), DB.Transform> _transformConverter;
@@ -28,22 +32,13 @@ public class LocalToGlobalToDirectShapeConverter
     _transformConverter = transformConverter;
   }
 
-  public DB.DirectShape Convert((Base atomicObject, IReadOnlyCollection<Matrix4x4> matrix) target)
+  public DB.DirectShape Convert(
+    (Base atomicObject, IReadOnlyCollection<Matrix4x4> matrix, DataObject? parentDataObject) target
+  )
   {
     // 1- set ds category
-    // NOTE: previously, builtInCategory was on the atomicObject level. this was subsequently moved to properties
-    string? category = null;
-
-    // NOTE: no longer limited to DataObject since the introduction of mapper
-    // The change from `if (target.atomicObject is DataObject dataObject)` is very hacky, but nothing else to do for now
-    // TODO: better define prop interfaces for different applications
-    if (
-      target.atomicObject["properties"] is Dictionary<string, object?> properties
-      && properties.TryGetValue("builtInCategory", out var builtInCategory)
-    )
-    {
-      category = builtInCategory?.ToString();
-    }
+    var category = ExtractBuiltInCategory(target.parentDataObject, target.atomicObject);
+    var name = target.parentDataObject?.name ?? target.atomicObject.TryGetName();
 
     var dsCategory = DB.BuiltInCategory.OST_GenericModel;
     if (category is not null)
@@ -62,10 +57,6 @@ public class LocalToGlobalToDirectShapeConverter
     // 2 - init DirectShape
     var result = DB.DirectShape.CreateElement(_converterSettings.Current.Document, new DB.ElementId(dsCategory));
 
-    // NOTE: this should technically be in a property extraction class / helper method
-    // This change is localised to [CNX-1825](https://linear.app/speckle/issue/CNX-1825/set-directshape-name)
-    // TODO: Property extraction is a greater conversation which needs to be had: [CNX-1830](https://linear.app/speckle/issue/CNX-1830/data-exchange-investigations)
-    var name = target.atomicObject.TryGetName();
     if (name is not null)
     {
       result.SetName(name);
@@ -121,4 +112,24 @@ public class LocalToGlobalToDirectShapeConverter
     result.SetShape(transformedGeometries);
     return result;
   }
+
+  private static string? ExtractBuiltInCategory(DataObject? parentDataObject, Base atomicObject)
+  {
+    // Try parent DataObject first (for InstanceProxy displayValue case)
+    if (parentDataObject?.properties.TryGetValue("builtInCategory", out var cat) == true)
+    {
+      return cat?.ToString();
+    }
+
+    // Fallback to atomicObject properties
+    if (
+      atomicObject["properties"] is Dictionary<string, object?> props
+      && props.TryGetValue("builtInCategory", out var fallbackCat)
+    )
+    {
+      return fallbackCat?.ToString();
+    }
+
+    return null;
+  }
 }