mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-08-11 02:02:22 +00:00
Bonsai/ifcopenshell: catch inconsistent winding, guard Blender volume path (#6125)
PR #8503 (already open on this branch) added an edge parity check to util.shape.get_volume: every undirected edge of a triangulated mesh must be shared by exactly two triangles, otherwise the divergence theorem sum is meaningless and nan is returned instead of a wrong number. That check matches Moult's "edge face connectivity" suggestion in the issue thread and is verified to turn the reported slabs' volume from 17.71 m3 (against a 1.82 m3 bounding box) into nan. It only implements half of aothms' actual suggestion though. He asked for two checks: unoriented edges used exactly twice, and oriented (directed) edges used at most once. The existing check only does the first. A mesh can pass an unordered edge count check while still having two triangles that share an edge with the same winding direction instead of opposite ones (for example one flipped normal on an otherwise closed shape). That is a real non-manifold defect that silently corrupts the volume sum instead of producing an error: a unit cube with one triangle's winding flipped in place still passes the count == 2 check per edge, but get_volume returns 0.667 instead of 1.0. is_manifold now also tracks directed edge usage and requires it to be exactly one, catching this case and returning nan for it too, verified against both the flipped cube and the original reported slabs. Separately, the issue reports the volume is "wildly off via ifcopenshell and blender", but PR #8503 only touches the ifcopenshell.util.shape path used by the IfcOpenShell qto calculator. Bonsai's own Blender qto calculator (bonsai.bim.module.qto.calculator) computes volumes with bmesh.calc_volume() on the live mesh, which has the identical divergence theorem assumption and the identical bug, entirely unguarded. Verified live in headless Blender: an open cube (one face deleted) reports 6.667 instead of erroring, and a cube with one face's winding flipped reports 5.333. get_net_volume and get_gross_volume now check bmesh edge.is_contiguous (manifold and matching winding) and return None instead, which the existing qto pipeline already treats as "skip this quantity". The dependent get_space_net_volume, get_gross_weight and get_net_weight functions are updated to propagate None instead of computing arithmetic on it. Verified with the reporter's attached slabs-only.ifc (three non-manifold IfcPolygonalFaceSet slabs): get_volume returns nan for all three through the real ifc5d.qto.quantify() dispatch, so no Volume quantity is written, matching Bruno's request to skip volume calculation for non-manifold tessellations entirely. Regression checked with a closed manifold cube through both calculators (unchanged, correct volume). Added unit tests for is_manifold/get_volume (util/shape.py) covering closed, open and inconsistent winding meshes, and a Bonsai qto regression test building a non-manifold wall mesh and asserting its Volume quantities are omitted rather than wrong. Black and ruff pass on all touched files. Generated with the assistance of an AI coding tool.
This commit is contained in:
@@ -451,19 +451,24 @@ def get_net_ceiling_area(obj: bpy.types.Object) -> float:
|
||||
return total_net_ceiling_area
|
||||
|
||||
|
||||
def get_space_net_volume(obj: bpy.types.Object) -> float:
|
||||
def get_space_net_volume(obj: bpy.types.Object) -> Union[float, None]:
|
||||
decompositions = get_obj_decompositions(obj)
|
||||
if not decompositions:
|
||||
return get_gross_volume(obj)
|
||||
|
||||
total_space_net_volume = get_gross_volume(obj)
|
||||
if total_space_net_volume is None:
|
||||
return None
|
||||
|
||||
for decomposition in decompositions:
|
||||
decomposition_type = decomposition.get_info()["type"]
|
||||
if decomposition_type == "IfcWall" or decomposition_type == "IfcColumn":
|
||||
decomposition_obj = tool.Ifc.get_object(decomposition)
|
||||
assert isinstance(decomposition_obj, bpy.types.Object)
|
||||
total_space_net_volume -= get_net_volume(decomposition_obj)
|
||||
decomposition_net_volume = get_net_volume(decomposition_obj)
|
||||
if decomposition_net_volume is None:
|
||||
return None
|
||||
total_space_net_volume -= decomposition_net_volume
|
||||
|
||||
return total_space_net_volume
|
||||
|
||||
@@ -578,16 +583,32 @@ def is_polygon_in_vg(polygon: bpy.types.MeshPolygon, vertices_in_vg: list[int])
|
||||
return True
|
||||
|
||||
|
||||
def get_net_volume(o: bpy.types.Object) -> float:
|
||||
def is_manifold(bm: bmesh.types.BMesh) -> bool:
|
||||
"""Checks whether a bmesh is a closed, consistently oriented manifold.
|
||||
|
||||
calc_volume assumes a watertight mesh with matching face winding across
|
||||
every edge. An open mesh or one with inconsistent winding gives a
|
||||
meaningless result, see https://github.com/IfcOpenShell/IfcOpenShell/issues/6125.
|
||||
|
||||
:param bm: A bmesh instance.
|
||||
:return: ``True`` if every edge is shared by exactly two faces with matching winding.
|
||||
"""
|
||||
return all(edge.is_contiguous for edge in bm.edges)
|
||||
|
||||
|
||||
def get_net_volume(o: bpy.types.Object) -> Union[float, None]:
|
||||
assert isinstance(o.data, bpy.types.Mesh)
|
||||
o_mesh = bmesh.new()
|
||||
o_mesh.from_mesh(o.data)
|
||||
if not is_manifold(o_mesh):
|
||||
o_mesh.free()
|
||||
return None
|
||||
volume = o_mesh.calc_volume()
|
||||
o_mesh.free()
|
||||
return volume
|
||||
|
||||
|
||||
def get_gross_volume(o: bpy.types.Object) -> float:
|
||||
def get_gross_volume(o: bpy.types.Object) -> Union[float, None]:
|
||||
if not has_openings(o):
|
||||
return get_net_volume(o)
|
||||
|
||||
@@ -596,6 +617,11 @@ def get_gross_volume(o: bpy.types.Object) -> float:
|
||||
mesh = get_gross_element_mesh(element)
|
||||
bm = get_bmesh_from_mesh(mesh)
|
||||
|
||||
if not is_manifold(bm):
|
||||
bm.free()
|
||||
delete_mesh(mesh)
|
||||
return None
|
||||
|
||||
gross_volume = bm.calc_volume()
|
||||
|
||||
bm.free()
|
||||
@@ -634,6 +660,8 @@ def get_gross_weight(obj: bpy.types.Object) -> Union[float, None]:
|
||||
return
|
||||
|
||||
gross_volume = get_gross_volume(obj)
|
||||
if gross_volume is None:
|
||||
return None
|
||||
gross_weight = obj_mass_density * gross_volume
|
||||
return gross_weight
|
||||
|
||||
@@ -656,6 +684,8 @@ def get_net_weight(obj: bpy.types.Object) -> Union[float, None]:
|
||||
return
|
||||
|
||||
net_volume = get_net_volume(obj)
|
||||
if net_volume is None:
|
||||
return None
|
||||
net_weight = obj_mass_density * net_volume
|
||||
return net_weight
|
||||
|
||||
|
||||
@@ -181,6 +181,79 @@ class TestGetCalculatedObjectQuantities(test.bim.bootstrap.NewFile):
|
||||
assert quantities["NetVolume"] == 282.517
|
||||
|
||||
|
||||
class TestGetCalculatedObjectQuantitiesNonManifold(test.bim.bootstrap.NewFile):
|
||||
"""Regression test for #6125: a non-manifold mesh must not produce a bogus volume."""
|
||||
|
||||
def test_run(self):
|
||||
import bmesh
|
||||
import ifc5d.qto
|
||||
|
||||
import bonsai.core.root
|
||||
|
||||
self.ifc = ifcopenshell.file()
|
||||
tool.Ifc.set(self.ifc)
|
||||
ifcopenshell.api.root.create_entity(self.ifc, ifc_class="IfcProject", name="My Project")
|
||||
import logging
|
||||
|
||||
import bonsai.bim.import_ifc as import_ifc
|
||||
|
||||
ifc_import_settings = import_ifc.IfcImportSettings.factory(
|
||||
bpy.context, tool.Ifc.get_path(), logging.getLogger("ImportIFC")
|
||||
)
|
||||
ifc_importer = import_ifc.IfcImporter(ifc_import_settings)
|
||||
ifc_importer.file = self.ifc
|
||||
ifc_importer.create_project()
|
||||
|
||||
context = ifcopenshell.api.context.add_context(self.ifc, context_type="Model")
|
||||
|
||||
bpy.ops.mesh.primitive_cube_add(location=(0.0, 0.0, 0.0), size=2)
|
||||
obj = bpy.context.active_object
|
||||
element = bonsai.core.root.assign_class(
|
||||
tool.Ifc,
|
||||
tool.Collector,
|
||||
tool.Root,
|
||||
obj=obj,
|
||||
ifc_class="IfcWall",
|
||||
predefined_type="ELEMENTEDWALL",
|
||||
context=context,
|
||||
)
|
||||
|
||||
# Corrupt the mesh into an open (non-watertight) shape after the IFC
|
||||
# representation is assigned, simulating a badly authored import
|
||||
# rather than something Bonsai's own authoring tools would produce.
|
||||
bm = bmesh.new()
|
||||
bm.from_mesh(obj.data)
|
||||
bm.faces.ensure_lookup_table()
|
||||
bmesh.ops.delete(bm, geom=[bm.faces[0]], context="FACES")
|
||||
bm.to_mesh(obj.data)
|
||||
bm.free()
|
||||
|
||||
rules = {
|
||||
"calculators": {
|
||||
"Blender": {
|
||||
"IfcWall": {
|
||||
"Qto_WallBaseQuantities": {
|
||||
"GrossFootprintArea": "get_gross_footprint_area",
|
||||
"GrossVolume": "get_gross_volume",
|
||||
"NetVolume": "get_net_volume",
|
||||
}
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
ifc_file = tool.Ifc.get()
|
||||
results = ifc5d.qto.quantify(ifc_file, {element}, rules)
|
||||
quantities = results[element]["Qto_WallBaseQuantities"]
|
||||
|
||||
# Topology-independent quantities are unaffected.
|
||||
assert quantities["GrossFootprintArea"] == 4
|
||||
# Volume is undefined for a non-manifold mesh, so it is skipped rather
|
||||
# than reporting a wrong number.
|
||||
assert "GrossVolume" not in quantities
|
||||
assert "NetVolume" not in quantities
|
||||
|
||||
|
||||
class TestGetBaseQto(test.bim.bootstrap.NewFile):
|
||||
def test_run(self):
|
||||
ifc = ifcopenshell.file()
|
||||
|
||||
@@ -68,14 +68,43 @@ def is_x(value: float, x: float, tolerance: Optional[float] = None) -> bool:
|
||||
return abs(x - value) < tolerance
|
||||
|
||||
|
||||
def is_manifold(geometry: W.Triangulation) -> bool:
|
||||
"""Checks whether a triangulated geometry is a closed, consistently oriented manifold
|
||||
|
||||
Two conditions are checked for every edge of every triangle:
|
||||
|
||||
- Unoriented use: as an unordered pair of vertices, an edge must be shared
|
||||
by exactly two triangles. A count of 1 means an open hole or boundary,
|
||||
a count above 2 means more than two triangles meet at that edge.
|
||||
- Oriented use: as an ordered pair of vertices, an edge must be used by at
|
||||
most one triangle. If two triangles use the same ordered edge, their
|
||||
windings are inconsistent (e.g. a flipped or duplicated face), which
|
||||
also invalidates volume calculations that rely on consistent winding.
|
||||
|
||||
:param geometry: Geometry output calculated by IfcOpenShell
|
||||
:return: ``True`` if the geometry is a closed, consistently oriented manifold
|
||||
"""
|
||||
faces = geometry.faces
|
||||
directed_use: dict[tuple[int, int], int] = {}
|
||||
undirected_use: dict[tuple[int, int], int] = {}
|
||||
for i in range(0, len(faces), 3):
|
||||
tri = (faces[i], faces[i + 1], faces[i + 2])
|
||||
for a, b in ((tri[0], tri[1]), (tri[1], tri[2]), (tri[2], tri[0])):
|
||||
directed_use[(a, b)] = directed_use.get((a, b), 0) + 1
|
||||
edge = (a, b) if a < b else (b, a)
|
||||
undirected_use[edge] = undirected_use.get(edge, 0) + 1
|
||||
return all(count == 2 for count in undirected_use.values()) and all(count == 1 for count in directed_use.values())
|
||||
|
||||
|
||||
def get_volume(geometry: W.Triangulation) -> float:
|
||||
"""Calculates the total internal volume of a geometry
|
||||
|
||||
The volume is derived from the divergence theorem (summing signed
|
||||
tetrahedra), which is only meaningful for a closed manifold (watertight)
|
||||
mesh. For non-manifold or open geometry that value is undefined and can be
|
||||
wildly over- or under-estimated, so ``float("nan")`` is returned instead of
|
||||
a bogus number. See https://github.com/IfcOpenShell/IfcOpenShell/issues/6125.
|
||||
tetrahedra), which is only meaningful for a closed, consistently oriented
|
||||
manifold (watertight) mesh. For non-manifold or open geometry that value
|
||||
is undefined and can be wildly over- or under-estimated, so
|
||||
``float("nan")`` is returned instead of a bogus number. See
|
||||
https://github.com/IfcOpenShell/IfcOpenShell/issues/6125.
|
||||
|
||||
:param geometry: Geometry output calculated by IfcOpenShell
|
||||
:return: The volume in m3, or ``nan`` if the mesh is not a closed manifold
|
||||
@@ -91,22 +120,12 @@ def get_volume(geometry: W.Triangulation) -> float:
|
||||
v123 = p1[0] * p2[1] * p3[2]
|
||||
return (1.0 / 6.0) * (-v321 + v231 + v312 - v132 - v213 + v123)
|
||||
|
||||
if not is_manifold(geometry):
|
||||
return float("nan")
|
||||
|
||||
# Can't optimize it using buffers - performance seems to get only worse.
|
||||
verts = geometry.verts
|
||||
faces = geometry.faces
|
||||
|
||||
# A watertight (closed manifold) mesh shares every edge between exactly two
|
||||
# triangles. If that does not hold the signed-tetrahedra sum below is
|
||||
# meaningless, so bail out with nan rather than returning a wild value.
|
||||
edge_face_count: dict[tuple[int, int], int] = {}
|
||||
for i in range(0, len(faces), 3):
|
||||
tri = (faces[i], faces[i + 1], faces[i + 2])
|
||||
for a, b in ((tri[0], tri[1]), (tri[1], tri[2]), (tri[2], tri[0])):
|
||||
edge = (a, b) if a < b else (b, a)
|
||||
edge_face_count[edge] = edge_face_count.get(edge, 0) + 1
|
||||
if any(count != 2 for count in edge_face_count.values()):
|
||||
return float("nan")
|
||||
|
||||
grouped_verts = [[verts[i], verts[i + 1], verts[i + 2]] for i in range(0, len(verts), 3)]
|
||||
volumes = [
|
||||
signed_triangle_volume(grouped_verts[faces[i]], grouped_verts[faces[i + 1]], grouped_verts[faces[i + 2]])
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
# IfcOpenShell - IFC toolkit and geometry engine
|
||||
# Copyright (C) 2026 Dion Moult <dion@thinkmoult.com>
|
||||
#
|
||||
# This file is part of IfcOpenShell.
|
||||
#
|
||||
# IfcOpenShell is free software: you can redistribute it and/or modify
|
||||
# it under the terms of the GNU Lesser General Public License as published by
|
||||
# the Free Software Foundation, either version 3 of the License, or
|
||||
# (at your option) any later version.
|
||||
#
|
||||
# IfcOpenShell is distributed in the hope that it will be useful,
|
||||
# but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
# GNU Lesser General Public License for more details.
|
||||
#
|
||||
# You should have received a copy of the GNU Lesser General Public License
|
||||
# along with IfcOpenShell. If not, see <http://www.gnu.org/licenses/>.
|
||||
# This file was generated with the assistance of an AI coding tool.
|
||||
|
||||
import math
|
||||
|
||||
import ifcopenshell.util.shape as subject
|
||||
|
||||
|
||||
class FakeTriangulation:
|
||||
"""A minimal stand-in for W.Triangulation, exposing only what get_volume/is_manifold use."""
|
||||
|
||||
def __init__(self, verts: list[tuple[float, float, float]], faces: list[tuple[int, int, int]]):
|
||||
self.verts = [c for v in verts for c in v]
|
||||
self.faces = [i for tri in faces for i in tri]
|
||||
|
||||
|
||||
def cube(size: float = 1.0) -> tuple[list[tuple[float, float, float]], list[tuple[int, int, int]]]:
|
||||
s = size
|
||||
verts = [
|
||||
(0, 0, 0),
|
||||
(s, 0, 0),
|
||||
(s, s, 0),
|
||||
(0, s, 0),
|
||||
(0, 0, s),
|
||||
(s, 0, s),
|
||||
(s, s, s),
|
||||
(0, s, s),
|
||||
]
|
||||
# Consistently wound (outward normals) triangulated cube.
|
||||
faces = [
|
||||
(0, 2, 1),
|
||||
(0, 3, 2),
|
||||
(4, 5, 6),
|
||||
(4, 6, 7),
|
||||
(0, 1, 5),
|
||||
(0, 5, 4),
|
||||
(3, 7, 6),
|
||||
(3, 6, 2),
|
||||
(0, 4, 7),
|
||||
(0, 7, 3),
|
||||
(1, 2, 6),
|
||||
(1, 6, 5),
|
||||
]
|
||||
return verts, faces
|
||||
|
||||
|
||||
class TestIsManifold:
|
||||
def test_closed_consistently_wound_mesh_is_manifold(self):
|
||||
verts, faces = cube()
|
||||
assert subject.is_manifold(FakeTriangulation(verts, faces)) is True
|
||||
|
||||
def test_open_mesh_is_not_manifold(self):
|
||||
verts, faces = cube()
|
||||
# Remove one face, leaving an open boundary.
|
||||
geometry = FakeTriangulation(verts, faces[:-1])
|
||||
assert subject.is_manifold(geometry) is False
|
||||
|
||||
def test_inconsistent_winding_is_not_manifold(self):
|
||||
# A single flipped triangle keeps every edge shared by exactly two
|
||||
# triangles (an unordered edge-count check alone would miss this),
|
||||
# but two faces now use the same directed edge.
|
||||
verts, faces = cube()
|
||||
faces = list(faces)
|
||||
i = faces.index((1, 2, 6))
|
||||
faces[i] = (1, 6, 2)
|
||||
geometry = FakeTriangulation(verts, faces)
|
||||
assert subject.is_manifold(geometry) is False
|
||||
|
||||
|
||||
class TestGetVolume:
|
||||
def test_manifold_cube_volume(self):
|
||||
verts, faces = cube(size=2)
|
||||
geometry = FakeTriangulation(verts, faces)
|
||||
assert math.isclose(subject.get_volume(geometry), 8.0, rel_tol=1e-9)
|
||||
|
||||
def test_open_mesh_returns_nan(self):
|
||||
verts, faces = cube()
|
||||
geometry = FakeTriangulation(verts, faces[:-1])
|
||||
assert math.isnan(subject.get_volume(geometry))
|
||||
|
||||
def test_inconsistent_winding_returns_nan_instead_of_wrong_value(self):
|
||||
verts, faces = cube()
|
||||
faces = list(faces)
|
||||
i = faces.index((1, 2, 6))
|
||||
faces[i] = (1, 6, 2)
|
||||
geometry = FakeTriangulation(verts, faces)
|
||||
# Without the manifold guard this silently returns 0.667 instead of 1.0.
|
||||
assert math.isnan(subject.get_volume(geometry))
|
||||
Reference in New Issue
Block a user