From 0b0d1ccc125ff8b294dead53ce5e0a6777046f60 Mon Sep 17 00:00:00 2001 From: Furgo <148809153+furgo16@users.noreply.github.com> Date: Tue, 18 Nov 2025 14:58:59 +0100 Subject: [PATCH] BIM: Smart removal of wall bases (#24550) * BIM: Implement smart base removal for Walls Previously, removing the Base object from an Arch Wall would cause the wall to reset its position to the document origin and could lead to unintended geometric changes for complex walls. This commit introduces a "smart debasing" mechanism integrated into the Component Task Panel's "Remove" button: - For walls based on a single straight line, the operation now preserves the wall's global position and parametric `Length`, making it an independent object. - For walls with complex bases (multi-segment, curved), a warning dialog is now presented to the user, explaining the consequences (shape alteration and position reset) before allowing the operation to proceed. This is supported by new API functions `Arch.is_debasable()` and `Arch.debaseWall()`, which contain the core logic for the feature. Fixes: https://github.com/FreeCAD/FreeCAD/issues/24453 * BIM: Move wall debasing logic into ArchWall proxy The logic for handling the removal of a wall's base object was previously implemented directly within the generic `ComponentTaskPanel` in `ArchComponent.py`. This created a tight coupling, forcing the generic component UI to have specific knowledge about the `ArchWall` type. This commit refactors the implementation to follow a more object-oriented and polymorphic design: 1. A new overridable method, `handleComponentRemoval(subobject)`, has been added to the base `ArchComponent` proxy class. Its default implementation maintains the standard removal behavior. 2. The `_Wall` proxy class in `ArchWall.py` now overrides this method. All wall-specific debasing logic, including the eligibility check and the user-facing warning dialog, now resides entirely within this override. 3. The `ComponentTaskPanel.removeElement` method has been simplified. It is now a generic dispatcher that calls `handleComponentRemoval` on the proxy of the object being edited, with no specific knowledge of object types. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * BIM: Correct user warning The operation can indeed be undone. Co-authored-by: Roy-043 <70520633+Roy-043@users.noreply.github.com> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Roy-043 <70520633+Roy-043@users.noreply.github.com> --- src/Mod/BIM/Arch.py | 156 +++++++++++++++++++++++++++ src/Mod/BIM/ArchCommands.py | 9 ++ src/Mod/BIM/ArchComponent.py | 33 ++++-- src/Mod/BIM/ArchWall.py | 49 +++++++++ src/Mod/BIM/bimtests/TestArchWall.py | 139 ++++++++++++++++++++++-- 5 files changed, 368 insertions(+), 18 deletions(-) diff --git a/src/Mod/BIM/Arch.py b/src/Mod/BIM/Arch.py index 008daf13f5..b275c17915 100644 --- a/src/Mod/BIM/Arch.py +++ b/src/Mod/BIM/Arch.py @@ -2151,6 +2151,162 @@ def makeWindow( return window +def is_debasable(wall): + """Determines if an Arch Wall can be cleanly converted to a baseless state. + + This function checks if a given wall is a valid candidate for a parametric + "debasing" operation, where its dependency on a Base object is removed and + it becomes driven by its own Length and Placement properties. + + Parameters + ---------- + wall : FreeCAD.DocumentObject + The Arch Wall object to check. + + Returns + ------- + bool + ``True`` if the wall is a valid candidate for debasing, otherwise ``False``. + + Notes + ----- + A wall is considered debasable if its ``Base`` object's final shape consists + of exactly one single, straight edge. This check is generic and works for + any base object that provides a valid ``.Shape`` property, including + ``Draft.Line`` and ``Sketcher::SketchObject`` objects. + """ + import Part + import Draft + + # Ensure the object is actually a wall + if Draft.getType(wall) != "Wall": + return False + + # Check for a valid Base object with a geometric Shape + if not hasattr(wall, "Base") or not wall.Base: + return False + if not hasattr(wall.Base, "Shape") or wall.Base.Shape.isNull(): + return False + + base_shape = wall.Base.Shape + + # The core condition: the final shape must contain exactly one edge. + # This correctly handles Sketches with multiple lines or construction geometry. + if len(base_shape.Edges) != 1: + return False + + # The single edge must be a straight line. + edge = base_shape.Edges[0] + if not isinstance(edge.Curve, (Part.Line, Part.LineSegment)): + return False + + # If all checks pass, the wall is debasable. + return True + + +def debaseWall(wall): + """ + Converts a line-based Arch Wall to be parametrically driven by its own + properties (Length, Width, Height) and Placement, removing its dependency + on a Base object. + + This operation preserves the wall's exact size and global position. + It is only supported for walls based on a single, straight line. + + Returns True on success, False otherwise. + """ + import FreeCAD + + if not is_debasable(wall): + FreeCAD.Console.PrintWarning(f"Wall '{wall.Label}' is not eligible for debasing.\n") + return False + + doc = wall.Document + doc.openTransaction(f"Debase Wall: {wall.Label}") + try: + # Calculate the final state before making any changes + # A wall's final position and orientation are derived from its Base object and its own + # properties like Width and Align. To make the wall independent, this final state must be + # captured. A simple transfer of the Base object's Placement property is unreliable for two + # main reasons: + # - Ambiguity: A Draft.Line's direction is defined by its vertex coordinates, so a line at + # 45° can have a 0° rotation in its Placement. + # - Coordinate Systems: A universal method is needed to handle both Draft objects (defined + # in global coordinates) and Sketch objects (defined in a local coordinate system). + # + # The solution is to use the wall.Proxy.basewires internal attribute. It is non-persistent + # and populated on the fly by calling getExtrusionData(). It contains the baseline edge + # already transformed into the document's global coordinate system. From the vertex + # positions of this globally-aware edge, the new wall's final Placement is calculated. + extrusion_data = wall.Proxy.getExtrusionData(wall) + if not extrusion_data or not hasattr(wall.Proxy, "basewires") or not wall.Proxy.basewires: + raise Exception("Could not retrieve extrusion data to calculate global placement.") + + # In addition to the baseline edge, getExtrusionData() also provides the extrusion vector, + # which is used to determine the wall's vertical orientation. + extrusion_vector = extrusion_data[1] + baseline_edge = wall.Proxy.basewires[0][0] + + # Now determine the wall's rotation inferred from its local axes: + # - The local X axis is along the baseline edge (length). + # - The local Z axis is along the extrusion vector (height). + # - The local Y axis is the cross product of X and Z (width, perpendicular to both the + # - above). + # Once the local axes are known, a FreeCAD.Rotation matrix can be constructed. + z_axis = extrusion_vector.normalize() + x_axis = (baseline_edge.lastVertex().Point - baseline_edge.firstVertex().Point).normalize() + y_axis = z_axis.cross(x_axis).normalize() + final_rotation = FreeCAD.Rotation(x_axis, y_axis, z_axis) + + # This will be the debased wall's local coordinate system origin (0, 0, 0). + # The wall's Align property (Left, Center, Right) determines how the wall's + # Width offsets the final position from the centerline. + centerline_position = baseline_edge.CenterOfMass + align_offset_distance = 0 + if wall.Align == "Left": + align_offset_distance = wall.Width.Value / 2.0 + elif wall.Align == "Right": + align_offset_distance = -wall.Width.Value / 2.0 + + # Convert the offset distance into a vector in the width direction (local Y axis). + align_offset_vector = y_axis * align_offset_distance + final_position = centerline_position - align_offset_vector + + final_placement = FreeCAD.Placement(final_position, final_rotation) + + # Store properties before unlinking + height = wall.Height.Value + length = wall.Length.Value + width = wall.Width.Value + + # 1. Apply the final placement first. + wall.Placement = final_placement + + # 2. Now, remove the base. The recompute triggered by this change + # will already have the correct placement to work with. + wall.Base = None + + # 3. Clear internal caches and set final properties. + if hasattr(wall.Proxy, "connectEdges"): + wall.Proxy.connectEdges = [] + + wall.Height = height + wall.Length = length + wall.Width = width + + # 4. Add an explicit recompute to ensure the final state is settled. + doc.recompute() + + except Exception as e: + doc.abortTransaction() + FreeCAD.Console.PrintError(f"Error debasing wall '{wall.Label}': {e}\n") + return False + finally: + doc.commitTransaction() + + return True + + def _initializeArchObject( objectType, baseClassName=None, diff --git a/src/Mod/BIM/ArchCommands.py b/src/Mod/BIM/ArchCommands.py index cf0469cde6..cfb393e89c 100644 --- a/src/Mod/BIM/ArchCommands.py +++ b/src/Mod/BIM/ArchCommands.py @@ -245,6 +245,15 @@ def removeComponents(objectsList, host=None): for o in objectsList: if o.InList: h = o.InList[0] + + is_base_removal = hasattr(h, "Base") and h.Base == o + has_handler = hasattr(h, "Proxy") and hasattr(h.Proxy, "handleComponentRemoval") + + if is_base_removal and has_handler: + # Dispatch to the object's own smart removal logic and skip the old code path. + h.Proxy.handleComponentRemoval(h, o) + continue + tp = Draft.getType(h) if tp in ["Floor", "Building", "Site", "BuildingPart"]: c = h.Group diff --git a/src/Mod/BIM/ArchComponent.py b/src/Mod/BIM/ArchComponent.py index f6820f5a5a..b61c186dd8 100644 --- a/src/Mod/BIM/ArchComponent.py +++ b/src/Mod/BIM/ArchComponent.py @@ -835,6 +835,13 @@ class Component(ArchIFC.IfcProduct): if o: o.ViewObject.hide() + def handleComponentRemoval(self, obj, subobject): + """ + Default handler for when a component is removed via the Task Panel. + Subclasses can override this to provide special behavior. + """ + removeFromComponent(obj, subobject) + def processSubShapes(self, obj, base, placement=None): """Add Additions and Subtractions to a base shape. @@ -2284,18 +2291,24 @@ class ComponentTaskPanel: self.update() def removeElement(self): - """This method is run as a callback when the user selects the remove button. - - Get the object selected in the tree widget. If there is an object in - the document with the same Name as the selected item in the tree, - remove it from the object being edited, with the removeFromComponent() - function. """ + This method is run as a callback when the user selects the remove button. + It calls a handler on the object's proxy to perform the removal. + """ + element_selected = self.tree.currentItem() + if not element_selected: + return + + element_to_remove = FreeCAD.ActiveDocument.getObject(str(element_selected.toolTip(0))) + + # Call the polymorphic handler on the object's proxy. + # This is generic and works for any Arch object. + if hasattr(self.obj.Proxy, "handleComponentRemoval"): + self.obj.Proxy.handleComponentRemoval(self.obj, element_to_remove) + else: + # Fallback for older proxies that might not have the method + removeFromComponent(self.obj, element_to_remove) - it = self.tree.currentItem() - if it: - comp = FreeCAD.ActiveDocument.getObject(str(it.toolTip(0))) - removeFromComponent(self.obj, comp) self.update() def accept(self): diff --git a/src/Mod/BIM/ArchWall.py b/src/Mod/BIM/ArchWall.py index f139df20d8..f574e26346 100644 --- a/src/Mod/BIM/ArchWall.py +++ b/src/Mod/BIM/ArchWall.py @@ -1520,6 +1520,55 @@ class _Wall(ArchComponent.Component): return (base, extrusion, placement) return None + def handleComponentRemoval(self, obj, subobject): + """ + Overrides the default component removal to implement smart debasing + when the Base object is being removed. + """ + import Arch + from PySide import QtGui + + # Check if the component being removed is this wall's Base + if hasattr(obj, "Base") and obj.Base == subobject: + if Arch.is_debasable(obj): + # This is a valid, single-line wall. Perform a clean debase. + Arch.debaseWall(obj) + else: + # This is a complex wall. Behavior depends on GUI availability. + if FreeCAD.GuiUp: + # --- GUI Path: Warn the user and ask for confirmation. --- + from PySide import QtGui + + msg_box = QtGui.QMessageBox() + msg_box.setWindowTitle(translate("ArchComponent", "Unsupported Base")) + msg_box.setText( + translate( + "ArchComponent", "The base of this wall is not a single straight line." + ) + ) + msg_box.setInformativeText( + translate( + "ArchComponent", + "Removing the base of this complex wall will alter its shape and reset its position.\n\n" + "Do you want to proceed?", + ) + ) + msg_box.setStandardButtons(QtGui.QMessageBox.Yes | QtGui.QMessageBox.Cancel) + msg_box.setDefaultButton(QtGui.QMessageBox.Cancel) + if msg_box.exec_() == QtGui.QMessageBox.Yes: + # User confirmed, perform the standard removal + super(_Wall, self).handleComponentRemoval(obj, subobject) + else: + # --- Headless Path: Do not perform the destructive action. Print a warning. --- + FreeCAD.Console.PrintWarning( + f"Skipping removal of complex base for wall '{obj.Label}'. " + "This interactive action is not supported in headless mode.\n" + ) + else: + # If it's not the base (e.g., an Addition), use the default behavior + # from the parent Component class. + super(_Wall, self).handleComponentRemoval(obj, subobject) + class _ViewProviderWall(ArchComponent.ViewProviderComponent): """The view provider for the wall object. diff --git a/src/Mod/BIM/bimtests/TestArchWall.py b/src/Mod/BIM/bimtests/TestArchWall.py index 30a323f58f..c3e745e3e7 100644 --- a/src/Mod/BIM/bimtests/TestArchWall.py +++ b/src/Mod/BIM/bimtests/TestArchWall.py @@ -118,23 +118,146 @@ class TestArchWall(TestArchBase.TestArchBase): def test_remove_base_from_wall_without_host(self): """ - Tests that removing a wall's base using removeComponents(host=None) - does not crash and successfully unlinks the base. - This is the non-regression test for the 'list' has no attribute 'Base' bug. - https://github.com/FreeCAD/FreeCAD/issues/24532 + Tests that removing a debasable wall's base using removeComponents + successfully unlinks the base. """ self.printTestMessage("Testing removal of a wall's base component...") # 1. Arrange: Create a wall with a base line = Draft.makeLine(App.Vector(0, 0, 0), App.Vector(2000, 0, 0)) + # Ensure the base object's shape is computed, making the wall debasable. + line.recompute() wall = Arch.makeWall(line) + self.document.recompute() # Ensure wall is fully formed self.assertIsNotNone(wall.Base, "Pre-condition failed: Wall should have a base.") + self.assertTrue( + Arch.is_debasable(wall), "Pre-condition failed: The test wall is not debasable." + ) - # 2. Act: Call removeComponents on the base, simulating the failing workflow - # Before the fix, this will raise an AttributeError. - # After the fix, it should complete without error. + # 2. Act: Call removeComponents on the base. + # This will trigger the is_debasable -> True -> debaseWall() path. Arch.removeComponents([wall.Base]) self.document.recompute() - # 3. Assert: The base should now be None + # 3. Assert: The base should now be None because debaseWall was successful. self.assertIsNone(wall.Base, "The wall's Base property was not cleared after removal.") + + def test_is_debasable_with_valid_line_base(self): + """Tests that a wall based on a single Draft.Line is debasable.""" + self.printTestMessage("Checking is_debasable with Draft.Line...") + line = Draft.makeLine(App.Vector(0, 0, 0), App.Vector(1000, 0, 0)) + line.recompute() + wall = Arch.makeWall(line) + self.document.recompute() + self.assertTrue(Arch.is_debasable(wall), "Wall on Draft.Line should be debasable.") + + def test_is_debasable_with_valid_sketch_base(self): + """Tests that a wall based on a Sketch with a single line is debasable.""" + self.printTestMessage("Checking is_debasable with single-line Sketch...") + sketch = self.document.addObject("Sketcher::SketchObject", "SingleLineSketch") + sketch.addGeometry(Part.LineSegment(App.Vector(0, 0, 0), App.Vector(1000, 0, 0))) + self.document.recompute() + wall = Arch.makeWall(sketch) + self.assertTrue(Arch.is_debasable(wall), "Wall on single-line Sketch should be debasable.") + + def test_is_debasable_with_multi_edge_base(self): + """Tests that a wall based on a multi-segment wire is not debasable.""" + self.printTestMessage("Checking is_debasable with multi-segment Wire...") + wire = Draft.makeWire( + [App.Vector(0, 0, 0), App.Vector(1000, 0, 0), App.Vector(1000, 1000, 0)] + ) + wall = Arch.makeWall(wire) + self.assertFalse( + Arch.is_debasable(wall), "Wall on multi-segment wire should not be debasable." + ) + + def test_is_debasable_with_curved_base(self): + """Tests that a wall based on an arc is not debasable.""" + self.printTestMessage("Checking is_debasable with curved base...") + arc = Draft.make_circle(radius=500, startangle=0, endangle=90) + self.document.recompute() + wall = Arch.makeWall(arc) + self.document.recompute() + self.assertFalse(Arch.is_debasable(wall), "Wall on curved base should not be debasable.") + + def test_is_debasable_with_no_base(self): + """Tests that a baseless wall is not debasable.""" + self.printTestMessage("Checking is_debasable with no base...") + wall = Arch.makeWall(length=1000) + self.assertFalse(Arch.is_debasable(wall), "Baseless wall should not be debasable.") + + def test_debase_wall_preserves_global_position(self): + """ + Tests that debaseWall correctly transfers the base's placement to the + wall, preserving its global position and dimensions. + """ + self.printTestMessage("Checking debaseWall preserves global position...") + + # 1. Arrange: Create a rotated and translated line, and a wall from it. + pl = App.Placement(App.Vector(1000, 500, 200), App.Rotation(App.Vector(0, 0, 1), 45)) + line = Draft.makeLine(App.Vector(0, 0, 0), App.Vector(2000, 0, 0)) + line.Placement = pl + line.recompute() # Use object-level recompute + + wall = Arch.makeWall(line, width=200, height=1500, align="Left") + self.document.recompute() + + # Store the wall's original state + original_bb = wall.Shape.BoundBox + original_volume = wall.Shape.Volume + original_length = wall.Length.Value + + # 2. Act: Debase the wall + success = Arch.debaseWall(wall) + self.document.recompute() + + # 3. Assert + self.assertTrue(success, "debaseWall should return True for a valid wall.") + self.assertIsNone(wall.Base, "Wall's Base should be None after debasing.") + + # Core assertions for preserving geometry and placement + self.assertAlmostEqual( + original_volume, + wall.Shape.Volume, + delta=1e-6, + msg="Wall volume should not change after debasing.", + ) + + # Compare individual properties of the BoundBox with a tolerance + final_bb = wall.Shape.BoundBox + self.assertAlmostEqual( + original_bb.XMin, final_bb.XMin, delta=1e-6, msg="Bounding box XMin does not match." + ) + self.assertAlmostEqual( + original_bb.XMax, final_bb.XMax, delta=1e-6, msg="Bounding box XMax does not match." + ) + self.assertAlmostEqual( + original_bb.YMin, final_bb.YMin, delta=1e-6, msg="Bounding box YMin does not match." + ) + self.assertAlmostEqual( + original_bb.YMax, final_bb.YMax, delta=1e-6, msg="Bounding box YMax does not match." + ) + self.assertAlmostEqual( + original_bb.ZMin, final_bb.ZMin, delta=1e-6, msg="Bounding box ZMin does not match." + ) + self.assertAlmostEqual( + original_bb.ZMax, final_bb.ZMax, delta=1e-6, msg="Bounding box ZMax does not match." + ) + + # Check parametric integrity + self.assertAlmostEqual( + wall.Length.Value, + original_length, + delta=1e-6, + msg="Wall's Length property should be preserved.", + ) + + # Verify it remains parametric by changing a property + wall.Height = 2000 + self.document.recompute() + self.assertNotAlmostEqual( + original_volume, + wall.Shape.Volume, + delta=1e-6, + msg="Wall should remain parametric and its volume should change with height.", + )