mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 19:58:50 +00:00
committed by
Sergey Matyunin
parent
a68e4c6b80
commit
728c564e5f
@@ -0,0 +1,282 @@
|
||||
# CODE-REVIEW-582-r1
|
||||
|
||||
Issue: [#582](https://github.com/Matysh/houseplan-card/issues/582) · этап: code · заход r1 ·
|
||||
блокирующих циклов израсходовано 0 из 4 · материал: `0a6592f4112fd7ceef68cbd3be7c44eeb8a7ef3e`
|
||||
(`origin/dev` = `bae6afaae7bac2dd45bca52b6caf64a16d96f182`, `origin/dev..HEAD` = `b9bf3daa`
|
||||
(документ ревью ТЗ) + `0a6592f4` (сама правка)).
|
||||
|
||||
## Скоуп
|
||||
|
||||
ТЗ (в теле issue) чинит воспроизводимый в HA Companion дефект: во время pinch-zoom
|
||||
пространства с фоном «Следует за Солнцем» появляются фиксированные белые/прозрачные
|
||||
области, вызванные тем, что постоянный `will-change: filter` из #532 висит на
|
||||
внутренней группе `.hp-paperg`, чей локальный SVG-bbox при `cell_cm: 1` может достигать
|
||||
тысяч единиц — Chromium промотирует слой пропорционально этому координатному размеру, а
|
||||
не экранному. Контракт (п.1–7) требует: сохранить визуальный контур и его перфоманс-выигрыш
|
||||
из #532, но привязать промотированный слой к экранному viewport независимо от `cell_cm`, и
|
||||
сделать то же самое для `houseplan-space-card`. AC1–AC9 в теле issue.
|
||||
|
||||
Это основная View-поверхность (J1) и touch-контракт View/kiosk (`docs/TOUCH-SUPPORT.md`,
|
||||
release-blocking), так что вопрос "работает ли оно на самом деле" здесь не формальность.
|
||||
|
||||
## Материал и что сделано в коде
|
||||
|
||||
- Новый общий рендерер `src/render/paper-scene.ts` (`renderPaperShapes`) — заменяет три
|
||||
дублированные ветки `path/poly/rect` в `houseplan-card.ts` и `space-render.ts`.
|
||||
- В `houseplan-card.ts` и `space-render.ts` добавлен ОТДЕЛЬНЫЙ `<svg class="hp-paper-outline-svg">`
|
||||
сосед перед основным `plan-svg`/`hp-static-plan-svg`, содержащий копию бумажных фигур;
|
||||
он использует тот же `viewBox`/`data-hp-live-viewbox`, что и основной план, и тот же
|
||||
`isoFloorMatrixCss()` в изометрии.
|
||||
- В `plan.styles.ts` тройной `drop-shadow` и `will-change: filter` перенесены с
|
||||
`.hp-paperg` на `.hp-paper-outline-svg` (стейдж-размерный корневой `<svg>`, растянутый
|
||||
правилом `.zoomwrap > svg { width:100%; height:100% }`), тем самым слой теперь
|
||||
действительно ограничен экранными пикселями, а не локальными SVG-единицами.
|
||||
- `space-card.ts` получил такое же правило для `.hp-static-stage .hp-paper-outline-svg`.
|
||||
- Новый браузерный смок `demo/smoke_daycycle_layer_budget.mjs` (CDP LayerTree + screencast)
|
||||
— свидетель AC1/AC3. Новый мутант `daycycle-outline-promoted-on-inner-paper` (guard —
|
||||
этот смок) — свидетель AC2. `smoke_daycycle_raster.mjs`/`smoke_bg_color.mjs` обновлены
|
||||
под новые имена классов — свидетели AC4/AC5. Новый `test/paper-scene-contract.test.mjs`
|
||||
— свидетель AC7. `scripts/smoke-links.mjs` регистрирует связь `renderPaperShapes` → 4
|
||||
смока. `scripts/bundle-budget.mjs` подвинул потолок на 200 Б с объяснением. Оба changelog
|
||||
и `docs/ARCHITECTURE.md`/`docs/SUN.md`/`docs/TOUCH-SUPPORT.md`/`docs/TESTING.md` обновлены
|
||||
согласованно с кодом. Трейлеры коммита `Issue: #582` / `User-Visible: yes` на месте.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Дешёвые гейты подтверждены зелёным Validate на точном SHA
|
||||
(https://github.com/Matysh/houseplan-card/actions/runs/34940487324, `workflow_dispatch`,
|
||||
без `heavy`): предполёт, классификация, «Фронтенд: типы/юниты/бандл», 6 шардов
|
||||
«Мутанты по диффу» — всё `success`. Тяжёлые джобы (`golden`, `smoke`, `performance_smoke`,
|
||||
`hacs`, `hassfest`, `geometry_parity`, `backend`) в этом прогоне **skipped** — не PR,
|
||||
не `Release:`, не `full=true`, поэтому они не гейтили этот пуш. Именно эти пропущенные
|
||||
джобы оказались важны (см. находки).
|
||||
|
||||
Я лично прогнал (то, что Validate не покрыл, и что явно относится к диффу):
|
||||
|
||||
| Гейт | Результат | Почему прогнан/не прогнан |
|
||||
|---|---|---|
|
||||
| `npm run bundle:sync` (typecheck+build+rollup+bundle-tree) | зелёный, `git status` после — чисто | дёшево, диф трогает `src/**` |
|
||||
| `node demo/smoke_daycycle_layer_budget.mjs` (AC1/AC3) | **зелёный**, 4 прогона подряд | прямое совпадение с диффом |
|
||||
| `node demo/smoke_bg_color.mjs` | **зелёный** (все 47 проверок, включая изменённую `staticCardLayersStayOrdered`) | зарегистрированная связь, CI её не гонял (heavy=false) |
|
||||
| `node demo/smoke_live_pan_coverage.mjs` | **зелёный** | зарегистрированная связь, CI её не гонял |
|
||||
| `node demo/smoke_daycycle_raster.mjs` (AC4, свидетель #532) | **красный, воспроизводимо 8/8** (ratio 2.16–2.50 против потолка 2.0); на `origin/dev` тот же смок даёт 1.01–1.14 в 7/7 запусках в ЭТОМ ЖЕ окружении | прямое совпадение с диффом; CI отчитался зелёным на этом SHA, но локальный результат детерминирован и объяснён (см. Находка №1) |
|
||||
| `npm run golden:verify` (AC6, полная матрица) | **4 из 4 day-cycle сцен = `different`**, остальные ~150 сцен `passed` | diff меняет рендер; CI `golden` был skipped на этом пуше, так что это первый реальный прогон на данном SHA |
|
||||
| `npm run inventory` | не гонял | не требуется для этого раунда |
|
||||
| `python -m pytest tests_backend -q`, HACS, Hassfest, geometry parity | не гонял | диф не трогает `custom_components/**/*.py`, манифесты или геометрию решётки/толщины стен |
|
||||
| `performance_smoke` (полный) | не гонял | AC4 уже красный на более дешёвом целевом свидетеле; полный перф-профиль не добавит новой информации до фикса |
|
||||
|
||||
Плюс диагностика причины (CDP `LayerTree` + `compositingReasons`, одна и та же гарнитура
|
||||
`demo/serve.mjs`, один и тот же демо-фикстур 1400×900, сравнение `origin/dev` vs `HEAD`
|
||||
бок о бок в одном окружении) — вынесена в Находку №1.
|
||||
|
||||
## Находки
|
||||
|
||||
### Находка №1 (High) — AC4 не держится: день-цикл при pan рерастеризует весь план, не только контур
|
||||
|
||||
**Файлы:** `src/houseplan-card.ts:11597-11603`, `src/styles/plan.styles.ts:97-105`,
|
||||
`src/space-render.ts` (тот же паттерн для static card).
|
||||
|
||||
**Механизм.** Раньше `will-change: filter` стоял на `.hp-paperg` — `<g>` ВНУТРИ
|
||||
`plan-svg`, с малым собственным bbox. `plan-svg` целиком в свой отдельный
|
||||
compositor-слой не попадал (в baseline `origin/dev` слой `plan-svg` в CDP `LayerTree`
|
||||
вообще не значится: контент рисуется в общей поверхности). Патч кладёт контур в
|
||||
ОТДЕЛЬНЫЙ `<svg class="hp-paper-outline-svg">`-СОСЕД перед `plan-svg` в том же
|
||||
`.zoomwrap`. Chromium корректно ограничивает промотированный слой контура экранными
|
||||
пикселями (это и было целью, AC1/AC2/AC3 — подтверждены), но побочный эффект: теперь
|
||||
`plan-svg` (ВЕСЬ видимый план — стены, комнаты, устройства, декор) визуально
|
||||
перекрывает промотированный слой-соседа и по правилам Chromium сам получает
|
||||
отдельный compositor-слой с причиной `"Overlaps other composited content"`
|
||||
(проверено CDP `LayerTree.compositingReasons` на обоих деревьях, один и тот же демо-стенд,
|
||||
1400×900):
|
||||
|
||||
```
|
||||
origin/dev (до патча): hp-paperg 920×720 (will-change: filter); НЕТ отдельного слоя plan-svg
|
||||
0a6592f4 (после патча): hp-paper-outline-svg 780×724 (will-change: filter);
|
||||
plan-svg 780×724 ("Overlaps other composited content") ← новый слой
|
||||
```
|
||||
|
||||
Это ПОЛНЫЙ план как отдельная растеризуемая поверхность, которая теперь должна
|
||||
перерисовываться при любом визуальном изменении плана во время жеста — то есть именно
|
||||
тот класс стоимости, который #532 устранял для внешнего контура, теперь возвращается
|
||||
для всего плана.
|
||||
|
||||
**Численное подтверждение (изолированный прогон, 20 шагов панорамы мышью, тот же
|
||||
демо-стенд, RasterTask по CDP Tracing):**
|
||||
|
||||
```
|
||||
origin/dev: 31.8 мс, 106 RasterTask
|
||||
0a6592f4: 74.7 мс, 282 RasterTask (≈2.35× по времени, ≈2.7× по числу тасков)
|
||||
```
|
||||
|
||||
**Свидетель AC4 (`demo/smoke_daycycle_raster.mjs`), 60-шаговая панорама, то же
|
||||
окружение, 8 прогонов на `HEAD` против 7 на `origin/dev`:**
|
||||
|
||||
| Дерево | ratio (потолок 2.0) |
|
||||
|---|---|
|
||||
| `origin/dev` | 1.09, 1.14, 1.03, 1.03, 1.04, 1.01 (все ≤ 1.14) |
|
||||
| `0a6592f4` | 2.19, 2.16, 2.23, 2.18, 2.50, 2.33 (все ≥ 2.16) |
|
||||
|
||||
Разброс между двумя деревьями непересекается ни разу за 13 запусков — это не шум
|
||||
конкретной машины, а систематическая разница, воспроизводимая в контролируемом A/B
|
||||
внутри ОДНОГО окружения (обе половины смока меряются в одном процессе на одной
|
||||
странице, как и задумано автором смока).
|
||||
|
||||
**Почему CI на этом SHA отчитался зелёным, а не потому что находки нет.** Job
|
||||
«Мутанты по диффу» реально исполняет `node demo/smoke_daycycle_raster.mjs` (лог
|
||||
`ok чистый прогон: node demo/smoke_daycycle_raster.mjs`, 07:14:24) и код проверки
|
||||
(`serve.mjs: finish()`) действительно ставит `process.exitCode=1` при `ratio > 2.0` —
|
||||
то есть CI-раннер в моменте объективно получил ratio ≤ 2.0. Проблема в марже: порог
|
||||
2.0 был откалиброван «примерно посередине по логарифму» между 7.9 (без подсказки
|
||||
вообще) и 0.26 (подсказка на тесной `.hp-paperg`, комментарий в самом смоке, строка 15).
|
||||
Новая реализация переехала с 0.26 на ~2.2–2.5 — то есть съела почти весь запас,
|
||||
оставленный именно на случай машинной вариативности, и на GitHub-раннере это на
|
||||
сегодня чуть-чуть укладывается, а на менее мощном/более шумном исполнителе — уже нет.
|
||||
Целевая платформа этой самой задачи — Android HA Companion WebView — как правило
|
||||
СЛАБЕЕ настольного CI-раннера, а не сильнее; то есть именно то устройство, ради
|
||||
которого чинится #582, статистически более рискованно по AC4, чем моя тестовая машина.
|
||||
|
||||
**Последствие.** AC4 сформулирован как "свидетель #532 остаётся зелёным" — де-факто он
|
||||
не остаётся зелёным как класс поведения (плана целиком, а не только контура), просто
|
||||
конкретный числовой порог на конкретном раннере пока не перешагнут. Это разваливает и
|
||||
собственное объяснение ТЗ в "Технические ограничения": "Фильтр/промоушен должен
|
||||
применяться на уровне экранной scene surface, а не координатно большой внутренней
|
||||
группы" — технически верно для самого контура, но не учитывает, что помещение этой
|
||||
scene surface СОСЕДОМ (а не потомком) плана заставляет браузер промотировать и сам план.
|
||||
|
||||
### Находка №2 (High) — AC6 не проходит: все 4 day-cycle golden-сцены отличаются от эталона на каноничной платформе
|
||||
|
||||
**Файлы:** `demo/golden/baselines/day-cycle-{dawn,day,dusk,night}-dark.png` (не
|
||||
обновлены в этом диффе — `git diff origin/dev...HEAD --stat -- demo/golden/` пуст).
|
||||
|
||||
`npm run golden:verify` (Linux, тот же движок, что CI/golden — каноничная платформа
|
||||
по AGENTS.md) на точном SHA даёт:
|
||||
|
||||
| Сцена | статус | differing px | diffRatio | maxObservedDelta | порог diffRatio |
|
||||
|---|---|---|---|---|---|
|
||||
| day-cycle-dawn-dark | different | 26 903 | 0.0349 | 81 | 0.0005 |
|
||||
| day-cycle-day-dark | different | 26 160 | 0.0339 | 81 | 0.0005 |
|
||||
| day-cycle-dusk-dark | different | 25 667 | 0.0333 | 81 | 0.0005 |
|
||||
| day-cycle-night-dark | different | 26 142 | 0.0339 | 94 | 0.0005 |
|
||||
|
||||
Это ~70× превышение допустимой доли различающихся пикселей и разница каналов до 94/255
|
||||
при допуске 10/255 — не антиалиасинг-дребезг на границе допуска. Визуально (сравнение
|
||||
`demo/golden/baselines/day-cycle-day-dark.png` и `artifacts/golden/actual/day-cycle-day-dark.png`
|
||||
на глаз почти неотличимы; diff-маска в `artifacts/golden/diff/day-cycle-day-dark.png`
|
||||
подсвечивает штриховку буквально ВСЕХ стен по всему плану, не только внешний контур)
|
||||
похоже на тот же механизм, что и Находка №1: как только `plan-svg` стал отдельным
|
||||
композитным слоем, финальная сборка кадра проходит через дополнительный проход
|
||||
растеризации/блендинга этого слоя, слегка меняя суб-пиксельное позиционирование штриховки
|
||||
стен по всему плану — отсюда широкий, но мелкий по амплитуде на глаз разброс.
|
||||
|
||||
Оставшиеся ~150 сцен матрицы прошли (`passed`), включая не-day-cycle сцены — регрессия
|
||||
локализована именно в day-cycle рендере, как и предсказывает механизм Находки №1.
|
||||
|
||||
AC6 прямо требует: "четыре существующие day-cycle сцены... проходят... Любое
|
||||
необходимое обновление baseline допускается только после объяснённого визуального
|
||||
сравнения". Baseline не обновлён, объяснения в issue/коммите нет — сам факт диффа не
|
||||
скрывается (никто не подделывал числа), но и не заявлен: ни в диффе, ни в комментариях
|
||||
issue нет упоминания, что `golden:verify` был прогнан и дал результат. Раз diff явно
|
||||
трогает рендер (`src/**`: `houseplan-card.ts`, `space-render.ts`, `paper-scene.ts`,
|
||||
`plan.styles.ts`), `golden:verify` был обязателен перед `S7-code-review`
|
||||
(AGENTS.md, "проверь по diff и AC" + сам этот процесс-документ).
|
||||
|
||||
**Обе находки объединяет один и тот же корень**: помещение дневного контура СОСЕДНИМ
|
||||
`<svg>` перед `plan-svg` вместо ВНУТРИ него как раньше — технически верно решает
|
||||
координатно-огромный слой (AC1–AC3 в порядке), но по пути превращает весь план в
|
||||
отдельный композитный слой, что стоит и лишней растеризации (Находка №1), и лишнего
|
||||
прохода блендинга, видимого в пикселях (Находка №2). Возможные направления для автора
|
||||
(не предписание, только ориентир): не выносить контур отдельным сиблингом, а найти
|
||||
способ ограничить экранными размерами именно промотированный слой ВНУТРИ существующего
|
||||
дерева `plan-svg` (например, через `clipPath`/`contain`, привязанный к экранному
|
||||
`viewBox`, а не к локальному bbox фигуры), либо явно и всегда держать `plan-svg` на
|
||||
собственном стабильном слое ДО появления контура, чтобы контур не мог его "продавить"
|
||||
через overlap-эвристику при первом появлении.
|
||||
|
||||
### Находка №3 (Low, не блокирует, оставлена без правки)
|
||||
|
||||
`src/houseplan-card.ts:11623-11624` — комментарий "One `<g>` around ALL paper shapes:
|
||||
the external day-cycle outline (styles.ts) is composited once for the whole sheet"
|
||||
относится к `.hp-paperg`, но сам фильтр туда больше не прикреплён (перенесён на
|
||||
`.hp-paper-outline-svg`, отдельный узел). Смысл абзаца (одна группа = отсутствие швов
|
||||
между комнатами) остаётся верным, но фраза про "the external day-cycle outline...
|
||||
is composited" на этом самом `<g>` — уже не так. Мелкая правка комментария, не код.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **AC1/AC2/AC3** — подтверждены и CI-логом мутанта (`daycycle-outline-promoted-on-inner-paper`,
|
||||
чистый прогон ok + красный на мутанте ok, 07:14:05–07:14:50 на точном SHA), и моим
|
||||
локальным повторным запуском `smoke_daycycle_layer_budget.mjs` (4/4 зелёных): контур
|
||||
теперь 826×736 (экранный порядок) вместо потенциальных 4700×4200; ни один content-слой
|
||||
> 4096; 9/9 отснятых кадров pinch без белых пикселей.
|
||||
- **Единый живой viewport (#579, контракт п.3–4).** `data-hp-live-viewbox` стоит на
|
||||
`.hp-paper-outline-svg` тем же значением (`camera`/`floor`), что на `plan-svg`;
|
||||
`live-viewport.ts:paintLiveViewport` обновляет оба через один и тот же
|
||||
`querySelectorAll` и одну и ту же проекцию — контур и план не могут разойтись по
|
||||
кадру или масштабу отдельно от терминального примирения. Проверено чтением, не
|
||||
исполнением отдельно от смоков выше (которые это же утверждение покрывают AC3).
|
||||
- **Основная бумага не фильтруется (контракт «Технические ограничения»).**
|
||||
`renderPaperShapes(paperShapes)` без `groupClass` даёт `.hp-paperg` без фильтра;
|
||||
`test/paper-scene-contract.test.mjs` проверяет отсутствие `will-change: filter` на
|
||||
`.hp-paperg`; смок `smoke_daycycle_raster.mjs` подтверждает
|
||||
`dayCyclePaperStaysUnfiltered: true` в моём прогоне.
|
||||
- **Изометрия и static card не потеряли зеркальность.** `<g transform=${iso ?
|
||||
isoFloorMatrixCss() : nothing}>` оборачивает `renderPaperShapes` в контурном SVG тем
|
||||
же способом, что в основном; `hp-static-stage` получил аналогичное правило z-index/
|
||||
overflow/pointer-events; `smoke_bg_color.mjs` (`staticCardLayersStayOrdered`) зелёный.
|
||||
- **`plan-svg` z-index стал безусловным** (было `class=${iso ? 'plan-svg' : nothing}`,
|
||||
стало `class="plan-svg"`) — раньше во flat-режиме класс не вешался вовсе и элемент
|
||||
зависел от порядка DOM для стэкинга; теперь это явное `z-index: 1` против `0` у
|
||||
контура. Не регрессия: до этого диффа конкурирующего элемента с явным z-index не было,
|
||||
после — есть (сам контур), и явный класс корректно фиксирует стэкинг. `test/isometric-contract.test.mjs`
|
||||
обновлён синхронно.
|
||||
- **Changelog/трейлеры/документация.** `Issue: #582` + `User-Visible: yes` на
|
||||
терминальном коммите; оба `docs/CHANGELOG*.md` правлены в нём же; `docs/ARCHITECTURE.md`,
|
||||
`docs/SUN.md`, `docs/TOUCH-SUPPORT.md`, `docs/TESTING.md` описывают именно то, что
|
||||
делает код (сверено построчно с диффом кода). `scripts/bundle-budget.mjs` — потолок
|
||||
подвинут на 200 Б с содержательным объяснением, общий бюджет не тронут.
|
||||
- **Мутационная регистрация.** `daycycle-outline-promoted-on-inner-paper` привязан к
|
||||
правильному guard-файлу и реально ловит возврат старого `will-change` (подтверждено
|
||||
и в CI, и локально).
|
||||
|
||||
## Чего не проверял и почему
|
||||
|
||||
- `python -m pytest tests_backend`, HACS, Hassfest, geometry-parity — diff не трогает
|
||||
`custom_components/**/*.py`, манифесты, `layout`/`marker.space`/толщину стен; эти
|
||||
гейты были в Validate `skipped` по классификации файлов, а не по heavy-флагу, что и
|
||||
ожидаемо для чисто фронтенд-диффа.
|
||||
- Полный `performance_smoke`/профили вне `smoke_daycycle_raster.mjs` — AC4 уже
|
||||
провалена на целевом, более дёшевом свидетеле; гонять более тяжёлый профиль до
|
||||
фикса не добавляет решающей информации.
|
||||
- `npm run inventory` — не требуется для вынесения вердикта этого раунда.
|
||||
- Полная browser-smoke матрица (250 файлов) — вне AC и вне выборки `smoke-select.mjs`
|
||||
(4 зарегистрированные связи, все 4 прогнаны явно, см. таблицу выше).
|
||||
- AC9 (полевая приёмка на реальном Android WebView владельцем в следующей бете) —
|
||||
вне этого этапа по определению самого AC.
|
||||
|
||||
## Вывод
|
||||
|
||||
Реализация корректно решает координатно-зависимый размер промотированного слоя
|
||||
(AC1–AC3), сохраняет единый live-viewport и визуальный контракт контура на уровне
|
||||
кода, документация и трейлеры в порядке. Но выбранная архитектура (контур —
|
||||
СОСЕДНИЙ `<svg>` перед планом, а не элемент внутри него) имеет побочный эффект:
|
||||
`plan-svg` целиком продавливается в отдельный композитный слой, что воспроизводимо
|
||||
проваливает собственный AC4 (перф-свидетель #532) вне узкой марки конкретного
|
||||
CI-раннера и приводит к measurable расхождению во всех 4 golden-сценах AC6 на
|
||||
каноничной Linux-платформе, не сопровождённому обновлением baseline или объяснением.
|
||||
Обе находки — High, обе в скоупе задачи (эта же ветка чинит то, что сама же
|
||||
сформулировала как риск: "Исправление WebView может регрессировать
|
||||
Firefox-производительность #532"). Возврат автору.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/582-webview-large-filter-layers`, коммит `0a6592f4112f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `9f4d95168fdda9650fa7d35f17d80c7177a7d12b`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 9f4d95168fdd
|
||||
```
|
||||
- Тело issue: `4c1e1ecfe8ce205f3cfb3c44eb6028a35d0bb4358323e05ba60c6fd85b3b40ba`
|
||||
- Вердикт конвейера: `red` · High 2
|
||||
Reference in New Issue
Block a user