mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-09-24 09:16:53 +00:00
Fix set_icon_gizmo_position so billboard ignores object rotation
set_icon_gizmo_position computed ``mw @ (Translation @ billboard_rot @ Scale)`` — the object's world matrix was applied AFTER the billboard rotation, so any non-trivial object rotation (e.g. a wall rotated in plan, a stair rotated to match a corridor) carried over into the icon's transform and tilted it edge-on to the camera instead of facing it. Switch to ``billboarded_at(world_pos, billboard_rot, scale)`` where ``world_pos = mw @ local_pos``: translate to world space first, then apply the billboard rotation independently of the object's rotation. This matches the manual pattern the base class's ``update_editing_gizmos`` already uses for validate/cancel/cycle for exactly this reason. Drops the now-stale workaround docstring on ``GizmoWallEdition._update_icon_row_extras`` that documented why it bypassed ``set_icon_gizmo_position`` — the helper does the right thing now. Adds ``test/bim/module/model/test_stair_gizmos.py`` as the regression guard: parametrised over six rotation angles, asserts that the rotation part of the resulting matrix equals ``billboard_rot`` (no contribution from ``mw``'s rotation) and that the translation lands at ``world_pos``. Also exercises ``set_icon_gizmo_position`` end-to-end via a stub gizmo to catch the exact shape of the previously-broken call site. Generated with the assistance of an AI coding tool.
This commit is contained in:
committed by
Thomas Krijnen
parent
95a31b49ec
commit
5d6878c321
@@ -4847,8 +4847,8 @@ class BaseParametricGizmoGroup:
|
|||||||
scale: Gizmo scale factor (default 0.5)
|
scale: Gizmo scale factor (default 0.5)
|
||||||
"""
|
"""
|
||||||
if gz := self.get_gizmo_if_visible(gizmo_name):
|
if gz := self.get_gizmo_if_visible(gizmo_name):
|
||||||
local_transform = Matrix.Translation(Vector((x, y, z))) @ billboard_rot @ Matrix.Scale(scale, 4)
|
world_pos = mw @ Vector((x, y, z))
|
||||||
gz.matrix_basis = mw @ local_transform
|
gz.matrix_basis = billboarded_at(world_pos, billboard_rot, scale)
|
||||||
|
|
||||||
def set_dimension_gizmo_position(
|
def set_dimension_gizmo_position(
|
||||||
self,
|
self,
|
||||||
|
|||||||
@@ -1969,13 +1969,8 @@ class GizmoWallEdition(bpy.types.GizmoGroup, gizmo.BaseParametricGizmoGroup):
|
|||||||
- Toggle-openings icon next to the pen. Lives outside edit mode because
|
- Toggle-openings icon next to the pen. Lives outside edit mode because
|
||||||
opening visibility is a viewport-display concern, not a wall-edit action.
|
opening visibility is a viewport-display concern, not a wall-edit action.
|
||||||
|
|
||||||
Uses the manual ``Translation(world_pos) @ billboard_rot @ Scale`` pattern
|
Uses ``billboarded_at`` directly for parity with the base class's
|
||||||
rather than the base class's ``set_icon_gizmo_position`` helper. The helper
|
``update_editing_gizmos`` validate/cancel/cycle pattern."""
|
||||||
computes ``mw @ (Translation @ billboard_rot @ Scale)``, which applies the
|
|
||||||
wall's rotation to the billboard — for a wall rotated in plan, the icons
|
|
||||||
end up tilted edge-on to the camera instead of facing it. The base class's
|
|
||||||
own ``update_editing_gizmos`` already uses the manual pattern for validate/
|
|
||||||
cancel/cycle for exactly this reason; we match it here."""
|
|
||||||
if not hasattr(self, "rotate_gizmo"):
|
if not hasattr(self, "rotate_gizmo"):
|
||||||
return
|
return
|
||||||
gizmo_prefs = self.get_gizmo_prefs()
|
gizmo_prefs = self.get_gizmo_prefs()
|
||||||
|
|||||||
@@ -0,0 +1,128 @@
|
|||||||
|
# Bonsai - OpenBIM Blender Add-on
|
||||||
|
# Copyright (C) 2026
|
||||||
|
#
|
||||||
|
# This file is part of Bonsai.
|
||||||
|
#
|
||||||
|
# Bonsai is free software: you can redistribute it and/or modify
|
||||||
|
# it under the terms of the GNU General Public License as published by
|
||||||
|
# the Free Software Foundation, either version 3 of the License, or
|
||||||
|
# (at your option) any later version.
|
||||||
|
#
|
||||||
|
# Bonsai 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 General Public License for more details.
|
||||||
|
#
|
||||||
|
# You should have received a copy of the GNU General Public License
|
||||||
|
# along with Bonsai. If not, see <http://www.gnu.org/licenses/>.
|
||||||
|
#
|
||||||
|
# This file was generated with the assistance of an AI coding tool.
|
||||||
|
|
||||||
|
"""Regression guard for the stair icon billboard fix.
|
||||||
|
|
||||||
|
Before the fix, ``set_icon_gizmo_position`` in ``bim.module.drawing.gizmos``
|
||||||
|
composed ``mw @ (Translation @ billboard_rot @ Scale)``, which applied the
|
||||||
|
stair's world rotation on top of the billboard rotation. The result was
|
||||||
|
icons (validate / cancel / lock / +/- / cycle / tread_lock) drawn edge-on
|
||||||
|
to the camera for any stair rotated in plan — effectively unclickable.
|
||||||
|
|
||||||
|
The fix routes through ``billboarded_at(world_pos, billboard_rot, scale)``,
|
||||||
|
which computes ``Translation(world_pos) @ billboard_rot @ Scale`` — the
|
||||||
|
object's rotation is folded into the translation only, never the rotation."""
|
||||||
|
|
||||||
|
import math
|
||||||
|
import types
|
||||||
|
|
||||||
|
import bpy
|
||||||
|
import pytest
|
||||||
|
from mathutils import Matrix, Vector
|
||||||
|
|
||||||
|
pytestmark = pytest.mark.model
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(autouse=True)
|
||||||
|
def _require_real_bpy():
|
||||||
|
if not isinstance(bpy, types.ModuleType) or hasattr(bpy, "_mock_name"):
|
||||||
|
pytest.skip("requires real Blender (bpy is mocked or absent)")
|
||||||
|
|
||||||
|
|
||||||
|
def _rotation_close(a: Matrix, b: Matrix, tol: float = 1e-6) -> bool:
|
||||||
|
for row_a, row_b in zip(a, b):
|
||||||
|
for va, vb in zip(row_a, row_b):
|
||||||
|
if abs(va - vb) > tol:
|
||||||
|
return False
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("angle_deg", [0, 30, 45, 90, 135, 217])
|
||||||
|
def test_billboarded_at_rotation_is_pure_billboard(angle_deg):
|
||||||
|
"""Object rotation must not leak into the gizmo's rotation part."""
|
||||||
|
from bonsai.bim.module.drawing.gizmos import billboarded_at
|
||||||
|
|
||||||
|
mw = Matrix.Rotation(math.radians(angle_deg), 4, "Z") @ Matrix.Translation((3, 4, 5))
|
||||||
|
billboard_rot = Matrix.Rotation(math.radians(30), 4, "X")
|
||||||
|
|
||||||
|
world_pos = mw @ Vector((1, 0, 2))
|
||||||
|
result = billboarded_at(world_pos, billboard_rot, scale=0.5)
|
||||||
|
|
||||||
|
# The rotation part of result, after stripping the 0.5 uniform scale,
|
||||||
|
# must equal billboard_rot — no contribution from mw's rotation.
|
||||||
|
rotation_part = result.to_3x3() * 2.0
|
||||||
|
assert _rotation_close(rotation_part.to_4x4(), billboard_rot)
|
||||||
|
|
||||||
|
|
||||||
|
def test_billboarded_at_translation_is_world_pos():
|
||||||
|
"""Translation lands exactly at the world-space target."""
|
||||||
|
from bonsai.bim.module.drawing.gizmos import billboarded_at
|
||||||
|
|
||||||
|
world_pos = Vector((1.23, 4.56, 7.89))
|
||||||
|
result = billboarded_at(world_pos, Matrix.Identity(4), scale=0.5)
|
||||||
|
assert (result.translation - world_pos).length < 1e-6
|
||||||
|
|
||||||
|
|
||||||
|
def test_set_icon_gizmo_position_does_not_apply_object_rotation():
|
||||||
|
"""End-to-end: the helper used by every stair icon (and shared with all
|
||||||
|
parametric gizmo groups) must produce a matrix whose rotation part is
|
||||||
|
billboard_rot, not mw_rotation @ billboard_rot. This is the exact bug
|
||||||
|
that left stair icons edge-on to the camera."""
|
||||||
|
from bonsai.bim.module.drawing.gizmos import (
|
||||||
|
BaseParametricGizmoGroup,
|
||||||
|
billboarded_at,
|
||||||
|
)
|
||||||
|
|
||||||
|
# Same inputs as the real call site (stair.py:747-765), but we drive the
|
||||||
|
# helper directly so we don't need a registered GizmoGroup. We bind a
|
||||||
|
# stand-in `get_gizmo_if_visible` that returns a tiny mock; the helper's
|
||||||
|
# observable output is the matrix_basis it assigns.
|
||||||
|
captured = {}
|
||||||
|
|
||||||
|
class _GizmoStub:
|
||||||
|
matrix_basis: Matrix = Matrix.Identity(4)
|
||||||
|
|
||||||
|
stub = _GizmoStub()
|
||||||
|
|
||||||
|
def _fake_get(name):
|
||||||
|
captured["name"] = name
|
||||||
|
return stub
|
||||||
|
|
||||||
|
# Bind the helper to a throwaway instance so `self.get_gizmo_if_visible`
|
||||||
|
# resolves to our stub without registering a real GizmoGroup with Blender.
|
||||||
|
fake_self = types.SimpleNamespace(get_gizmo_if_visible=_fake_get)
|
||||||
|
BaseParametricGizmoGroup.set_icon_gizmo_position(
|
||||||
|
fake_self,
|
||||||
|
"validate_gizmo",
|
||||||
|
mw=Matrix.Rotation(math.radians(45), 4, "Z") @ Matrix.Translation((3, 4, 5)),
|
||||||
|
x=1.0,
|
||||||
|
y=0.0,
|
||||||
|
z=2.0,
|
||||||
|
billboard_rot=Matrix.Rotation(math.radians(30), 4, "X"),
|
||||||
|
scale=0.5,
|
||||||
|
)
|
||||||
|
|
||||||
|
expected_world_pos = (Matrix.Rotation(math.radians(45), 4, "Z") @ Matrix.Translation((3, 4, 5))) @ Vector((1, 0, 2))
|
||||||
|
expected = billboarded_at(expected_world_pos, Matrix.Rotation(math.radians(30), 4, "X"), 0.5)
|
||||||
|
|
||||||
|
assert captured["name"] == "validate_gizmo"
|
||||||
|
for row_a, row_b in zip(stub.matrix_basis, expected):
|
||||||
|
for va, vb in zip(row_a, row_b):
|
||||||
|
assert abs(va - vb) < 1e-6
|
||||||
Reference in New Issue
Block a user