From c818f48a47380c31d9fe24e0e970d26599388d61 Mon Sep 17 00:00:00 2001 From: Thomas Krijnen Date: Sun, 9 Aug 2026 09:14:41 +0200 Subject: [PATCH] Own completed iterator results uniquely Generated with the assistance of an AI coding tool. --- src/ifcgeom/iterator.cpp | 111 +++++++++--------- src/ifcgeom/iterator.h | 31 +++-- .../test/test_create_shape.py | 16 +++ src/ifcwrap/IfcGeomWrapper.i | 2 + 4 files changed, 90 insertions(+), 70 deletions(-) diff --git a/src/ifcgeom/iterator.cpp b/src/ifcgeom/iterator.cpp index 7d47288562..882d30db52 100644 --- a/src/ifcgeom/iterator.cpp +++ b/src/ifcgeom/iterator.cpp @@ -51,7 +51,7 @@ bool ifcopenshell::geom::iterator::initialize() { return std::make_pair(prod, ifcopenshell::geom::taxonomy::cast(prod_item)->matrix); }); } - tasks_.push_back(res); + tasks_.push_back(std::move(res)); } if (settings_.get().get() && settings_.get().get()) { @@ -182,8 +182,16 @@ void ifcopenshell::geom::iterator::process_finished_rep(geometry_conversion_resu std::lock_guard lk(element_ready_mutex_); - all_processed_elements_.insert(all_processed_elements_.end(), rep->elements.begin(), rep->elements.end()); - all_processed_native_elements_.insert(all_processed_native_elements_.end(), rep->breps.begin(), rep->breps.end()); + all_processed_elements_.insert( + all_processed_elements_.end(), + std::make_move_iterator(rep->elements.begin()), + std::make_move_iterator(rep->elements.end())); + all_processed_native_elements_.insert( + all_processed_native_elements_.end(), + std::make_move_iterator(rep->native_elements.begin()), + std::make_move_iterator(rep->native_elements.end())); + rep->elements.clear(); + rep->native_elements.clear(); if (!task_result_ptr_initialized) { task_result_iterator_ = all_processed_elements_.begin(); @@ -382,23 +390,29 @@ void ifcopenshell::geom::iterator::create_element_(ifcopenshell::geom::converter kernel_logger.set_product(product); - ifcopenshell::geom::native_element* brep = static_cast(create_processed_element_([kernel, settings, product, place, rep]() { + std::unique_ptr brep(static_cast(create_processed_element_([kernel, settings, product, place, rep]() { return kernel->create_brep_for_representation_and_product(rep->item, product, place); - })); + }))); if (!brep) { kernel_logger.set_product(std::optional{}); return; } - auto elem = process_based_on_settings(settings, brep, kernel_logger); + auto* brep_for_reuse = brep.get(); + std::unique_ptr elem; + if (settings.get().get() == ifcopenshell::geom::settings::NATIVE) { + elem = std::move(brep); + } else { + elem = process_based_on_settings(settings, brep.get(), kernel_logger); + } if (!elem) { kernel_logger.set_product(std::optional{}); return; } - rep->breps = { brep }; - rep->elements = { elem }; + rep->native_elements.push_back(std::move(brep)); + rep->elements.push_back(std::move(elem)); for (auto it = rep->products.begin() + 1; it != rep->products.end(); ++it) { const auto& p = *it; @@ -407,14 +421,23 @@ void ifcopenshell::geom::iterator::create_element_(ifcopenshell::geom::converter kernel_logger.set_product(product2); - ifcopenshell::geom::native_element* brep2 = static_cast(create_processed_element_([kernel, settings, product2, place2, brep]() { - return kernel->create_brep_for_processed_representation(product2, place2, brep); - })); + std::unique_ptr brep2(static_cast(create_processed_element_([kernel, settings, product2, place2, brep_for_reuse]() { + return kernel->create_brep_for_processed_representation(product2, place2, brep_for_reuse); + }))); if (brep2) { - auto elem2 = process_based_on_settings(settings, brep2, kernel_logger, dynamic_cast(elem)); + std::unique_ptr elem2; + if (settings.get().get() == ifcopenshell::geom::settings::NATIVE) { + elem2 = std::move(brep2); + } else { + elem2 = process_based_on_settings( + settings, + brep2.get(), + kernel_logger, + dynamic_cast(rep->elements.front().get())); + } if (elem2) { - rep->breps.push_back(brep2); - rep->elements.push_back(elem2); + rep->native_elements.push_back(std::move(brep2)); + rep->elements.push_back(std::move(elem2)); } } } @@ -422,30 +445,28 @@ void ifcopenshell::geom::iterator::create_element_(ifcopenshell::geom::converter kernel_logger.set_product(std::optional{}); } -ifcopenshell::geom::element* ifcopenshell::geom::iterator::process_based_on_settings(ifcopenshell::geom::settings settings, ifcopenshell::geom::native_element* elem, ifcopenshell::logger& logger, ifcopenshell::geom::triangulation_element* previous) +std::unique_ptr ifcopenshell::geom::iterator::process_based_on_settings(ifcopenshell::geom::settings settings, ifcopenshell::geom::native_element* elem, ifcopenshell::logger& logger, ifcopenshell::geom::triangulation_element* previous) { if (settings.get().get() == ifcopenshell::geom::settings::SERIALIZED) { try { - return new ifcopenshell::geom::serialized_element(*elem); + return std::make_unique(*elem); } catch (...) { logger.message(ifcopenshell::logger::LOG_ERROR, "GEO", 54, "Getting a serialized element from model failed."); return nullptr; } } else if (settings.get().get() == ifcopenshell::geom::settings::TRIANGULATED) { - return create_processed_element_([elem, previous, &logger]() { - try { - if (!previous) { - return new triangulation_element(*elem); - } else { - return new triangulation_element(*elem, previous->geometry_pointer()); - } - } catch (...) { - logger.message(ifcopenshell::logger::LOG_ERROR, "GEO", 55, "Getting a triangulation element from model failed."); + try { + if (!previous) { + return std::make_unique(*elem); + } else { + return std::make_unique(*elem, previous->geometry_pointer()); } - return (triangulation_element*)nullptr; - }); + } catch (...) { + logger.message(ifcopenshell::logger::LOG_ERROR, "GEO", 55, "Getting a triangulation element from model failed."); + return nullptr; + } } else { - return elem; + throw std::runtime_error("native iterator output must be moved directly"); } } @@ -511,14 +532,8 @@ express::base ifcopenshell::geom::iterator::next() { { std::lock_guard lock(element_ready_mutex_); - auto* element = *task_result_iterator_; - auto* native_element = *native_task_result_iterator_; - *task_result_iterator_ = nullptr; - *native_task_result_iterator_ = nullptr; - if (native_element != element) { - delete native_element; - } - delete element; + task_result_iterator_->reset(); + native_task_result_iterator_->reset(); } if (num_threads_ != 1) { @@ -534,7 +549,7 @@ express::base ifcopenshell::geom::iterator::next() { task_result_iterator_++; native_task_result_iterator_++; - return (*task_result_iterator_)->product(); + return task_result_iterator_->get()->product(); } else { // Increment the iterator over the list of products using the current // shape representation @@ -552,7 +567,7 @@ express::base ifcopenshell::geom::iterator::next() { task_result_iterator_++; native_task_result_iterator_++; - return (*task_result_iterator_)->product(); + return task_result_iterator_->get()->product(); } } @@ -564,14 +579,10 @@ std::unique_ptr ifcopenshell::geom::iterator::get() std::unique_ptr ret; { std::lock_guard lock(element_ready_mutex_); - ret.reset(*task_result_iterator_); + ret = std::move(*task_result_iterator_); if (!ret) { throw std::runtime_error("current element has already been retrieved"); } - *task_result_iterator_ = nullptr; - if (*native_task_result_iterator_ == ret.get()) { - *native_task_result_iterator_ = nullptr; - } } // If we want to organize the element considering their hierarchy @@ -861,19 +872,5 @@ ifcopenshell::geom::iterator::~iterator() { delete k; } - if (task_result_ptr_initialized) { - std::lock_guard lock(element_ready_mutex_); - while (task_result_iterator_ != all_processed_elements_.end()) { - auto* element = *task_result_iterator_; - auto* native_element = *native_task_result_iterator_; - if (native_element != element) { - delete native_element; - } - delete element; - task_result_iterator_++; - native_task_result_iterator_++; - } - } - delete converter_; } diff --git a/src/ifcgeom/iterator.h b/src/ifcgeom/iterator.h index 45d8987d1e..0dd99f5ec0 100644 --- a/src/ifcgeom/iterator.h +++ b/src/ifcgeom/iterator.h @@ -83,6 +83,12 @@ namespace ifcopenshell::geom { struct IFC_GEOM_API geometry_conversion_result { + geometry_conversion_result() = default; + geometry_conversion_result(const geometry_conversion_result&) = delete; + geometry_conversion_result& operator=(const geometry_conversion_result&) = delete; + geometry_conversion_result(geometry_conversion_result&&) noexcept = default; + geometry_conversion_result& operator=(geometry_conversion_result&&) noexcept = default; + int index; // For NoParallelMapping==true @@ -93,8 +99,8 @@ namespace ifcopenshell::geom { express::base representation; std::vector products_2; - std::vector breps; - std::vector elements; + std::vector> native_elements; + std::vector> elements; bool is_parallel() const { return !!representation; @@ -112,11 +118,11 @@ namespace ifcopenshell::geom { std::vector tasks_; std::vector::iterator task_iterator_; - std::list all_processed_elements_; - std::list all_processed_native_elements_; + std::list> all_processed_elements_; + std::list> all_processed_native_elements_; - std::list::iterator task_result_iterator_; - std::list::iterator native_task_result_iterator_; + std::list>::iterator task_result_iterator_; + std::list>::iterator native_task_result_iterator_; std::mutex element_ready_mutex_; bool task_result_ptr_initialized = false; @@ -167,7 +173,7 @@ namespace ifcopenshell::geom { ifcopenshell::geom::settings settings, geometry_conversion_result* rep); - ifcopenshell::geom::element* process_based_on_settings( + std::unique_ptr process_based_on_settings( ifcopenshell::geom::settings settings, ifcopenshell::geom::native_element* elem, ifcopenshell::logger& logger, @@ -297,16 +303,15 @@ namespace ifcopenshell::geom { std::unique_ptr get_native() { validate_iterator_state(); + if (settings_.get().get() == ifcopenshell::geom::settings::NATIVE) { + throw std::runtime_error("native output is returned by get(); use get() instead"); + } std::lock_guard lock(element_ready_mutex_); - auto* result = *native_task_result_iterator_; + auto result = std::move(*native_task_result_iterator_); if (!result) { throw std::runtime_error("current native element has already been retrieved"); } - *native_task_result_iterator_ = nullptr; - if (*task_result_iterator_ == result) { - *task_result_iterator_ = nullptr; - } - return std::unique_ptr(result); + return result; } std::unique_ptr get_object(int id); diff --git a/src/ifcopenshell-python/test/test_create_shape.py b/src/ifcopenshell-python/test/test_create_shape.py index 218b95daa2..4118685dde 100644 --- a/src/ifcopenshell-python/test/test_create_shape.py +++ b/src/ifcopenshell-python/test/test_create_shape.py @@ -233,6 +233,22 @@ def test_iterator_get_native_transfers_ownership(num_threads): assert element.id == element_id +@pytest.mark.parametrize("num_threads", [1, 2]) +def test_iterator_native_output_is_retrieved_with_get(num_threads): + settings = ifcopenshell.geom.settings() + settings.set("iterator-output", W.NATIVE) + iterator = ifcopenshell.geom.iterator(settings, fn, num_threads) + assert iterator.initialize() + + with pytest.raises(RuntimeError, match=r"use get\(\) instead"): + iterator.get_native() + + element = iterator.get() + element_id = element.id + iterator.next() + assert element.id == element_id + + def test_logging(): assert ifcopenshell.logger logger = ifcopenshell.logger() diff --git a/src/ifcwrap/IfcGeomWrapper.i b/src/ifcwrap/IfcGeomWrapper.i index 1619412d66..9322d7340a 100644 --- a/src/ifcwrap/IfcGeomWrapper.i +++ b/src/ifcwrap/IfcGeomWrapper.i @@ -24,6 +24,8 @@ %ignore boost::hash_value; %ignore ifcopenshell::geom::native_element::geometry_pointer; %ignore ifcopenshell::geom::triangulation_element::geometry_pointer; +%ignore ifcopenshell::geom::geometry_conversion_result::native_elements; +%ignore ifcopenshell::geom::geometry_conversion_result::elements; // This is only used for RGB colours, hence the size of 3 %typemap(out) const double* {