mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -0,0 +1,257 @@
|
||||
# CODE-REVIEW-249-r1
|
||||
|
||||
- Issue: [#249](https://github.com/Matysh/houseplan-card/issues/249) — ограниченная геометрия узла из трёх и более стен
|
||||
- Этап: code (PROCESS.md §2.7)
|
||||
- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (этот заход не расходует бюджет: вердикт красный)
|
||||
- Коммит на ревью: `062a98a1d841a5de633392357dcf0d4264ed3620` ("fix: bound multi-wall junction bevels")
|
||||
- ТЗ: `docs/specs/249-multiwall-junction-bevel.md`, редакция r2 (SPEC-REVIEW-249-r2, зелёный)
|
||||
- Материал: `git diff origin/dev...HEAD`, полный (не delta-review — это первый заход этапа code)
|
||||
|
||||
## Скоуп проверки
|
||||
|
||||
Прочитаны: `docs/SCOPE.md` (J1), `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`, ТЗ
|
||||
`docs/specs/249-multiwall-junction-bevel.md` (все 13 разделов, включая §9 AC1–AC7),
|
||||
тело issue #249 и все комментарии (аналитика, продуктовые Q&A, ТЗ, оба раунда
|
||||
spec-ревью, отчёт автора о реализации). Просмотрен весь diff
|
||||
`src/wall-thickness.ts` (416 добавленных/изменённых строк), новый fixture,
|
||||
`test/wall-thickness.test.mjs` (переписанные и новые тесты),
|
||||
`demo/smoke_multiwall_junction.mjs`, изменения golden matrix/harness/test,
|
||||
обновления `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`, `docs/TESTING.md`,
|
||||
`docs/CHANGELOG.md`/`.ru.md`.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Гейты реализации (соразмерны диапазону diff — трогает `src/**`, требует
|
||||
check-docs; смоки выбраны по AC4 и по инструменту `smoke-select.mjs`, а не
|
||||
прогнаны полным набором):
|
||||
|
||||
| Гейт | Результат | Прогнал |
|
||||
|---|---|---|
|
||||
| `npx tsc --noEmit` | зелёный | да |
|
||||
| `npm test` | 1117 passed / 0 failed / 0 skipped (автор заявлял 1116+1 skip — итог тестов совпадает, расхождение по skip не расследовано, не блокирует) | да |
|
||||
| `npm run build` | зелёный | да |
|
||||
| SHA-256 трёх копий бандла | совпадают: `9e2c89fd...eefe05ec7` | да, сверил вручную |
|
||||
| `node scripts/check-docs.mjs` | зелёный (7 файлов, 10 внешних ссылок) | да, обязателен — diff трогает `src/**` |
|
||||
| `node demo/smoke_multiwall_junction.mjs` | зелёный, 15/15 | да — прямой AC4 гейт |
|
||||
| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | нашёл 1 прямое совпадение: `smoke_decor_layer_order.mjs` (символ `roomPoly`) | да |
|
||||
| `node demo/smoke_decor_layer_order.mjs` | зелёный, 26/26 | да |
|
||||
| `node demo/smoke_split_corner_wall.mjs` | зелёный, 14/14 | да — тема пересекается с новым junction-кодом |
|
||||
| `node demo/smoke_junction_patch_resilience.mjs` | зелёный, 14/14 | да |
|
||||
| `node demo/smoke_wall_junctions.mjs` | зелёный, 12/12 | да |
|
||||
|
||||
**Не прогонялось** (предрелизные гейты по PROCESS.md §8, не гейт ревью):
|
||||
`npm run golden:verify` (полный), `npm run golden:accept`, performance-профили,
|
||||
полный `smokes:select`/167 смоков, backend/HA-harness (Python не тронут).
|
||||
Golden baseline не принимался — соответствует отчёту автора.
|
||||
|
||||
Все заявленные автором числа (SHA-256 бандла, счётчики smoke JSON, состав
|
||||
`smoke-select`) подтверждены самостоятельным прогоном, а не переписаны со слов
|
||||
автора.
|
||||
|
||||
Дополнительно я самостоятельно воспроизвёл геометрию (см. находку H1) через
|
||||
compiled `test-build/wall-thickness.js`, вызывая экспортированные
|
||||
`roomWallProfile`/`insetContour`/`outsetContour`/`buildMultiWallNodeMap`
|
||||
напрямую, и сравнил результат с тем же вызовом на `origin/dev` (через
|
||||
временный `git worktree`) — оба прогона выполнены, артефакты и worktree
|
||||
удалены до завершения ревью, в репозитории не оставлено файлов.
|
||||
|
||||
## Находки
|
||||
|
||||
### H1 (High, блокирует). `innerContourForRoom` (clean floor) получает
|
||||
геометрически неверную bevel-точку на узлах с несимметричной толщиной
|
||||
сходящихся стен — floor выходит за пределы контура здания
|
||||
|
||||
**Точка отказа:** `src/wall-thickness.ts:1066-1069` (fallback-ветка
|
||||
`insetContour`/`outsetContour`, идентичный код в обеих функциях) —
|
||||
существующий (не новый) код, который для чрезмерной митры кладёт две точки
|
||||
`poly[i] + normal·offset` независимо для каждого ребра, без проверки, что
|
||||
результат остаётся внутри многоугольника. Раньше эта ветка почти не
|
||||
срабатывала для узлов 3+ (порог был `MITRE_LIMIT×maxO`, то есть до `4×`
|
||||
толщины), поэтому дефект был латентным. #249 делает порог `1.25×H` — то есть
|
||||
именно тем механизмом, который теперь регулярно уводит обычные T/L-стыки
|
||||
перегородки с внешней стеной в эту ветку, — и подставляет туда несимметричные
|
||||
толщины (перегородка много толще внешней стены), при которых точка одного
|
||||
ребра уходит по нормали на всю величину offset этого ребра, что при остром угле
|
||||
между рёбрами пробивает противоположную сторону многоугольника насквword.
|
||||
|
||||
**Воспроизведение (числами, не «на глаз»):** тот же `cornerSplitFixture`, что
|
||||
уже используется в тестах `test/wall-thickness.test.mjs` (`outerCm: 15`,
|
||||
`dividerCm: 100`, path по умолчанию — то есть комбинация, реально
|
||||
прогоняемая гейтом «corner Split preserves facade... every positive-thickness
|
||||
3-ray matrix», строка `outerCm=15, dividerCm=100`):
|
||||
|
||||
```
|
||||
node — узел (100, 100), угол между наружным ребром и перегородкой ≈ 26.6°
|
||||
H = 41.6667 (полутолщина перегородки), R = 1.25×H = 52.0833
|
||||
|
||||
roomWallProfile('source').poly = [[100,100],[900,100],[900,500]]
|
||||
roomWallProfile('source').offsets = [6.25, 6.25, 41.6667]
|
||||
|
||||
insetContour(poly, offsets, multiWallNodes) →
|
||||
[118.63389981249824, 62.7322003750035], ← точка floor ВНЕ здания
|
||||
[100, 106.25],
|
||||
[893.75, 106.25],
|
||||
[893.75, 450.2902504687544]
|
||||
```
|
||||
|
||||
Комната "source" — часть прямоугольника `y ∈ [100,700]`. Точка
|
||||
`[118.63, 62.73]` имеет `y = 62.73 < 100`, то есть лежит **выше верхнего
|
||||
края здания**, вне контура вообще (не просто вне комнаты — вне здания
|
||||
целиком). Расстояние точки от узла — ровно `41.6667` (халф-толщина
|
||||
перегородки), то есть формально `≤ R` (52.08), поэтому проверка §7.2.4
|
||||
(«join-вершина не дальше `R + epsilon`») этот дефект не ловит: контракт
|
||||
ограничивает только *расстояние*, а не сторону многоугольника, а фактическая
|
||||
точка получена смещением от узла по нормали чужого (перегородочного) ребра на
|
||||
всю его полутолщину — без проверки, что это не пробивает соседнее ребро.
|
||||
|
||||
Union-уровневый `multiWallBevelTriangles`/`bevelMultiWallBody` (новый код
|
||||
#249, добавляющий вырезание лишнего клина из финального wall body) эту же
|
||||
несимметрию на этом узле не ломает так явно, потому что `wallBodiesGeometry`
|
||||
дополнительно пересекает итоговое тело с `exterior.centre`
|
||||
(`src/wall-thickness.ts:2364-ish`, `if (body && exterior) body =
|
||||
intersection(body, exterior.centre)`), что подрезает вылезшую геометрию
|
||||
обратно к контуру здания. У `innerContourForRoom` (clean floor) такой
|
||||
защитной пересечки нет вовсе — функция отдаёт результат `insetContour`
|
||||
напрямую (после диффа: `src/wall-thickness.ts` в районе
|
||||
`innerContourForRoom`), поэтому дефект долетает до потребителя без всякой
|
||||
подрезки.
|
||||
|
||||
**Проверка «было/стало»:** тот же вызов на `origin/dev` (до #249, через
|
||||
временный `git worktree`, тот же `cornerSplitFixture`, идентичный код helper'ов)
|
||||
даёт `insetContour` без всплеска — расхождение строгого инварианта
|
||||
`floor_union == original.poly − wall_body` составляет `≈ 6.5e-11` (шум
|
||||
плавающей точки, тест на dev проходит с допуском `1e-7`). На коммите #249 то
|
||||
же расхождение — `≈ 1686.24` (на 10 порядков больше допуска), причём вся
|
||||
дельта — это именно тот вырвавшийся за пределы здания клин.
|
||||
|
||||
**Почему это не поймано авторскими гейтами:** этот же fixture
|
||||
(`outerCm=15`, `dividerCm=100`) используется в тесте
|
||||
`test/wall-thickness.test.mjs` — было
|
||||
`'corner Split clean floors are exactly the room union minus canonical
|
||||
walls'` со строгим `closeTo(geometryDifferenceArea(actual, expected), 0,
|
||||
1e-7)` в обоих направлениях. Автор **заменил** этот тест на
|
||||
`'corner Split clean-floor contours use the same bounded bevel endpoints'`
|
||||
(новое имя), где вместо точного геометрического равенства теперь проверяется
|
||||
только «каждая bevel-точка присутствует где-то среди вершин floor-контура»
|
||||
(`floors.some((floor) => floor.some((point) => distance < 1e-7))`) — эта
|
||||
проверка тривиально проходит, даже когда floor-контур неверно вышел за
|
||||
пределы здания, поскольку сама вырвавшаяся точка — это ровно та «bevel-точка»,
|
||||
которую тест ищет. Ни `demo/smoke_multiwall_junction.mjs`
|
||||
(`cleanFloorConsumerIsPresent` проверяет только наличие path, не его форму),
|
||||
ни новый golden-сценарий (`fill_mode: 'none'` — floor не рисуется вовсе)
|
||||
дефект не покрывают. AC4 ТЗ прямо называет floor/room fills/room hover
|
||||
каноническим потребителем той же исправленной геометрии (§7.4) — это
|
||||
нарушено.
|
||||
|
||||
**Почему это Blocking, а не косметика:** это ровно тот же класс дефекта, для
|
||||
которого написано ТЗ (клин, вылезающий за пределы стены), только
|
||||
переехавший с внешнего контура кладки на внутренний контур пола, причём для
|
||||
совершенно реалистичной конфигурации — перегородка заметно толще наружной
|
||||
стены, встречающая её под острым/умеренным углом. Инструмент, которым это
|
||||
создаётся («Split» комнаты по диагонали с последующей установкой толщины
|
||||
перегородки), — штатный редакторский путь, не синтетика теста.
|
||||
|
||||
**Что нужно автору:** переоценить fallback-ветку `insetContour`/
|
||||
`outsetContour` (строки 1066–1069) для случаев, когда единичное смещение по
|
||||
нормали одного ребра на всю величину его offset выходит за пределы,
|
||||
образуемые соседним ребром — либо ограничивать длину смещения проекцией на
|
||||
соседнее ребро/дистанцией до узла, либо (как минимум) вернуть
|
||||
`innerContourForRoom` защитное пересечение с исходным полигоном комнаты,
|
||||
аналогичное тому, что `wallBodiesGeometry` уже делает для wall body. Отдельно
|
||||
нужно вернуть строгую (не ослабленную) проверку инварианта
|
||||
`floor_union == room_union − canonical_wall_body`, которая существовала до
|
||||
этого коммита — она и обнаружила бы дефект сама.
|
||||
|
||||
---
|
||||
|
||||
### M1 (Medium, в скоупе — чинится в этом же цикле). AC2 не покрыт буквально
|
||||
заявленным кейсом «три стены 15/50/70 см» (три РАЗНЫЕ толщины у трёх лучей)
|
||||
|
||||
Спецификация (§9, AC2) требует минимум четыре сценария, включая «трёх стен
|
||||
15/50/70 см» — то есть узел, где ВСЕ ТРИ луча имеют разную толщину (без
|
||||
повторов). Матрица в `test/wall-thickness.test.mjs`
|
||||
(`'issue #249 node classification is order, direction and scale
|
||||
independent'`) покрывает: 3 равных луча, 3 луча с halves `[7,5,5]`
|
||||
(**только два разных значения** — 7 и 5,5), 4 равных луча, 4 луча
|
||||
`[2,5,3,7]` (все разные). Ни один 3-лучевой случай с тремя различными
|
||||
попарными halfDepth не тестируется явно; сам fixture AC1 тоже
|
||||
50/50/70 см (две толщины совпадают). Кейс `[7,5,5]` покрывает оба
|
||||
направления одной несимметричной пары через обход по кругу, но не покрывает
|
||||
тройку из трёх взаимно разных величин (нет пары «средний-большой» без
|
||||
повторов), которую владелец в issue привёл как конкретный числовой пример
|
||||
(«например 15/50/70»).
|
||||
|
||||
Учитывая находку H1 (именно несимметрия толщин на многолучевом узле — источник
|
||||
дефекта), это не формальная придирка: тест с тремя различными halfDepth,
|
||||
особенно при остром угле, с высокой вероятностью тоже поймал бы H1 или его
|
||||
аналог. Чинится точечно — один дополнительный кейс в существующем
|
||||
`cases`-массиве с тремя различными halves, без новых продуктовых вопросов.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **AC1** (экспортный fixture #249, узел 50/50/70): unit
|
||||
`'issue #249 bounds the exported three-wall junction with straight
|
||||
bevels'` — воспроизведён и зелёный; `H = 4.8611`, все join-вершины `≤
|
||||
1.25×H + epsilon`, один связный компонент, повторный расчёт даёт тот же
|
||||
результат, вход не мутируется (deep-equal до/после). Для ЭТОГО конкретного
|
||||
узла (толщины 50/50/70, угол не острый) floor-консьюмер H1 не проявляет —
|
||||
проверил отдельно (`innerContourForRoom` на этом fixture даёт точки на
|
||||
разумном расстоянии 3.5–4.9 от узла, без выхода за пределы), дефект
|
||||
специфичен для более несимметричных/острых конфигураций.
|
||||
- **AC3** (двухлучевые узлы не меняются): существующий 2-лучевой набор
|
||||
тестов не тронут (кроме одного, №197-фикстура, у которого расширенный
|
||||
fixture содержит собственный узел степени 3+ — это отдельный, задокументированный
|
||||
в тесте, легитимный сдвиг площади, не 2-лучевой регресс). Отдельный
|
||||
тест `twoRay` подтверждает `insetContour`/`outsetContour` с пустой
|
||||
multi-wall картой идентичны вызову без карты.
|
||||
- **AC4** (общий body для всех поверхностей): `demo/smoke_multiwall_junction.mjs`
|
||||
зелёный по всем 15 полям — Plan/View/kiosk/Static/hidden-Iso совпадают
|
||||
байт-в-байт по пути, light barriers используют ту же masonry, HA-tick и
|
||||
смена темы не перестраивают topology (кэш переиспользуется).
|
||||
- **AC5** (golden фиксирует видимый результат): новый сценарий
|
||||
`multiwall-junction-bevel-view-dark` с semantic assertions (узел заполнен,
|
||||
отброшенный клин пуст, 3 луча) — не может пройти на пустом/неверном кадре;
|
||||
версия матрицы поднята (38→39); `test/golden-matrix.test.mjs` проверяет
|
||||
форму сценария.
|
||||
- **AC6** (данные не мутируются): подтверждено deep-equality до/после в новом
|
||||
unit-тесте и в `'corner Split rendering does not materialize or mutate
|
||||
saved geometry'`.
|
||||
- **AC7** (гейты реализации): все четыре зелёные, включая build/typecheck —
|
||||
перепроверено самостоятельно, не только со слов автора.
|
||||
- **Документация**: `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`,
|
||||
`docs/TESTING.md` обновлены по существу (не формально) и соответствуют
|
||||
фактическому коду; `docs/CHANGELOG.md`/`.ru.md` оба обновлены в этом же
|
||||
коммите при `User-Visible: yes`; трейлеры `Issue: #249` и
|
||||
`User-Visible: yes` на месте.
|
||||
- **Union-уровневая коррекция** (`multiWallBevelTriangles`/
|
||||
`bevelMultiWallBody`) корректна для симметричных/сбалансированных узлов
|
||||
(проверено чтением кода и подтверждено зелёными таргетированными unit- и
|
||||
smoke-тестами) — проблема локализована именно в старой fallback-ветке
|
||||
`insetContour`/`outsetContour`, не в новой union-логике.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Полный `npm run golden:verify`/`golden:accept`, полный набор из 167 смоков,
|
||||
performance-профили — предрелизные гейты по PROCESS.md §8, не обязаны быть
|
||||
прогнаны на этом заходе; риск R1 самой спеки («широкий blast radius для
|
||||
T-стыков») явно требует именно полного golden-прогона перед бетой, и находка
|
||||
H1 показывает, что этот прогон обязателен и по причине, шире описанной в
|
||||
спеке — не только визуальный дрейф, но и потенциальный явный дефект.
|
||||
Golden-сценарий #249 не поймает H1 — там `fill_mode: 'none'`.
|
||||
Не устанавливал/не запускал этот набор сам.
|
||||
- Не искал системно другие узлы (существующие golden fixtures, реальные
|
||||
сохранённые планы) с похожей несимметрией толщин/острым углом, где H1 мог
|
||||
бы проявиться иначе (например, в `openingWallIndex`/tunnel-геометрии рядом с
|
||||
таким узлом) — за рамками того, что доказывает конкретный repro.
|
||||
- Backend/Python — не тронут диффом, HA-harness не требовался и не запускал.
|
||||
|
||||
## Резюме
|
||||
|
||||
Реализация в целом добросовестно покрывает контракт ТЗ (единая node map,
|
||||
union-уровневая коррекция клина, документация, golden/smoke на заявленном
|
||||
экспортном сценарии) — но именно в том месте, где спецификация явно
|
||||
предупреждала о риске (§11 R2: «фаска может создать щель или ложную
|
||||
кладку… визуально малый дефект»), обнаружен подтверждённый воспроизведением
|
||||
дефект (H1), который автор не поймал, потому что ослабил единственный тест,
|
||||
способный его обнаружить. Это блокирующая находка — заход не может быть
|
||||
зелёным.
|
||||
Reference in New Issue
Block a user