mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-09-12 14:33:28 +00:00
Promote idle-row icons into the slot system
The toggle_openings icon lived outside the IconSlot layout — each host (wall, roof) declared an ad-hoc setup_pen_row_toggle_openings_icon + update_pen_row_toggle_openings_icon pair, and GizmoArrayEdition queried a hardcoded _FEATURE_IDLE_MAX_X dict to position past it. On an arrayed wall the dict was shadowed: find_for_element returns "array" before "wall" in EDIT_TYPES order, the wall reservation was never consulted, and the first per-layer ARRAY icon (local X=0.37) landed 13cm from the wall's toggle_openings (X=0.50) — visually on top of each other. Promote idle-row icons into the slot system instead of patching the dict: * IconSlot gains an Optional visible_when predicate for state-driven visibility (toggle_openings only when the host carries openings). * BaseParametricGizmoGroup gains idle_slots: ClassVar[tuple[IconSlot]] + _idle_slot_x_positions() + _idle_row_right_edge() helpers; the setup + idle-branch positioning loops mirror the existing feature_slots path. * Wall and roof declare toggle_openings as an idle_slot and drop their ad-hoc setup/update calls. * GizmoArrayEdition's _resolve_feature_idle_max_x walks BaseParametricGizmoGroup.REGISTRY and takes the max _idle_row_right_edge() across peers whose poll passes — no more hardcoded dict, no more find_for_element-order shadowing. * setup_pen_row_toggle_openings_icon + update_pen_row_toggle_openings_icon helpers deleted from drawing/gizmos.py. * 3 forward-compat AST guards pin the new contract. Also bundles an unrelated array-test fix: TestUsingArrays in test/tool/test_model.py was asserting against bpy.context.selected_objects which is a fragile signal after remove_array / apply_array. A new _array_objects() helper filters bpy.data.objects via the BIM_Array pset's IfcActuator type instead. Layout on an arrayed wall after the fix: pen X = 0.00 toggle X = 0.50 (idle_slot 0) array[0] X = 0.87 (one ICON_ARRAY_GAP past idle row) array[1] X = 1.27 All separated by the standard inter-icon spacing. Generated with the assistance of an AI coding tool.
This commit is contained in:
committed by
Thomas Krijnen
parent
b873db11db
commit
99d758a330
@@ -211,3 +211,176 @@ def test_join_intersection_stacks_along_screen_up_in_both_states():
|
||||
"billboarded_at writes for the join/unjoin/extend/fillet icons bypass "
|
||||
"the stacking contract and re-introduce the top-view collapse bug."
|
||||
)
|
||||
|
||||
|
||||
def _get_wall_axis_callers_in(method) -> set[str]:
|
||||
"""Return the set of attribute chains in ``method``'s source that resolve
|
||||
to ``tool.Model.get_wall_axis``. Empty set means the method does not read
|
||||
from the mesh-bound-box axis source."""
|
||||
source = textwrap.dedent(inspect.getsource(method))
|
||||
tree = ast.parse(source)
|
||||
offenders: set[str] = set()
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
func = node.func
|
||||
if not isinstance(func, ast.Attribute) or func.attr != "get_wall_axis":
|
||||
continue
|
||||
# Reconstruct the receiver chain to surface it in the assertion message.
|
||||
chain: list[str] = [func.attr]
|
||||
receiver = func.value
|
||||
while isinstance(receiver, ast.Attribute):
|
||||
chain.append(receiver.attr)
|
||||
receiver = receiver.value
|
||||
if isinstance(receiver, ast.Name):
|
||||
chain.append(receiver.id)
|
||||
offenders.add(".".join(reversed(chain)))
|
||||
return offenders
|
||||
|
||||
|
||||
def _method_writes_ifc_axis(method) -> bool:
|
||||
"""True iff ``method``'s body calls ``self.set_axis(...)`` — the only
|
||||
path that writes a wall's IFC reference line via
|
||||
``ifcopenshell.api.geometry.assign_representation``. Methods that only
|
||||
read ``axis["base"]`` / ``axis["side"]`` for layer-polygon work (slab
|
||||
clipping, opening snap) never call ``set_axis`` and are not under this
|
||||
rule."""
|
||||
source = textwrap.dedent(inspect.getsource(method))
|
||||
tree = ast.parse(source)
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
func = node.func
|
||||
if isinstance(func, ast.Attribute) and func.attr == "set_axis":
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def test_dumb_wall_joiner_axis_writers_read_ifc_reference_line():
|
||||
"""Any ``DumbWallJoiner`` method that writes the IFC reference line
|
||||
(via ``self.set_axis`` → ``ifcopenshell.api.geometry.assign_representation``)
|
||||
must read its input axis from the IFC reference line too — not from
|
||||
``tool.Model.get_wall_axis``, whose X-extent comes from ``obj.bound_box``
|
||||
(the Body mesh AABB). The bound-box axis drifts past or short of the
|
||||
IFC reference line at mitred / butt-jointed walls and at walls with end
|
||||
openings; mixing it on input with the IFC axis on output produces
|
||||
non-colinear sub-axes that compound through chained extend/split/join
|
||||
edits.
|
||||
|
||||
The IFC-anchored helper is ``tool.Wall.get_world_reference_line`` for
|
||||
world-space endpoints, or ``ifcopenshell.util.representation.get_reference_line``
|
||||
for local-SI endpoints.
|
||||
|
||||
Joiner methods that only read layer-polygon base/side (e.g. ``clip``
|
||||
for slab intersection) are exempt — they need the body footprint, not
|
||||
the axis, and never call ``set_axis``."""
|
||||
from bonsai.bim.module.model.wall import DumbWallJoiner
|
||||
|
||||
offenders: dict[str, set[str]] = {}
|
||||
for name, method in inspect.getmembers(DumbWallJoiner, predicate=inspect.isfunction):
|
||||
if not _method_writes_ifc_axis(method):
|
||||
continue
|
||||
bad_calls = _get_wall_axis_callers_in(method)
|
||||
if bad_calls:
|
||||
offenders[name] = bad_calls
|
||||
|
||||
assert not offenders, (
|
||||
f"DumbWallJoiner methods that call self.set_axis must not read the "
|
||||
f"bound-box-derived axis: {offenders}. Use "
|
||||
"tool.Wall.get_world_reference_line for world-space endpoints, or "
|
||||
"ifcopenshell.util.representation.get_reference_line for local-SI "
|
||||
"endpoints. Mixing bound_box on input with IFC axis on output "
|
||||
"produces non-colinear sub-axes that compound through chained "
|
||||
"extend/split/join edits."
|
||||
)
|
||||
|
||||
|
||||
def test_extend_walls_to_polyline_set_origin_uses_ifc_reference_line():
|
||||
"""``ExtendWallsToPolylinePoint.set_origin`` seeds the polyline preview
|
||||
anchor at one of the wall's axis endpoints. The downstream operator
|
||||
(``DumbWallJoiner.extend``) projects the user's chosen target onto the
|
||||
IFC reference line; if the preview anchor comes from
|
||||
``tool.Model.get_wall_axis`` (bound_box) the user sees the preview at
|
||||
one endpoint and the wall lands at a different one — the visible
|
||||
"extend falls short by a few cm/m" symptom."""
|
||||
from bonsai.bim.module.model.wall import ExtendWallsToPolylinePoint
|
||||
|
||||
offenders = _get_wall_axis_callers_in(ExtendWallsToPolylinePoint.set_origin)
|
||||
|
||||
assert not offenders, (
|
||||
f"ExtendWallsToPolylinePoint.set_origin must not read the bound-box-derived "
|
||||
f"axis: {offenders}. Use tool.Wall.get_world_reference_line so the preview "
|
||||
"anchor lands on the same IFC reference line the downstream extend operator "
|
||||
"projects onto."
|
||||
)
|
||||
|
||||
|
||||
def test_wall_toggle_openings_uses_idle_slots():
|
||||
"""The wall's toggle_openings icon must be declared in
|
||||
``GizmoWallEdition.idle_slots`` so the base class lays it out at the
|
||||
standard pen-row position. Routing it through ad-hoc setup helpers
|
||||
instead would re-introduce the X-collision with the array's first
|
||||
per-layer icon — the bug this contract was added to prevent."""
|
||||
from bonsai.bim.module.model.wall import GizmoWallEdition
|
||||
|
||||
slot_names = {s.name for s in GizmoWallEdition.idle_slots}
|
||||
assert "toggle_openings" in slot_names, (
|
||||
"GizmoWallEdition.idle_slots must contain a slot named 'toggle_openings'. "
|
||||
"The base class derives its X position from the slot's tuple index so peer "
|
||||
"groups (GizmoArrayEdition's per-layer icons) can query a real layout edge "
|
||||
"via _idle_row_right_edge() instead of a hardcoded per-feature table."
|
||||
)
|
||||
|
||||
|
||||
def test_no_pen_row_toggle_openings_helpers_remain():
|
||||
"""The legacy ``setup_pen_row_toggle_openings_icon`` and
|
||||
``update_pen_row_toggle_openings_icon`` helpers were removed once
|
||||
toggle_openings migrated into the ``idle_slots`` system. A re-introduced
|
||||
helper would shadow the slot-driven layout — features calling it would
|
||||
set up a second gizmo at a different X and the collision-prevention
|
||||
contract would silently regress.
|
||||
|
||||
Walks the wall and roof modules (the historical callers) plus
|
||||
drawing/gizmos.py (the historical home) for any reference to either
|
||||
name."""
|
||||
import bonsai.bim.module.drawing.gizmos as gizmos_mod
|
||||
import bonsai.bim.module.model.roof as roof_mod
|
||||
import bonsai.bim.module.model.wall as wall_mod
|
||||
|
||||
forbidden = ("setup_pen_row_toggle_openings_icon", "update_pen_row_toggle_openings_icon")
|
||||
for mod in (gizmos_mod, roof_mod, wall_mod):
|
||||
source = inspect.getsource(mod)
|
||||
for name in forbidden:
|
||||
assert name not in source, (
|
||||
f"{mod.__name__} still references {name!r}. The toggle_openings icon "
|
||||
f"is now declared via idle_slots; the ad-hoc helpers were removed to "
|
||||
f"prevent layout drift between feature groups."
|
||||
)
|
||||
|
||||
|
||||
def test_array_idle_max_x_walks_registry_not_hardcoded_dict():
|
||||
"""``GizmoArrayEdition._resolve_feature_idle_max_x`` must query peer
|
||||
parametric gizmo groups' ``_idle_row_right_edge`` rather than indexing
|
||||
a hardcoded per-feature ``_FEATURE_IDLE_MAX_X`` dict. The dict approach
|
||||
was the source of the toggle_openings ↔ array-layer-icon collision bug
|
||||
on arrayed walls (find_for_element returns 'array' first, shadowing the
|
||||
wall reservation)."""
|
||||
from bonsai.bim.module.model.array import GizmoArrayEdition
|
||||
|
||||
assert not hasattr(GizmoArrayEdition, "_FEATURE_IDLE_MAX_X"), (
|
||||
"GizmoArrayEdition._FEATURE_IDLE_MAX_X was a hardcoded per-feature dict "
|
||||
"that shadowed peer groups' real idle rows for compound elements (arrayed "
|
||||
"walls). It was replaced by a registry walk via REGISTRY + "
|
||||
"_idle_row_right_edge() — re-introducing the dict would re-create the bug."
|
||||
)
|
||||
|
||||
source = inspect.getsource(GizmoArrayEdition._resolve_feature_idle_max_x)
|
||||
assert "_idle_row_right_edge" in source, (
|
||||
"_resolve_feature_idle_max_x must call peer_cls._idle_row_right_edge() so "
|
||||
"the X position derives from each peer's actual declared idle_slots."
|
||||
)
|
||||
assert "REGISTRY" in source, (
|
||||
"_resolve_feature_idle_max_x must iterate BaseParametricGizmoGroup.REGISTRY "
|
||||
"to discover peer groups; find_for_element returns ONE entry and shadows "
|
||||
"compound-element memberships."
|
||||
)
|
||||
|
||||
@@ -588,6 +588,10 @@ class TestGenerateStair2DProfile(NewFile):
|
||||
|
||||
|
||||
class TestUsingArrays(NewFile):
|
||||
@staticmethod
|
||||
def _array_objects() -> list[bpy.types.Object]:
|
||||
return [o for o in bpy.data.objects if (e := tool.Ifc.get_entity(o)) and e.is_a("IfcActuator")]
|
||||
|
||||
def setup_array(self, add_second_layer=False, sync_children=False):
|
||||
tool.Project.get_project_props().template_file = "0"
|
||||
bpy.ops.bim.create_project()
|
||||
@@ -619,9 +623,9 @@ class TestUsingArrays(NewFile):
|
||||
def test_remove_array_last_to_first(self):
|
||||
self.setup_array(add_second_layer=True)
|
||||
bpy.ops.bim.remove_array(item=1)
|
||||
assert len(bpy.context.selected_objects) == 4
|
||||
assert len(self._array_objects()) == 4
|
||||
bpy.ops.bim.remove_array(item=0)
|
||||
assert len(bpy.context.selected_objects) == 1
|
||||
assert len(self._array_objects()) == 1
|
||||
|
||||
def test_remove_array_first_to_last(self):
|
||||
self.setup_array(add_second_layer=True)
|
||||
@@ -647,7 +651,7 @@ class TestUsingArrays(NewFile):
|
||||
bpy.ops.bim.apply_array() # apply second layer
|
||||
bpy.ops.bim.apply_array() # apply first layer
|
||||
|
||||
objs = bpy.context.selected_objects
|
||||
objs = self._array_objects()
|
||||
assert len(objs) == 12
|
||||
|
||||
# check BBIM_Array psets are removed
|
||||
|
||||
Reference in New Issue
Block a user