diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 29580698d..822d8977c 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -20,7 +20,7 @@ jobs: dotnet-version: 8.0.4xx # Align with global.json (including roll forward rules) - name: Cache Nuget - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: ~/.nuget/packages key: ${{ runner.os }}-nuget-${{ hashFiles('**/packages.lock.json') }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index da1caa474..16206296c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -26,7 +26,7 @@ jobs: dotnet-version: 8.0.4xx # Align with global.json (including roll forward rules) - name: Cache Nuget - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: ~/.nuget/packages key: ${{ runner.os }}-nuget-${{ hashFiles('**/packages.lock.json') }} @@ -35,7 +35,7 @@ jobs: run: ./build.ps1 zip - name: ⬆️ Upload artifacts - uses: actions/upload-artifact@v5 + uses: actions/upload-artifact@v6 with: name: output-${{ env.SEMVER }} path: output/*.* diff --git a/Converters/Revit/Speckle.Converters.RevitShared/Helpers/DisplayValueExtractor.cs b/Converters/Revit/Speckle.Converters.RevitShared/Helpers/DisplayValueExtractor.cs index aecc04a14..b465e7f74 100644 --- a/Converters/Revit/Speckle.Converters.RevitShared/Helpers/DisplayValueExtractor.cs +++ b/Converters/Revit/Speckle.Converters.RevitShared/Helpers/DisplayValueExtractor.cs @@ -4,6 +4,7 @@ using Speckle.Converters.Common.ToSpeckle; using Speckle.Converters.RevitShared.Extensions; using Speckle.Converters.RevitShared.Services; using Speckle.Converters.RevitShared.Settings; +using Speckle.Converters.RevitShared.ToSpeckle; using Speckle.DoubleNumerics; using Speckle.Objects; using Speckle.Sdk; @@ -164,11 +165,11 @@ public sealed class DisplayValueExtractor } /// - /// Processes collections of different geometry types and converts them to display values. - /// Extracted as a common method to reduce code duplication between regular geometry processing and special cases like rebar. + /// Converts sorted geometry into DisplayValueResults . /// /// - /// Essentially all the ensuing steps after the common get_Geometry element method + /// Applies localToWorld only to curves, points, polylines. + /// Meshes remain in symbol space to generate correct instance proxies and avoid duplicates. /// private List ProcessGeometryCollections( DB.Element element, @@ -176,50 +177,35 @@ public sealed class DisplayValueExtractor DB.Transform? localToWorld ) { - // handle all solids and meshes by their material var meshesByMaterial = GetMeshesByMaterial(collections.Meshes, collections.Solids); - List displayMeshes = _meshByMaterialConverter.Convert( + var displayMeshes = _meshByMaterialConverter.Convert( (meshesByMaterial, element.Id, ShouldSetElementDisplayToTransparent(element)) ); List displayValue = new(collections.TotalCount); - Matrix4x4? matrix = localToWorld is not null ? TransformToMatrix(localToWorld) : null; - foreach (SOG.Mesh mesh in displayMeshes) + foreach (var mesh in displayMeshes) { + // if we have a transform, keep mesh in symbol space and attach transform displayValue.Add( - matrix.HasValue - ? DisplayValueResult.WithTransform(mesh, matrix.Value) + localToWorld != null + ? DisplayValueResult.WithTransform(mesh, TransformToMatrix(localToWorld)) : DisplayValueResult.WithoutTransform(mesh) ); } - // transform curves, polylines, and points to world coordinates before conversion. - // Unlike meshes/solids which are proxified with transform matrices, these geometry - // types must have their final world coordinates baked directly into their geometry. foreach (var curve in collections.Curves) { - if (localToWorld is not null) - { - using var transformedCurve = curve.CreateTransformed(localToWorld); - displayValue.Add(DisplayValueResult.WithoutTransform(GetCurveDisplayValue(transformedCurve))); - } - else - { - displayValue.Add(DisplayValueResult.WithoutTransform(GetCurveDisplayValue(curve))); - } + var transformedCurve = localToWorld != null ? curve.CreateTransformed(localToWorld) : curve; + displayValue.Add(DisplayValueResult.WithoutTransform(GetCurveDisplayValue(transformedCurve))); } - // Note: Creating new polyline/point instances for transformation isn't ideal for perf, - // but Revit API doesn't provide in-place transform methods. Trade-off is acceptable since - // family instances typically don't have massive numbers of raw polylines/points in their geometry. foreach (var polyline in collections.Polylines) { - if (localToWorld is not null) + if (localToWorld != null) { - var coords = polyline.GetCoordinates(); - var transformedCoords = coords.Select(coord => localToWorld.OfPoint(coord)).ToList(); - using var transformedPolyline = DB.PolyLine.Create(transformedCoords); + var coords = polyline.GetCoordinates().Select(p => localToWorld.OfPoint(p)).ToList(); + using var transformedPolyline = DB.PolyLine.Create(coords); displayValue.Add(DisplayValueResult.WithoutTransform(_polylineConverter.Convert(transformedPolyline))); } else @@ -230,7 +216,7 @@ public sealed class DisplayValueExtractor foreach (var point in collections.Points) { - if (localToWorld is not null) + if (localToWorld != null) { using var transformedPoint = DB.Point.Create(localToWorld.OfPoint(point.Coord)); displayValue.Add(DisplayValueResult.WithoutTransform(_pointConverter.Convert(transformedPoint))); @@ -330,23 +316,17 @@ public sealed class DisplayValueExtractor }; /// - /// According to the remarks on the GeometryInstance class in the RevitAPIDocs, - /// https://www.revitapidocs.com/2024/fe25b14f-5866-ca0f-a660-c157484c3a56.htm, - /// a family instance geometryElement should have a top-level geometry instance when the symbol - /// does not have modified geometry (the docs say that modified geometry will not have a geom instance, - /// however in my experience, all family instances have a top-level geom instance, but if the family instance - /// is modified, then the geom instance won't contain any geometry.) - /// - /// This remark also leads me to think that a family instance will not have top-level solids and geom instances. - /// We are logging cases where this is not true. - /// - /// Note: this is basically a geometry unpacker for all types of geometry + /// Sorts element geometry into solids, meshes, curves, polylines, points. /// + /// + /// GeometryInstances are processed via GetSymbolGeometry() with accumulated transforms, + /// keeping meshes in symbol space and avoiding double transforms. + /// private void SortGeometry( DB.Element element, GeometryCollections collections, DB.GeometryElement geom, - DB.Transform? worldToLocal + DB.Transform? accumulatedTransform ) { foreach (DB.GeometryObject geomObj in geom) @@ -359,56 +339,62 @@ public sealed class DisplayValueExtractor switch (geomObj) { case DB.Solid solid: - // skip invalid solid if (solid.Faces.Size == 0) { continue; } - if (worldToLocal is not null) + if (accumulatedTransform != null) { - solid = DB.SolidUtils.CreateTransformed(solid, worldToLocal); + // apply transform to bring solid into document/world space + // only apply once to avoid double-transform bugs + solid = DB.SolidUtils.CreateTransformed(solid, accumulatedTransform); } + collections.Solids.Add(solid); break; case DB.Mesh mesh: - if (worldToLocal is not null) + if (accumulatedTransform != null) { - mesh = mesh.get_Transformed(worldToLocal); + // apply accumulated transform to mesh + // prevents geometry from being incorrectly transformed later [Ref: CNX-2875] + mesh = mesh.get_Transformed(accumulatedTransform); } + collections.Meshes.Add(mesh); break; - // curves, polylines, and points are transformed to world space in ProcessGeometryCollections, - // not here, because they cannot be proxified like meshes. case DB.Curve curve: + // curves are stored as-is; transforms are applied later in ProcessGeometryCollections collections.Curves.Add(curve); break; case DB.PolyLine polyline: + // polylines also handled later during display value processing collections.Polylines.Add(polyline); break; case DB.Point point: + // points remain in local space; transformed later if needed collections.Points.Add(point); break; case DB.GeometryInstance instance: - // element transforms should not be carried down into nested geometryInstances. - // Nested geomInstances should have their geom retrieved with GetInstanceGeom, not GetSymbolGeom - if (worldToLocal == null) //see remark on method for why this is safe to do... - { - SortGeometry(element, collections, instance.GetInstanceGeometry(), null); - } - else - { - SortGeometry(element, collections, instance.GetSymbolGeometry(), null); - } + // GeometryInstance.Transform: symbol → parent coordinate system + // multiply with accumulatedTransform to handle nested instances + var instanceTransform = instance.Transform; + var nextTransform = + accumulatedTransform != null ? accumulatedTransform.Multiply(instanceTransform) : instanceTransform; + + // always use symbol geometry, never GetInstanceGeometry() [Ref: CNX-2875] + SortGeometry(element, collections, instance.GetSymbolGeometry(), nextTransform); break; case DB.GeometryElement geometryElement: - SortGeometry(element, collections, geometryElement, null); + // raw GeometryElement: it has no transform of its own + // pass accumulatedTransform from parent if present + SortGeometry(element, collections, geometryElement, accumulatedTransform); break; } } @@ -500,6 +486,26 @@ public sealed class DisplayValueExtractor return currentOptions; } + // cable trays (and fittings) are MEP system families whose geometry detail is effectively view-driven. + // So, we've seen that, Options.DetailLevel is ignored by get_Geometry() for these categories unless a View is + // explicitly supplied, and Revit will always return a medium-detail representation otherwise [Ref: CNX-2735] + // We force extraction through the active view here (if there is one!) + if ( + elementBuiltInCategory == DB.BuiltInCategory.OST_CableTray + || elementBuiltInCategory == DB.BuiltInCategory.OST_CableTrayFitting + ) + { + try + { + return new DB.Options { View = _converterSettings.Current.Document.NotNull().ActiveView }; + } + catch (Exception ex) when (!ex.IsFatal()) + { + // linked docs or invalid view context – fall back to non-view-specific options + return currentOptions; + } + } + // NOTE: On steel elements. This is an incomplete solution. // If steel element proxies will be sucked in via category selection, and they are not visible in the current view, they will not be extracted out. // I'm inclined to go with this as a semi-permanent limitation. See: