Dimitrie/cnx 430 re evaluate parameters

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

@@ -69,7 +69,8 @@ public async Task Receive(string modelCardId)
         .Initialize(
           _revitConversionSettingsFactory.Create(
             DetailLevelType.Coarse, //TODO figure out
-            null
+            null,
+            false
           )
         );
       // Receive host objects

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

@@ -88,7 +88,11 @@ public List<ISendFilter> GetSendFilters()
   }
 
   public List<ICardSetting> GetSendSettings() =>
-    [new DetailLevelSetting(DetailLevelType.Medium), new ReferencePointSetting(ReferencePointType.InternalOrigin)];
+    [
+      new DetailLevelSetting(DetailLevelType.Medium),
+      new ReferencePointSetting(ReferencePointType.InternalOrigin),
+      new SendParameterNullOrEmptyStringsSetting(false)
+    ];
 
   public void CancelSend(string modelCardId) => _cancellationManager.CancelOperation(modelCardId);
 
@@ -116,7 +120,8 @@ public async Task Send(string modelCardId)
         .Initialize(
           _revitConversionSettingsFactory.Create(
             _toSpeckleSettingsManager.GetDetailLevelSetting(modelCard),
-            _toSpeckleSettingsManager.GetReferencePointSetting(modelCard)
+            _toSpeckleSettingsManager.GetReferencePointSetting(modelCard),
+            _toSpeckleSettingsManager.GetSendParameterNullOrEmptyStringsSetting(modelCard)
           )
         );
 

Connectors/Revit/Speckle.Connectors.RevitShared/Operations/Send/RevitRootObjectBuilder.cs

@@ -1,4 +1,3 @@
-using System.Diagnostics;
 using Autodesk.Revit.DB;
 using Microsoft.Extensions.Logging;
 using Revit.Async;
@@ -12,6 +11,7 @@
 using Speckle.Converters.Common;
 using Speckle.Converters.RevitShared.Helpers;
 using Speckle.Converters.RevitShared.Settings;
+using Speckle.Converters.RevitShared.ToSpeckle;
 using Speckle.Sdk;
 using Speckle.Sdk.Models;
 using Speckle.Sdk.Models.Collections;
@@ -29,15 +29,17 @@ public class RevitRootObjectBuilder : IRootObjectBuilder<ElementId>
   private readonly SendCollectionManager _sendCollectionManager;
   private readonly RevitMaterialCacheSingleton _revitMaterialCacheSingleton;
   private readonly ILogger<RevitRootObjectBuilder> _logger;
+  private readonly ParameterDefinitionHandler _parameterDefinitionHandler;
 
   public RevitRootObjectBuilder(
     IRootToSpeckleConverter converter,
     IConverterSettingsStore<RevitConversionSettings> converterSettings,
     ISendConversionCache sendConversionCache,
     ElementUnpacker elementUnpacker,
     SendCollectionManager sendCollectionManager,
-    RevitMaterialCacheSingleton revitMaterialCacheSingleton,
-    ILogger<RevitRootObjectBuilder> logger
+    ILogger<RevitRootObjectBuilder> logger,
+    ParameterDefinitionHandler parameterDefinitionHandler,
+    RevitMaterialCacheSingleton revitMaterialCacheSingleton
   )
   {
     _converter = converter;
@@ -47,6 +49,7 @@ ILogger<RevitRootObjectBuilder> logger
     _sendCollectionManager = sendCollectionManager;
     _revitMaterialCacheSingleton = revitMaterialCacheSingleton;
     _logger = logger;
+    _parameterDefinitionHandler = parameterDefinitionHandler;
 
     _rootObject = new Collection()
     {
@@ -108,7 +111,7 @@ public async Task<RootObjectBuilderResult> Build(
         }
         else
         {
-          converted = await RevitTask.RunAsync(() => _converter.Convert(revitElement)).ConfigureAwait(false);
+          converted = await RevitTask.RunAsync(() => _converter.Convert(revitElement)).ConfigureAwait(false); // Could we run these batched? Is there maybe a performance penalty for running these to speckle conversions individually in revittask.runasync?
           converted.applicationId = applicationId;
         }
 
@@ -133,10 +136,8 @@ public async Task<RootObjectBuilderResult> Build(
     var idsAndSubElementIds = _elementUnpacker.GetElementsAndSubelementIdsFromAtomicObjects(atomicObjects);
     var materialProxies = _revitMaterialCacheSingleton.GetRenderMaterialProxyListForObjects(idsAndSubElementIds);
     _rootObject[ProxyKeys.RENDER_MATERIAL] = materialProxies;
-
-    Debug.WriteLine(
-      $"Cache hit count {cacheHitCount} out of {objects.Count} ({(double)cacheHitCount / objects.Count})"
-    );
+    // NOTE: these are currently not used anywhere, so we could even skip them (?).
+    _rootObject[ProxyKeys.PARAMETER_DEFINITIONS] = _parameterDefinitionHandler.Definitions;
 
     return new RootObjectBuilderResult(_rootObject, results);
   }

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

@@ -0,0 +1,12 @@
+using Speckle.Connectors.DUI.Settings;
+
+namespace Speckle.Connectors.Revit.Operations.Send.Settings;
+
+public class SendParameterNullOrEmptyStringsSetting(bool value) : ICardSetting
+{
+  public string? Id { get; set; } = "nullemptyparams";
+  public string? Title { get; set; } = "Send null/empty parameters";
+  public string? Type { get; set; } = "boolean";
+  public List<string>? Enum { get; set; }
+  public object? Value { get; set; } = value;
+}

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

@@ -20,6 +20,7 @@ public class ToSpeckleSettingsManager : IToSpeckleSettingsManager
   // cache invalidation process run with ModelCardId since the settings are model specific
   private readonly Dictionary<string, DetailLevelType> _detailLevelCache = new();
   private readonly Dictionary<string, Transform?> _referencePointCache = new();
+  private readonly Dictionary<string, bool?> _sendNullParamsCache = new();
 
   public ToSpeckleSettingsManager(
     RevitContext revitContext,
@@ -44,9 +45,7 @@ fidelityString is not null
       {
         if (previousType != fidelity)
         {
-          var objectIds = modelCard.SendFilter != null ? modelCard.SendFilter.GetObjectIds() : [];
-          var unpackedObjectIds = _elementUnpacker.GetUnpackedElementIds(objectIds);
-          _sendConversionCache.EvictObjects(unpackedObjectIds);
+          EvictCacheForModelCard(modelCard);
         }
       }
       _detailLevelCache[modelCard.ModelCardId.NotNull()] = fidelity;
@@ -76,9 +75,7 @@ out ReferencePointType referencePoint
         // invalidate conversion cache if the transform has changed
         if (previousTransform != currentTransform)
         {
-          var objectIds = modelCard.SendFilter != null ? modelCard.SendFilter.GetObjectIds() : [];
-          var unpackedObjectIds = _elementUnpacker.GetUnpackedElementIds(objectIds);
-          _sendConversionCache.EvictObjects(unpackedObjectIds);
+          EvictCacheForModelCard(modelCard);
         }
       }
 
@@ -89,6 +86,29 @@ out ReferencePointType referencePoint
     throw new ArgumentException($"Invalid reference point value: {referencePointString}");
   }
 
+  public bool GetSendParameterNullOrEmptyStringsSetting(SenderModelCard modelCard)
+  {
+    var value = modelCard.Settings?.First(s => s.Id == "nullemptyparams").Value as bool?;
+    var returnValue = value != null && value.NotNull();
+    if (_sendNullParamsCache.TryGetValue(modelCard.ModelCardId.NotNull(), out bool? previousValue))
+    {
+      if (previousValue != returnValue)
+      {
+        EvictCacheForModelCard(modelCard);
+      }
+    }
+
+    _sendNullParamsCache[modelCard.ModelCardId] = returnValue;
+    return returnValue;
+  }
+
+  private void EvictCacheForModelCard(SenderModelCard modelCard)
+  {
+    var objectIds = modelCard.SendFilter != null ? modelCard.SendFilter.GetObjectIds() : [];
+    var unpackedObjectIds = _elementUnpacker.GetUnpackedElementIds(objectIds);
+    _sendConversionCache.EvictObjects(unpackedObjectIds);
+  }
+
   private Transform? GetTransform(RevitContext context, ReferencePointType referencePointType)
   {
     Transform? referencePointTransform = null;

Connectors/Revit/Speckle.Connectors.RevitShared/Speckle.Connectors.RevitShared.projitems

@@ -37,6 +37,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Operations\Receive\RevitHostObjectBuilder.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Operations\Receive\TransactionManager.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Operations\Send\RevitRootObjectBuilder.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Operations\Send\Settings\SendParameterNullOrEmptyStringsSetting.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Operations\Send\Settings\ToSpeckleSettingsManager.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Operations\Send\Settings\ReferencePointSetting.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Operations\Send\Settings\DetailLevelSetting.cs" />

Converters/Revit/Speckle.Converters.RevitShared.DependencyInjection/RevitConverterModule.cs

@@ -5,6 +5,7 @@
 using Speckle.Converters.RevitShared.Helpers;
 using Speckle.Converters.RevitShared.Services;
 using Speckle.Converters.RevitShared.Settings;
+using Speckle.Converters.RevitShared.ToSpeckle;
 
 namespace Speckle.Converters.RevitShared.DependencyInjection;
 
@@ -46,5 +47,8 @@ public void Load(SpeckleContainerBuilder builder)
     builder.AddScoped<ISlopeArrowExtractor, SlopeArrowExtractor>();
 
     builder.AddScoped<IRevitCategories, RevitCategories>();
+
+    builder.AddScoped<ParameterDefinitionHandler>();
+    builder.AddScoped<ParameterExtractor>();
   }
 }

Converters/Revit/Speckle.Converters.RevitShared.Tests/ModelCurveArrayToSpeckleConverterTests.cs

@@ -53,7 +53,7 @@ public void Convert()
     var units = "units";
     revitConversionContextStack
       .Setup(x => x.Current)
-      .Returns(new RevitConversionSettings(null!, DetailLevelType.Coarse, null, units));
+      .Returns(new RevitConversionSettings(null!, DetailLevelType.Coarse, null, units, false));
 
     var scaleLength = 2.2;
     scalingServiceToSpeckle.Setup(x => x.ScaleLength(2 + 3)).Returns(scaleLength);

Converters/Revit/Speckle.Converters.RevitShared/Extensions/ParameterExtensions.cs

@@ -2,6 +2,7 @@
 
 namespace Speckle.Converters.RevitShared.Extensions;
 
+[Obsolete("Will be removed in the near future")]
 public static class ParameterExtensions
 {
   /// <summary>
@@ -16,16 +17,14 @@ public static string GetInternalName(this DB.Parameter rp)
     {
       return rp.GUID.ToString();
     }
-    else
-    {
-      var def = (InternalDefinition)rp.Definition;
-      if (def.BuiltInParameter == BuiltInParameter.INVALID)
-      {
-        return def.Name;
-      }
 
-      return def.BuiltInParameter.ToString();
+    var def = (InternalDefinition)rp.Definition;
+    if (def.BuiltInParameter == BuiltInParameter.INVALID)
+    {
+      return def.Name;
     }
+
+    return def.BuiltInParameter.ToString();
   }
 
   public static BuiltInParameter? GetBuiltInParameter(this Parameter rp)

Converters/Revit/Speckle.Converters.RevitShared/Helpers/ParameterObjectAssigner.cs

@@ -6,8 +6,7 @@
 
 namespace Speckle.Converters.RevitShared.Helpers;
 
-// POC: rationalise whether this and ParameterObjectBuilder are sufficiently different??
-// did it go away?
+[Obsolete("Do not use this anymore. It's been replaced by the ParameterExtractor class.")]
 public sealed class ParameterObjectAssigner
 {
   private readonly ITypedConverter<Parameter, SOBR.Parameter> _paramConverter;
@@ -25,27 +24,31 @@ ILogger<ParameterObjectAssigner> logger
     _logger = logger;
   }
 
-  public void AssignParametersToBase(Element target, Base @base)
+#pragma warning disable IDE0060
+  [Obsolete("Do not use this anymore. It's been replaced by the ParameterExtractor class.")]
+  public void AssignParametersToBase(Element target, Base @base) // NOTE: commented out for ease of benchmarking (for now)
+#pragma warning restore IDE0060
   {
-    Dictionary<string, Parameter> instanceParameters = _parameterValueExtractor.GetAllRemainingParams(target);
-    ElementId elementId = target.GetTypeId();
-
-    Base paramBase = new();
-    AssignSpeckleParamToBaseObject(instanceParameters, paramBase);
-
-    // POC: Some elements can have an invalid element type ID, I don't think we want to continue here.
-    if (elementId != ElementId.InvalidElementId && target is not Level) //ignore type props of levels..!
-    {
-      var elementType = target.Document.GetElement(elementId);
-      // I don't think we should be adding the type parameters to the object like this
-      Dictionary<string, Parameter> typeParameters = _parameterValueExtractor.GetAllRemainingParams(elementType);
-      AssignSpeckleParamToBaseObject(typeParameters, paramBase, true);
-    }
-
-    if (paramBase.GetMembers(DynamicBaseMemberType.Dynamic).Count > 0)
-    {
-      @base["parameters"] = paramBase;
-    }
+    // return;
+    // Dictionary<string, Parameter> instanceParameters = _parameterValueExtractor.GetAllRemainingParams(target);
+    // ElementId elementId = target.GetTypeId();
+    //
+    // Base paramBase = new();
+    // AssignSpeckleParamToBaseObject(instanceParameters, paramBase);
+    //
+    // // POC: Some elements can have an invalid element type ID, I don't think we want to continue here.
+    // if (elementId != ElementId.InvalidElementId && target is not Level) //ignore type props of levels..!
+    // {
+    //   var elementType = target.Document.GetElement(elementId);
+    //   // I don't think we should be adding the type parameters to the object like this
+    //   Dictionary<string, Parameter> typeParameters = _parameterValueExtractor.GetAllRemainingParams(elementType);
+    //   AssignSpeckleParamToBaseObject(typeParameters, paramBase, true);
+    // }
+    //
+    // if (paramBase.GetMembers(DynamicBaseMemberType.Dynamic).Count > 0)
+    // {
+    //   @base["parameters"] = paramBase;
+    // }
   }
 
   private void AssignSpeckleParamToBaseObject(

Converters/Revit/Speckle.Converters.RevitShared/Helpers/RevitMaterialCacheSingleton.cs

@@ -8,6 +8,7 @@ namespace Speckle.Converters.RevitShared.Helpers;
 /// <para>
 /// Why is this needed? Because two reasons: send caching bypasses converter and revit conversions typically generate multiple display values per element. Ask dim for more and he might start crying.
 /// </para>
+/// TODO: this dude needs to be split into single responsability (render materials and material quantities), and removed from the context - as it's not needed for it to be there. It can be DI'ed as appropriate (see ParameterDefinitionHandler)
 /// </summary>
 public class RevitMaterialCacheSingleton
 {