Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CNX-9775 sending revit elements via dynamo #3495

Merged
merged 5 commits into from
Jul 1, 2024
Merged

CNX-9775 sending revit elements via dynamo #3495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c5f3e2
POC: nullable cache
oguzhankoral Jun 8, 2024
28241e2
Remove commented out lines
oguzhankoral Jun 14, 2024
4147323
Merge branch 'dev' into CNX-9775-Sending-Revit-elements-via-Dynamo
oguzhankoral Jun 14, 2024
4fb68e4
fix: Formatting
AlanRynne Jun 19, 2024
1c0c1ca
Merge branch 'dev' into CNX-9775-Sending-Revit-elements-via-Dynamo
oguzhankoral Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 76 additions & 48 deletions Objects/Converters/ConverterRevit/ConverterRevitShared/AllRevitCategories.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,45 @@ public IRevitCategoryInfo GetRevitCategoryInfo<T>(Base @base)
return elementType;
}

var matchingType = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>()
.GetAllObjects()
.Where(catInfo => catInfo.ElementTypeType == typeof(T) && catInfo.BuiltInCategories.Count == 0)
.FirstOrDefault();
IRevitCategoryInfo matchingType;

if (revitDocumentAggregateCache is null)
{
matchingType = elementType;
}
else
{
matchingType = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>()
.GetAllObjects()
.Where(catInfo => catInfo.ElementTypeType == typeof(T) && catInfo.BuiltInCategories.Count == 0)
.FirstOrDefault();
}

if (matchingType != null)
{
return matchingType;
}

var categoryInfo = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>()
.GetOrAdd(
typeof(T).Name,
() =>
{
return new RevitCategoryInfo(typeof(T).Name, null, typeof(T), new List<BuiltInCategory>());
},
out _
);
IRevitCategoryInfo categoryInfo;

if (revitDocumentAggregateCache is null)
{
categoryInfo = new RevitCategoryInfo(typeof(T).Name, null, typeof(T), new List<BuiltInCategory>());
}
else
{
categoryInfo = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>()
.GetOrAdd(
typeof(T).Name,
() =>
{
return new RevitCategoryInfo(typeof(T).Name, null, typeof(T), new List<BuiltInCategory>());
},
out _
);
}

return categoryInfo;
}
Expand Down Expand Up @@ -128,30 +146,26 @@ public IRevitCategoryInfo GetRevitCategoryInfo(string categoryName)
}

categoryName = CategoryNameFormatted(categoryName);
var revitCategoryInfoCache = revitDocumentAggregateCache.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>();

categoryInfo = revitCategoryInfoCache.TryGet(categoryName);
if (categoryInfo != null)
IRevitObjectCache<IRevitCategoryInfo> revitCategoryInfoCache;
if (revitDocumentAggregateCache is not null)
{
return categoryInfo;
}
revitCategoryInfoCache = revitDocumentAggregateCache.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>();

foreach (var info in revitCategoryInfoCache.GetAllObjects())
{
if (categoryName.IndexOf(info.CategoryName, StringComparison.OrdinalIgnoreCase) >= 0)
categoryInfo = revitCategoryInfoCache.TryGet(categoryName);
if (categoryInfo != null)
Comment on lines +155 to +156
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me a bit supicious... we were previously calling GetAllObjects instead of this and I'm not sure the reason why we did this.

Your change seems legit though... just wondering if we're missing something here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also my most suspicious place, regardless I believe we should test this with real version too, on both Revit and Dynamo to understand we are having side effect on anything. 🤞

{
return info;
return categoryInfo;
}
foreach (var alias in info.CategoryAliases)
else
{
if (categoryName.IndexOf(alias, StringComparison.OrdinalIgnoreCase) >= 0)
{
return info;
}
return SHC.Undefined;
}
}

return SHC.Undefined;
else
{
return SHC.Undefined;
}
Comment on lines +165 to +168
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be simpler if we flip the condition? could somewhat reduce nesting and make the logic a bit more linear.

The 2 return statements above this one could also be collapsed into a return categoryInfo ?? SHC.Undefined which I consider neater (super personal opinion here...)

Copy link
Member

@AlanRynne AlanRynne Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this turns into more than a 30 second refactor, disregard it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you were referencing to this body (from 156-163) right? Confused because your comment targeting to 166-169

if (categoryInfo != null)
      {
        return categoryInfo;
      }
      else
      {
        return SHC.Undefined;
      }

}
#endregion

Expand All @@ -173,29 +187,43 @@ public IRevitCategoryInfo GetRevitCategoryInfo(string categoryName)
// pre 2.16 we're passing the "category" string
else
{
var revitCat = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<Category>()
.TryGet(unformattedCatName);

if (revitCat == null)
if (revitDocumentAggregateCache is null)
{
return null;
}
else
{
var revitCat = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<Category>()
.TryGet(unformattedCatName);

bic = Categories.GetBuiltInCategory(revitCat);
formattedName = CategoryNameFormatted(unformattedCatName);
if (revitCat == null)
{
return null;
}

bic = Categories.GetBuiltInCategory(revitCat);
formattedName = CategoryNameFormatted(unformattedCatName);
}
}

return revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>()
.GetOrAdd(
formattedName,
() =>
{
return new RevitCategoryInfo(formattedName, null, null, new List<BuiltInCategory> { bic });
},
out _
);
if (revitDocumentAggregateCache is null)
{
return new RevitCategoryInfo(formattedName, null, null, new List<BuiltInCategory> { bic });
}
else
{
return revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<IRevitCategoryInfo>()
.GetOrAdd(
formattedName,
() =>
{
return new RevitCategoryInfo(formattedName, null, null, new List<BuiltInCategory> { bic });
},
out _
);
}
}

private static string CategoryNameFormatted(string name)
Expand Down
76 changes: 45 additions & 31 deletions Objects/Converters/ConverterRevit/ConverterRevitShared/ConversionUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,39 +390,46 @@ private Parameter ParameterToSpeckle(
#else
ForgeTypeId unitTypeId = null;
#endif
ParameterToSpeckleData paramData;

// Local function to create ParameterToSpeckleData
ParameterToSpeckleData CreateParamData()
{
Comment on lines +395 to +397
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why local function? 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😓 I asked this myself too while writing.. We use creating param data under both condition of revitDocumentAggregateCache is null or not. And this param data parameters come from preprocessor directives as below, then it was more painful to extract this as private global function..

#if REVIT2020
    DisplayUnitType unitTypeId = default;
#else
    ForgeTypeId unitTypeId = null;
#endif

var definition = rp.Definition;
var newParamData = new ParameterToSpeckleData()
{
Definition = definition,
InternalName = paramInternalName,
IsReadOnly = rp.IsReadOnly,
IsShared = rp.IsShared,
IsTypeParameter = isTypeParameter,
Name = definition.Name,
UnitType = definition.GetUnityTypeString(),
};
if (rp.StorageType == StorageType.Double)
{
unitTypeId = rp.GetUnitTypeId();
newParamData.UnitsSymbol = GetSymbolUnit(rp, definition, unitTypeId);
newParamData.ApplicationUnits =
unitsOverride != null ? UnitsToNative(unitsOverride).ToUniqueString() : unitTypeId.ToUniqueString();
}
return newParamData;
}

// The parameter definitions are cached using the ParameterToSpeckleData struct
// This is done because in the case of type and instance parameter there is lots of redundant data that needs to be extracted from the Revit DB
// Caching noticeably speeds up the send process
// TODO : could add some generic getOrAdd overloads to avoid creating closures
var paramData = revitDocumentAggregateCache
.GetOrInitializeEmptyCacheOfType<ParameterToSpeckleData>(out _)
.GetOrAdd(
paramInternalName,
() =>
{
var definition = rp.Definition;
var newParamData = new ParameterToSpeckleData()
{
Definition = definition,
InternalName = paramInternalName,
IsReadOnly = rp.IsReadOnly,
IsShared = rp.IsShared,
IsTypeParameter = isTypeParameter,
Name = definition.Name,
UnitType = definition.GetUnityTypeString(),
};
if (rp.StorageType == StorageType.Double)
{
unitTypeId = rp.GetUnitTypeId();
newParamData.UnitsSymbol = GetSymbolUnit(rp, definition, unitTypeId);
newParamData.ApplicationUnits =
unitsOverride != null ? UnitsToNative(unitsOverride).ToUniqueString() : unitTypeId.ToUniqueString();
}
return newParamData;
},
out _
);
if (revitDocumentAggregateCache is null)
{
paramData = CreateParamData();
}
else
{
paramData = revitDocumentAggregateCache
.GetOrInitializeEmptyCacheOfType<ParameterToSpeckleData>(out _)
.GetOrAdd(paramInternalName, CreateParamData, out _);
}

return paramData.GetParameterObjectWithValue(rp.GetValue(paramData.Definition, unitTypeId));
}
Expand Down Expand Up @@ -450,9 +457,16 @@ ForgeTypeId unitTypeId
return null;
}

return revitDocumentAggregateCache
.GetOrInitializeEmptyCacheOfType<string>(out _)
.GetOrAdd(unitTypeId.ToUniqueString(), () => unitTypeId.GetSymbol(), out _);
if (revitDocumentAggregateCache is null)
{
return unitTypeId.GetSymbol();
}
else
{
return revitDocumentAggregateCache
.GetOrInitializeEmptyCacheOfType<string>(out _)
.GetOrAdd(unitTypeId.ToUniqueString(), () => unitTypeId.GetSymbol(), out _);
}
}

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion Objects/Converters/ConverterRevit/ConverterRevitShared/ConverterRevit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#nullable enable
using System;
using System.Collections.Generic;
using System.Linq;
Expand Down Expand Up @@ -109,7 +110,7 @@ public ConverterRevit()
Report.Log($"Using converter: {Name} v{ver}");
}

private IRevitDocumentAggregateCache revitDocumentAggregateCache;
private IRevitDocumentAggregateCache? revitDocumentAggregateCache;
private IConvertedObjectsCache<Base, Element> receivedObjectsCache;
private TransactionManager transactionManager;

Expand Down
36 changes: 25 additions & 11 deletions Objects/Converters/ConverterRevit/ConverterRevitShared/PartialClasses/ConvertWall.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,19 +264,33 @@ private IEnumerable<ElementId> GetSubsetOfElementsInView(BuiltInCategory categor
return children;
}

var allSubelementsInView = revitDocumentAggregateCache
.GetOrInitializeEmptyCacheOfType<HashSet<ElementId>>(out _)
.GetOrAdd(
category.ToString(),
() =>
{
using var filter = new ElementCategoryFilter(category);
using var collector = new FilteredElementCollector(Doc, ViewSpecificOptions.View.Id);
var allSubelementsInView = new HashSet<ElementId>();

if (revitDocumentAggregateCache is null)
{
var filter = new ElementCategoryFilter(category);
var collector = new FilteredElementCollector(Doc, ViewSpecificOptions.View.Id);

return new HashSet<ElementId>(collector.WhereElementIsNotElementType().WherePasses(filter).ToElementIds());
},
out _
allSubelementsInView = new HashSet<ElementId>(
collector.WhereElementIsNotElementType().WherePasses(filter).ToElementIds()
);
}
else
{
allSubelementsInView = revitDocumentAggregateCache
.GetOrInitializeEmptyCacheOfType<HashSet<ElementId>>(out _)
.GetOrAdd(
category.ToString(),
() =>
{
using var filter = new ElementCategoryFilter(category);
using var collector = new FilteredElementCollector(Doc, ViewSpecificOptions.View.Id);

return new HashSet<ElementId>(collector.WhereElementIsNotElementType().WherePasses(filter).ToElementIds());
},
out _
);
}

return children.Where(allSubelementsInView.Contains);
}
Expand Down
6 changes: 6 additions & 0 deletions Objects/Converters/ConverterRevit/ConverterRevitShared/RevitElementTypeUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,12 @@ public void SetElementFamily(Base @base, string family)
appObj.Update(logItem: $"Could not find valid incoming type for element of type \"{element.speckle_type}\"");
}
var typeInfo = AllCategories.GetRevitCategoryInfo<T>(element);

if (revitDocumentAggregateCache is null)
{
return default;
}
Comment on lines +144 to +147
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe? This means that it will always return null if the cache is not set, which is not immediately obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic was around this as below so I thought it is safe to return immediately if even cache is null....

var types = revitDocumentAggregateCache
      .GetOrInitializeWithDefaultFactory<List<ElementType>>()
      .GetOrAddGroupOfTypes(typeInfo);

    if (!types.Any())
    {
      var name = typeof(T).Name;
      if (element["category"] is string category && !string.IsNullOrWhiteSpace(category))
      {
        name = category;
      }

      appObj.Update(
        status: ApplicationObject.State.Failed,
        logItem: $"Could not find any loaded family to use for category {name}."
      );

      return default;
    }


var types = revitDocumentAggregateCache
.GetOrInitializeWithDefaultFactory<List<ElementType>>()
.GetOrAddGroupOfTypes(typeInfo);
Expand Down