mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
refactor: one function owns the junction geometry of a space (#229 r2 M1)
Дефект High-1 был одинаковым в двух местах — и в живом рисовании, и в
«Оптимизировать планы», — потому что каждый вызывающий собирал геометрию
примыканий сам. Ревью r2 справедливо заметило, что и защита получилась
однобокой: юнит и мутант сторожили только оптимизатор, а путь карты — тот, где
дефект и был виден пользователю, — не сторожил никто. Заплатка в виде второго
мутанта-близнеца оставила бы причину на месте: два списка координат, которые
обязаны совпадать, но ничем не связаны.
Поэтому геометрия переехала в `spaceMergeGeometry(space, { excludeDraftId })`:
один источник комнат, колонн и концов черновиков, одни координаты, одно место,
где можно ошибиться. Оба вызывающих теперь строчка вызова.
Покрытие идёт за причиной, а не за симптомом: три юнита в
`test/wall-merge.test.mjs` проверяют масштаб полигонов (включая комнаты в форме
x/y/w/h и комнату без геометрии), исключение активного черновика и сам T-стык к
середине стороны комнаты. Мутанты `partition-merge-rescales-rooms` и
`chain-merge-sees-own-draft` перенацелены на общий модуль и теперь краснеют для
обоих путей сразу: 2 и 1 падение, проверено применением патча.
Сценарий с комнатой в смоке пробовал — не взлетел: рисование в комнату
поднимает `_offerWallFaces`, и цепочка не завершается штатно. Ломать смок под
тест не стал, юниты общего модуля покрывают оба пути честнее.
Issue: #229
User-Visible: no
This commit is contained in:
File diff suppressed because one or more lines are too long
@@ -73,6 +73,7 @@ const res = await page.evaluate(async () => {
|
||||
out.touchedWallGrew = !!extended && Math.abs(extended.b[0] * 1000 - 700) < 0.01;
|
||||
out.draftIsGone = !(space().room_drafts || []).length;
|
||||
|
||||
|
||||
return out;
|
||||
});
|
||||
checkAll(res);
|
||||
|
||||
File diff suppressed because one or more lines are too long
Vendored
+45
-45
File diff suppressed because one or more lines are too long
@@ -1,7 +1,7 @@
|
||||
{
|
||||
"version": 1,
|
||||
"fixture": "synthetic-only",
|
||||
"sourceFingerprint": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceFingerprint": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"captureScriptSha256": "34f2219790d46efd8250e7a1bd829cb8fc0b0547e1260635fefa52407551b41b",
|
||||
"command": "npm run build && node demo/docs/capture.mjs",
|
||||
"scenarios": {
|
||||
@@ -13,7 +13,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "d7ce576f5a70b1977277e15985bb409f40f205f6016eeebc62f76a4394effdb4"
|
||||
},
|
||||
"view-touch": {
|
||||
@@ -24,7 +24,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "62c13b7f9dc6c0576735a727a1988bc35b9b28fc2a9d34e9a746a4dc3e01b28a"
|
||||
},
|
||||
"space-create": {
|
||||
@@ -35,7 +35,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "a73bb2301677752c7980712af554d40f3fd6077944249677245c8135d325b1b9"
|
||||
},
|
||||
"room-contour-close": {
|
||||
@@ -46,7 +46,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "8f234c7750809cbc75b3ce364dc2d3b55eebacd4b2b5e42b223d6e2cee9bb4ac"
|
||||
},
|
||||
"plan-context-tray": {
|
||||
@@ -57,7 +57,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "3eee486fe5cc226e8b3c19c4035ebb279087432a69959d24b7edabc59d848582"
|
||||
},
|
||||
"device-editor": {
|
||||
@@ -68,7 +68,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "a6360309a9a6125bdc2dfd411463d281af2f21a09b07e58eae1fc23d7371b8e5"
|
||||
},
|
||||
"device-display-preview": {
|
||||
@@ -79,7 +79,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "4476e223863379ef9dfd0a1c89fb4c169a1249268e733ac11e5a2eec8d90f8f5"
|
||||
},
|
||||
"background-editor": {
|
||||
@@ -90,7 +90,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "a83926911c2d6d468c81dfdf74be7e00cbbca9a02afade6e6b5644bdf4f46852"
|
||||
},
|
||||
"room-card": {
|
||||
@@ -101,7 +101,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "48a5685039a5e0de2a28190d857b193c86e2e63606e3c7a8c40fc9d2b190da4e"
|
||||
},
|
||||
"device-info": {
|
||||
@@ -112,7 +112,7 @@
|
||||
},
|
||||
"theme": "dark",
|
||||
"language": "en",
|
||||
"sourceSha256": "afa20fd81ac20e56571c5fc16e5f881b9fac8ead93f923b52d132c853f55cc48",
|
||||
"sourceSha256": "fc61db49515bfed7d6f6a7cb6f2011fafc323e3a75d048244fb02f0e3d39587d",
|
||||
"imageSha256": "307b2ba224cd0b55c2e2936767a16cdda7af3ddffd292705f20c5a300e56c648"
|
||||
}
|
||||
}
|
||||
|
||||
@@ -719,27 +719,27 @@ export const MUTANTS = [
|
||||
},
|
||||
{
|
||||
id: 'partition-merge-rescales-rooms',
|
||||
guard: 'node --test --test-name-pattern="issue 229" test/plan-optimizer.test.mjs',
|
||||
guard: 'node --test --test-name-pattern="issue 229" test/wall-merge.test.mjs',
|
||||
because: 'комнаты хранятся в тех же координатах, что перегородки: лишнее деление '
|
||||
+ 'уносит их в угол и примыкание к стене комнаты перестаёт находиться '
|
||||
+ '(CODE-REVIEW-229-r1, High-1)',
|
||||
patches: [{
|
||||
file: 'src/plan-optimizer.ts',
|
||||
find: ` .filter((poly: number[][] | null): poly is number[][] => !!poly),`,
|
||||
replace: ` .filter((poly: number[][] | null): poly is number[][] => !!poly)
|
||||
.map((poly: number[][]) => poly.map((p) => [p[0] / NORM_W, p[1] / NORM_W])),`,
|
||||
file: 'src/wall-merge.ts',
|
||||
find: ` .filter((poly: number[][] | null): poly is number[][] => !!poly),`,
|
||||
replace: ` .filter((poly: number[][] | null): poly is number[][] => !!poly)
|
||||
.map((poly: number[][]) => poly.map((p) => [p[0] / 1000, p[1] / 1000])),`,
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'chain-merge-sees-own-draft',
|
||||
guard: 'node demo/smoke_wall_chain_merge.mjs',
|
||||
guard: 'node --test --test-name-pattern="issue 229" test/wall-merge.test.mjs',
|
||||
because: 'завершаемая цепочка ещё лежит в room_drafts, и её собственные концы '
|
||||
+ 'нельзя принимать за чужое примыкание — иначе стык с существующей стеной '
|
||||
+ 'никогда не срастается (CODE-REVIEW-229-r1, High-2)',
|
||||
patches: [{
|
||||
file: 'src/houseplan-card.ts',
|
||||
find: ' if (draft?.id && draft.id === this._activeDraftId) return [];',
|
||||
replace: ' void 0;',
|
||||
file: 'src/wall-merge.ts',
|
||||
find: ' if (exclude && draft?.id === exclude) return [];',
|
||||
replace: ' void exclude;',
|
||||
}],
|
||||
},
|
||||
{
|
||||
|
||||
+2
-18
@@ -265,7 +265,7 @@ import {
|
||||
applySpaceOrder, canStartTabDrag, markersNeedingPlacement, passedDragThreshold,
|
||||
reorderSpaceIds,
|
||||
} from './space-order';
|
||||
import { applyOpeningMoves, mergeCollinearPartitions } from './wall-merge';
|
||||
import { applyOpeningMoves, mergeCollinearPartitions, spaceMergeGeometry } from './wall-merge';
|
||||
|
||||
const CARD_VERSION = '1.66.0';
|
||||
const DISPLAY_LABEL_KEYS: Record<DeviceDisplayMode, I18nKey> = {
|
||||
@@ -6548,26 +6548,10 @@ class HouseplanCard extends LitElement {
|
||||
private _mergeSpacePartitions(sp: any, seedIds?: string[]): number {
|
||||
const partitions = (sp?.partitions || []) as PartitionCfg[];
|
||||
if (partitions.length < 2) return 0;
|
||||
const rooms = (sp.rooms || []) as any[];
|
||||
const result = mergeCollinearPartitions(partitions, {
|
||||
pitch: GRID_STEP_N,
|
||||
seedIds,
|
||||
geometry: {
|
||||
// Same coordinates as the partitions — `roomPoly` returns the raw
|
||||
// config polygon (review CODE-REVIEW-229-r1, High-1).
|
||||
roomPolygons: rooms
|
||||
.map((room) => roomPoly(room))
|
||||
.filter((poly): poly is number[][] => !!poly),
|
||||
columns: sp.wall_columns || [],
|
||||
// The chain being finished is still persisted as a draft at this
|
||||
// point, and its own ends must not pass for someone else's junction —
|
||||
// the same exclusion plan-snap-overlay makes (review r1, High-2).
|
||||
draftEnds: (sp.room_drafts || []).flatMap((draft: any) => {
|
||||
if (draft?.id && draft.id === this._activeDraftId) return [];
|
||||
const points = draft?.points || [];
|
||||
return points.length ? [points[0], points[points.length - 1]] : [];
|
||||
}),
|
||||
},
|
||||
geometry: spaceMergeGeometry(sp, { excludeDraftId: this._activeDraftId }),
|
||||
});
|
||||
if (!result.merged) return 0;
|
||||
sp.partitions = result.partitions;
|
||||
|
||||
+2
-15
@@ -24,7 +24,7 @@ import {
|
||||
degradeWalls, normalizeWallIntervals, rekeyWallsAfterMove, roomWallProfile,
|
||||
setWallThickness, type WallEntry,
|
||||
} from './wall-thickness';
|
||||
import { applyOpeningMoves, mergeCollinearPartitions } from './wall-merge';
|
||||
import { applyOpeningMoves, mergeCollinearPartitions, spaceMergeGeometry } from './wall-merge';
|
||||
|
||||
/** Bump when a new lossless maintenance pass is added. */
|
||||
export const PLAN_MODEL_VERSION = 6;
|
||||
@@ -502,20 +502,7 @@ export function optimizePlans(configIn: any, layoutIn: Record<string, any>): Opt
|
||||
// plan finally loses them — explicitly, with a report and an undo (#229).
|
||||
const partitionMerge = mergeCollinearPartitions(space.partitions || [], {
|
||||
pitch: GRID_STEP_N,
|
||||
geometry: {
|
||||
// Rooms are stored in the same coordinates as partitions: `roomPoly`
|
||||
// hands back the raw config polygon, so scaling it here would push
|
||||
// every room into a corner and no junction would ever be found
|
||||
// (review CODE-REVIEW-229-r1, High-1).
|
||||
roomPolygons: (space.rooms || [])
|
||||
.map((room: any) => roomPoly(room))
|
||||
.filter((poly: number[][] | null): poly is number[][] => !!poly),
|
||||
columns: space.wall_columns || [],
|
||||
draftEnds: (space.room_drafts || []).flatMap((draft: any) => {
|
||||
const points = draft?.points || [];
|
||||
return points.length ? [points[0], points[points.length - 1]] : [];
|
||||
}),
|
||||
},
|
||||
geometry: spaceMergeGeometry(space),
|
||||
});
|
||||
if (partitionMerge.merged) {
|
||||
partitionsMerged += partitionMerge.merged;
|
||||
|
||||
@@ -16,6 +16,7 @@
|
||||
|
||||
import type { OpeningCfg, PartitionCfg, WallColumnCfg } from './types';
|
||||
import { materializePartitionOpening, resolvePartitionOpeningCompat } from './partition-openings';
|
||||
import { roomPoly } from './logic';
|
||||
|
||||
/** Collinearity, as a fraction of one grid pitch (spec §8.3). */
|
||||
export const EPS_ANGLE = 0.02;
|
||||
@@ -260,3 +261,35 @@ export function applyOpeningMoves(
|
||||
}
|
||||
return moved;
|
||||
}
|
||||
|
||||
/**
|
||||
* The junction geometry of one space, in the coordinates the partitions use.
|
||||
*
|
||||
* Both callers — the live editor and “Optimize plans” — need exactly the same
|
||||
* three things, and when each built them for itself the identical mistake was
|
||||
* made twice: room polygons were scaled a second time and no room junction was
|
||||
* ever found (review CODE-REVIEW-229-r1/r2). One function, one set of
|
||||
* coordinates, one place to get it wrong.
|
||||
*
|
||||
* `excludeDraftId` is the chain being finished right now: it is already
|
||||
* persisted in `room_drafts`, and its own ends are not a foreign junction —
|
||||
* the same exclusion `plan-snap-overlay` makes.
|
||||
*/
|
||||
export function spaceMergeGeometry(
|
||||
space: any, options?: { excludeDraftId?: string | null },
|
||||
): MergeGeometry {
|
||||
const exclude = options?.excludeDraftId || null;
|
||||
return {
|
||||
// `roomPoly` hands back the raw config polygon: rooms are stored in the
|
||||
// same normalised coordinates as partitions, so nothing is rescaled here.
|
||||
roomPolygons: (space?.rooms || [])
|
||||
.map((room: any) => roomPoly(room))
|
||||
.filter((poly: number[][] | null): poly is number[][] => !!poly),
|
||||
columns: space?.wall_columns || [],
|
||||
draftEnds: (space?.room_drafts || []).flatMap((draft: any) => {
|
||||
if (exclude && draft?.id === exclude) return [];
|
||||
const points = draft?.points || [];
|
||||
return points.length ? [points[0], points[points.length - 1]] : [];
|
||||
}),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ import test from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import {
|
||||
applyOpeningMoves, EPS_JOIN, mergeCollinearPartitions, junctionAt, remapHostT,
|
||||
spaceMergeGeometry,
|
||||
} from '../test-build/wall-merge.js';
|
||||
|
||||
const PITCH = 10; // one grid pitch in test coordinates
|
||||
@@ -210,3 +211,52 @@ test('issue 229 openings hosted elsewhere are left alone', () => {
|
||||
assert.equal(moved, 0);
|
||||
assert.equal(JSON.stringify(openings), before);
|
||||
});
|
||||
|
||||
test('issue 229 space geometry keeps rooms in the coordinates partitions use', () => {
|
||||
// Both callers used to build this themselves and both rescaled the polygon,
|
||||
// so a junction on a room side was never found (CODE-REVIEW-229-r1 High-1,
|
||||
// r2 Medium-1). One function now, checked in the units both paths share.
|
||||
const space = {
|
||||
rooms: [
|
||||
{ id: 'r1', poly: [[0.1, 0.1], [0.5, 0.1], [0.5, 0.5], [0.1, 0.5]] },
|
||||
{ id: 'r2', x: 0.6, y: 0.1, w: 0.2, h: 0.2 },
|
||||
{ id: 'broken' },
|
||||
],
|
||||
wall_columns: [{ id: 'c1', center: [0.7, 0.7] }],
|
||||
};
|
||||
const geometry = spaceMergeGeometry(space);
|
||||
assert.deepEqual(geometry.roomPolygons[0], [[0.1, 0.1], [0.5, 0.1], [0.5, 0.5], [0.1, 0.5]]);
|
||||
assert.deepEqual(geometry.roomPolygons[1][0], [0.6, 0.1], 'x/y/w/h rooms come through too');
|
||||
assert.equal(geometry.roomPolygons.length, 2, 'a room without geometry is skipped');
|
||||
assert.deepEqual(geometry.columns, space.wall_columns);
|
||||
});
|
||||
|
||||
test('issue 229 the chain being finished is not its own junction', () => {
|
||||
const space = {
|
||||
rooms: [],
|
||||
room_drafts: [
|
||||
{ id: 'active', points: [[0.1, 0.1], [0.3, 0.1]] },
|
||||
{ id: 'other', points: [[0.6, 0.6], [0.8, 0.6]] },
|
||||
],
|
||||
};
|
||||
assert.deepEqual(
|
||||
spaceMergeGeometry(space).draftEnds,
|
||||
[[0.1, 0.1], [0.3, 0.1], [0.6, 0.6], [0.8, 0.6]],
|
||||
'without an active chain every draft anchors a node',
|
||||
);
|
||||
assert.deepEqual(
|
||||
spaceMergeGeometry(space, { excludeDraftId: 'active' }).draftEnds,
|
||||
[[0.6, 0.6], [0.8, 0.6]],
|
||||
'the chain about to disappear does not hold a node',
|
||||
);
|
||||
});
|
||||
|
||||
test('issue 229 a junction on a room side survives, through the shared geometry', () => {
|
||||
const partitions = [seg('p1', 100, 500, 300, 500), seg('p2', 300, 500, 500, 500)];
|
||||
const space = { rooms: [{ id: 'r1', x: 100, y: 100, w: 400, h: 400 }] };
|
||||
const result = mergeCollinearPartitions(partitions, {
|
||||
pitch: PITCH, geometry: spaceMergeGeometry(space),
|
||||
});
|
||||
assert.equal(result.merged, 0, 'the middle of the room side holds the node');
|
||||
assert.equal(result.partitions.length, 2);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user