PR review comments

- Dictionary lookups for material and section proxies
- Only create proxies for assigned sections and materials (not pretty)

Connectors/CSi/Speckle.Connectors.CSiShared/HostApp/Helpers/ISectionUnpacker.cs

@@ -5,5 +5,9 @@ namespace Speckle.Connectors.CSiShared.HostApp.Helpers;
 
 public interface ISectionUnpacker
 {
-  List<IProxyCollection> UnpackSections(Collection rootObjectCollection);
+  IReadOnlyDictionary<string, IProxyCollection> UnpackSections(
+    Collection rootObjectCollection,
+    string[] frameSectionNames,
+    string[] shellSectionNames
+  );
 }

Connectors/CSi/Speckle.Connectors.CSiShared/HostApp/MaterialUnpacker.cs

@@ -36,17 +36,16 @@ CsiMaterialPropertyExtractor propertyExtractor
     _propertyExtractor = propertyExtractor;
   }
 
-  public List<IProxyCollection> UnpackMaterials(Collection rootObjectCollection)
+  public IReadOnlyDictionary<string, IProxyCollection> UnpackMaterials(
+    Collection rootObjectCollection,
+    string[] materialNames
+  )
   {
     try
     {
       using var activity = _activityFactory.Start("Unpack Materials");
 
-      int numberOfMaterials = 0;
-      string[] materialNames = [];
-      _csiApplicationService.SapModel.PropMaterial.GetNameList(ref numberOfMaterials, ref materialNames);
-
-      Dictionary<string, IProxyCollection> materials = [];
+      var materials = new Dictionary<string, IProxyCollection>();
 
       foreach (string materialName in materialNames)
       {
@@ -73,18 +72,17 @@ public List<IProxyCollection> UnpackMaterials(Collection rootObjectCollection)
         }
       }
 
-      var materialProxies = materials.Values.ToList();
-      if (materialProxies.Count > 0)
+      if (materials.Count > 0)
       {
-        rootObjectCollection[ProxyKeys.MATERIAL] = materialProxies;
+        rootObjectCollection[ProxyKeys.MATERIAL] = materials.Values.ToList();
       }
 
-      return materialProxies;
+      return materials;
     }
     catch (Exception ex) when (!ex.IsFatal())
     {
       _logger.LogError(ex, "Failed to unpack materials");
-      return [];
+      return new Dictionary<string, IProxyCollection>();
     }
   }
 }

Connectors/CSi/Speckle.Connectors.CSiShared/HostApp/Relationships/IObjectSectionRelationshipManager.cs

@@ -15,5 +15,8 @@ public interface IObjectSectionRelationshipManager
   /// <summary>
   /// Establishes relationships between converted objects and their section proxies.
   /// </summary>
-  void EstablishRelationships(List<Base> convertedObjectsByType, List<IProxyCollection> sections);
+  void EstablishRelationships(
+    List<Base> convertedObjectsByType,
+    IReadOnlyDictionary<string, IProxyCollection> sections
+  );
 }

Connectors/CSi/Speckle.Connectors.CSiShared/HostApp/Relationships/ISectionMaterialRelationshipManager.cs

@@ -14,5 +14,8 @@ public interface ISectionMaterialRelationshipManager
   /// <summary>
   /// Establishes bidirectional relationships between section and material proxies.
   /// </summary>
-  void EstablishRelationships(List<IProxyCollection> sections, List<IProxyCollection> materials);
+  void EstablishRelationships(
+    IReadOnlyDictionary<string, IProxyCollection> sections,
+    IReadOnlyDictionary<string, IProxyCollection> materials
+  );
 }

Connectors/CSi/Speckle.Connectors.CSiShared/HostApp/Relationships/ObjectSectionRelationshipManager.cs

@@ -23,33 +23,39 @@ public ObjectSectionRelationshipManager(ILogger<ObjectSectionRelationshipManager
     _logger = logger;
   }
 
-  public void EstablishRelationships(List<Base> convertedObjectsByType, List<IProxyCollection> sections)
+  public void EstablishRelationships(
+    List<Base> convertedObjectsByType,
+    IReadOnlyDictionary<string, IProxyCollection> sections
+  )
   {
     foreach (var obj in convertedObjectsByType)
     {
       string? sectionName = GetObjectSectionName(obj);
-      if (string.IsNullOrEmpty(sectionName))
+      if (sectionName == null)
       {
+        _logger.LogError($"No section name (sectionId) found for object {obj.applicationId}.");
         continue;
       }
 
-      var section = sections.FirstOrDefault(s => s.id == sectionName);
-      if (section == null)
+      if (!sections.TryGetValue(sectionName, out var section))
       {
-        continue;
+        continue; // This is valid. An opening has "none" for sectionId assignment. Not an error.
       }
 
-      if (!section.objects.Contains(obj.applicationId!))
+      if (section.objects.Contains(obj.applicationId!))
       {
-        section.objects.Add(obj.applicationId!);
+        _logger.LogError($"No object should be processed twice. This is occuring for Section {obj.applicationId}");
+        continue;
       }
+
+      section.objects.Add(obj.applicationId!);
     }
   }
 
   private string? GetObjectSectionName(Base baseObject)
   {
-    // TODO: We need to refine the accessibility of sectionProperty in a more robust manner
-    // 🙍‍♂️ This below is horrible! I know. But both SHELL and FRAME have the same nested property structure (unformalised)
+    // 🙍‍♂️ This below is horrible! Heavy use of dictionary-style property access is brittle
+    // TODO: Make better :)
     try
     {
       if (baseObject["properties"] is not Dictionary<string, object?> properties)

Connectors/CSi/Speckle.Connectors.CSiShared/HostApp/Relationships/SectionMaterialRelationshipManager.cs

@@ -19,27 +19,37 @@ public SectionMaterialRelationshipManager(ILogger<SectionMaterialRelationshipMan
     _logger = logger;
   }
 
-  public void EstablishRelationships(List<IProxyCollection> sections, List<IProxyCollection> materials)
+  public void EstablishRelationships(
+    IReadOnlyDictionary<string, IProxyCollection> sections,
+    IReadOnlyDictionary<string, IProxyCollection> materials
+  )
   {
-    foreach (var section in sections)
+    foreach (var section in sections.Values)
     {
       // This is critical that FrameSectionUnpacker and ShellSectionUnpacker extract material name exactly the same!
+      // Maybe better to access materialId nested within properties? This "formalised" extraction result is not nice.
       var materialName = ((Base)section)["MaterialName"]?.ToString();
-      if (string.IsNullOrEmpty(materialName))
+      if (materialName == null)
       {
+        _logger.LogError($"Section {section.id} has no material name");
         continue;
       }
 
-      var material = materials.FirstOrDefault(m => m.id == materialName);
-      if (material == null)
+      if (!materials.TryGetValue(materialName, out var material))
       {
+        _logger.LogError(
+          $"Material {materialName} not found for section {section.id}. This indicates a conversion error"
+        );
         continue;
       }
 
-      if (!material.objects.Contains(section.id!))
+      if (material.objects.Contains(section.id!))
       {
-        material.objects.Add(section.id!);
+        _logger.LogError($"No section should be processed twice. This is occuring for Section {section.id}");
+        continue;
       }
+
+      material.objects.Add(section.id!);
     }
   }
 }

Connectors/CSi/Speckle.Connectors.CSiShared/Operations/Send/CsiRootObjectBuilder.cs

@@ -8,6 +8,7 @@
 using Speckle.Connectors.CSiShared.HostApp.Relationships;
 using Speckle.Converters.Common;
 using Speckle.Converters.CSiShared;
+using Speckle.Converters.CSiShared.Utils;
 using Speckle.Sdk;
 using Speckle.Sdk.Logging;
 using Speckle.Sdk.Models;
@@ -38,7 +39,10 @@ public class CsiRootObjectBuilder : IRootObjectBuilder<ICsiWrapper>
   private readonly ISectionUnpacker _sectionUnpacker;
   private readonly ISectionMaterialRelationshipManager _sectionMaterialRelationshipManager;
   private readonly IObjectSectionRelationshipManager _objectSectionRelationshipManager;
-  private readonly List<Base> _convertedObjectsForProxies = [];
+  private readonly List<Base> _convertedObjectsForProxies = []; // Not nice, but a way to store converted objects
+  private readonly HashSet<string> _assignedFrameSectionIds = []; // Track which sections we NEED to create proxies for
+  private readonly HashSet<string> _assignedShellSectionIds = []; // Track which sections we NEED to create proxies for
+  private readonly HashSet<string> _assignedMaterialIds = []; // Track which materials we NEED to create proxies for
   private readonly ILogger<CsiRootObjectBuilder> _logger;
   private readonly ISdkActivityFactory _activityFactory;
   private readonly ICsiApplicationService _csiApplicationService;
@@ -160,10 +164,11 @@ private SendConversionResult ConvertCsiObject(ICsiWrapper csiObject, Collection
       var collection = _sendCollectionManager.AddObjectCollectionToRoot(converted, typeCollection);
       collection.elements.Add(converted);
 
-      // NOTE: See remarks in docstrings
+      // If object requires section relationship, collect both section and material names
       if (csiObject.RequiresSectionRelationship)
       {
         _convertedObjectsForProxies.Add(converted);
+        AddMaterialAndSectionIdsToCache(converted);
       }
 
       return new(Status.SUCCESS, applicationId, sourceType, converted);
@@ -188,19 +193,81 @@ private SendConversionResult ConvertCsiObject(ICsiWrapper csiObject, Collection
   /// </remarks>
   private void ProcessProxies(Collection rootObjectCollection)
   {
+    // TODO: Only unpack materials and sections which are assigned in the model
     try
     {
       using var activity = _activityFactory.Start("Process Proxies");
 
-      var materialProxies = _materialUnpacker.UnpackMaterials(rootObjectCollection);
-      var sectionProxies = _sectionUnpacker.UnpackSections(rootObjectCollection);
+      var materials = _materialUnpacker.UnpackMaterials(rootObjectCollection, _assignedMaterialIds.ToArray());
+      var sections = _sectionUnpacker.UnpackSections(
+        rootObjectCollection,
+        _assignedFrameSectionIds.ToArray(),
+        _assignedShellSectionIds.ToArray()
+      );
 
-      _sectionMaterialRelationshipManager.EstablishRelationships(sectionProxies, materialProxies);
-      _objectSectionRelationshipManager.EstablishRelationships(_convertedObjectsForProxies, sectionProxies);
+      _sectionMaterialRelationshipManager.EstablishRelationships(sections, materials);
+      _objectSectionRelationshipManager.EstablishRelationships(_convertedObjectsForProxies, sections);
     }
     catch (Exception ex) when (!ex.IsFatal())
     {
       _logger.LogError(ex, "Failed to process section and material proxies");
     }
   }
+
+  /// <summary>
+  /// Extracts section and material names from a converted object's assignments and adds them to their respective collections.
+  /// This is only done for objects (FRAME and SHELL) where material - section - object relationships need to be est.
+  /// Why do we need this? We only want to create proxies for assigned sections and materials.
+  /// For this, we need to know what sections and materials have been assigned.
+  /// </summary>
+  /// <remarks>
+  /// This method safely traverses the nested dictionary structure of the converted object to find:
+  /// - sectionId under properties -> Assignments -> sectionId
+  /// - materialId under properties -> Assignments -> materialId
+  /// Both IDs are collected independently, as one can exist without the other.
+  /// </remarks>
+  private void AddMaterialAndSectionIdsToCache(Base converted)
+  {
+    // TODO: Improve. This is extremely brittle, but an appropriate workaround / interim solution!
+    // Check if we can get the assignments dictionary
+    if (
+      converted["properties"] is not Dictionary<string, object?> { } properties
+      || !properties.TryGetValue(ObjectPropertyCategory.ASSIGNMENTS, out var assignmentsObj)
+      || assignmentsObj is not Dictionary<string, object?> { } assignments
+    )
+    {
+      return;
+    }
+
+    // Get the object type
+    var objectType = converted["type"]?.ToString();
+
+    // Collect section IDs if they exist and are non-empty
+    if (
+      assignments.TryGetValue("sectionId", out var section)
+      && section?.ToString() is { } sectionId
+      && sectionId != "None"
+    )
+    {
+      switch (objectType)
+      {
+        case var type when type == ModelObjectType.FRAME.ToString():
+          _assignedFrameSectionIds.Add(sectionId);
+          break;
+        case var type when type == ModelObjectType.SHELL.ToString():
+          _assignedShellSectionIds.Add(sectionId);
+          break;
+      }
+    }
+
+    // Collect material IDs if they exist and are non-empty
+    if (
+      assignments.TryGetValue("materialId", out var material)
+      && material?.ToString() is { } materialId
+      && materialId != "None"
+    )
+    {
+      _assignedMaterialIds.Add(materialId);
+    }
+  }
 }

Connectors/CSi/Speckle.Connectors.CSiShared/Speckle.Connectors.CSiShared.projitems

@@ -13,8 +13,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bindings\CsiSharedSelectionBinding.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bindings\CsiSharedSendBinding.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Filters\CsiSharedSelectionFilter.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)HostApp\Helpers\MaterialProxy.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)HostApp\Helpers\SectionProxy.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)HostApp\MaterialUnpacker.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)HostApp\CsiSendCollectionManager.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)HostApp\Helpers\CsiFrameSectionPropertyExtractor.cs" />