Skip to content

Enhanced Room Editing - #307

Merged
nicosandller merged 17 commits into
mainfrom
feat/rectangle-rooms
Sep 21, 2026
Merged

nicosandller merged 17 commits into
mainfrom
feat/rectangle-rooms

Conversation

@TruthOf42

@TruthOf42 TruthOf42 commented Sep 15, 2026 •

Copy link
Copy Markdown
Collaborator

Rooms can now be drawn and edited as rectangles, instead of only as click-by-click polygons.

What's new

Draw. With the Area tool, press-drag-release draws a finished rectangle in one gesture — it snaps to the grid, auto-selects, and flips back to the Select tool. A plain click still starts a polygon, so nothing about the old workflow changes; the two are told apart by a short hold (AREA_DRAG_HOLD_MS) after the pointer starts moving. A draft that would land on top of an existing room is clamped away from it, and one that ends up too small is discarded rather than committed.

Resize. A selected rectangle gets edge handles as well as corner handles. Dragging an edge moves that wall while the opposite one stays put. Where two rooms share an edge, the neighbour follows, so abutting rooms stay coincident instead of tearing apart — including one long edge pushing against two shorter rooms, and a pushed group stopping before it overlaps a third room or collapses a neighbour. Locked rooms are left alone by coupling.

Walls and dividers. Double-clicking a rectangle's edge cycles it through wall → divider → none. A wall is a real wall: it blocks lamp light and sunlight and closes off dead space, in the card and in the editor preview alike. A divider is drawn as a dashed line and blocks nothing — for splitting an open plan visually without walling it off. Both are derived from the room, so they move with it and are never stored as separate wall entries.

Config

One new optional field on an area, written by the editor:

areas:
  - id: living
    points: [...]
    sideWalls:
      top: wall
      right: divider

Keys are top / right / bottom / left, values wall / divider / none. Absent on polygon rooms and on any YAML written before this change, so existing floorplans render exactly as they did. Documented in docs/configuration.md; the README section on areas covers the new gestures.

Issues

Closes #290. Closes #291. Covers the drawing and editing half of #288 — mapping area coloring to a room (#292) is not in scope here.

Testing

src/editor-geometry.test.ts covers the new geometry helpers (rectangle construction, corner and edge resize, overlap clamping, shared-edge detection and coupling, the wall/divider cycle). src/editor-drag.browser.test.ts covers the gestures in real Chromium — press-drag vs click, edge dragging and toggling, and the shared-edge cases above.

Open review items

See the review on this PR — the corner-drag path and a lock check still need fixing before merge.


Description added by the maintainer to document an existing PR; the implementation is @TruthOf42's.

🤖 Generated with Claude Code

@nicosandller nicosandller left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of #307 — rectangle rooms

Nice feature, and the test coverage is genuinely good (~1,100 lines of new tests). A few things to fix before merge.

Payment links

Checked first: unchanged. The Bitcoin and Ethereum addresses in .github/FUNDING.yml and the Buy Me A Coffee link in the README are byte-identical to main. This PR touches 6 files, all under src/.

Tests

All green. CI passes (build, browser-tests, validate-hacs). Locally: 1,635 unit tests and 132 browser tests pass, and npm run typecheck is clean.

Please fix (blocking)

1. The branch conflicts with main. src/editor.ts and src/floorplan-card.ts both conflict. Merge main in and re-push.

2. The README wasn't updated. README line 322 still tells people the old way:

Pick the Area tool and click each corner

That's no longer the whole story — you can now press-drag to get a finished rectangle in one gesture, and double-click a room's edge to cycle it through wall → divider → none. None of that is written down anywhere. docs/configuration.md also needs the new divider field on a wall.

3. autoWalls says it's runtime-only, but it gets saved. The comment in types.ts says:

Runtime-only rectangle edge state ... without exposing them as a user-facing config field

But _toggleRectAreaSide calls _updateArea, which calls _commitFloor — so autoWalls lands in the user's saved YAML. That's fine as a design choice, but then it's a real config field and should be documented like one. Either document it, or don't persist it. Right now the comment and the behaviour disagree, which will confuse the next person.

Small stuff (not blocking)

  • 0.001 appears 16 times across editor-geometry.ts and editor.ts. Give it a name. Note rectAreaSharedSides takes an epsilon parameter while rectAreaSharedEdgeCouple hardcodes the same value — worth making consistent.
  • The four-corner array is written out three times (rectAreaPoints, rectAreaEdgeResize, rectAreaClamp). The last two can just call rectAreaPoints.
  • thickness: 8 is hardcoded in rectAreaSideWalls. There's already WALL_THICKNESS = 8 in render.ts — use it, so the two can't drift apart.
  • The divider style string is duplicated: "stroke-width:2; stroke-dasharray:2 12; opacity:0.7;" appears verbatim in both editor.ts and floorplan-card.ts. Worth a shared helper next to wallStrokeStyle.
  • Use the types you just added. Array<"top" | "right" | "bottom" | "left"> is spelled out by hand in several spots even though RectAreaAutoWallSide now exists, and Partial<Record<RectAreaAutoWallSide, RectAreaAutoWallState>> is written out where the RectAreaAutoWalls alias would do. In _applyDrag, ["top","right","bottom","left"][idx] as ... could just be RECT_AREA_AUTO_WALL_SIDES[idx].
  • _renderWall does a full scan per wall. It rebuilds areas × 4 sides and runs .find() every time any wall renders, just to work out whether that wall is an auto-wall. Building the lookup once per render would be cheaper on a busy floorplan.

Screenshots

There's no PR description and no screenshots. Could you add a before/after? Not blocking, but it makes this kind of UI change much easier to review.

I ran the editor on both branches and confirmed the change works as intended:

  • Drawing: on main, a press-drag with the Area tool just places one point ("1 point placed — click to add more"). On this branch the same gesture produces a finished rectangle room, auto-selects it, and flips back to the Select tool.
  • Walls: on main, a room with autoWalls set renders as a plain fill (the field is ignored). On this branch the perimeter draws as solid walls, the shared boundary between two adjoining rooms draws as a dashed divider, and the diamond toggle handles appear at each edge midpoint.

(The HA docker container can't run in my sandbox, so I drove the editor directly in Chromium instead. Screenshots have gone to @nicosandller.)


Generated by Claude Code

@TruthOf42 TruthOf42 linked an issue Sep 15, 2026 that may be closed by this pull request
@TruthOf42

Copy link
Copy Markdown
Collaborator Author
Screencast.from.2026-09-14.21-56-08.webm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved correctness, locking, lifecycle, and generated-wall integration issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds drag-created rectangular rooms, rectangle resizing/shared-edge coupling, and configurable wall/divider edges.

Changes:

  • Extends area and wall metadata.
  • Adds rectangle geometry and editor interactions.
  • Updates rendering and adds unit/browser tests.
File summaries
File Summary
src/types.ts Adds rectangle wall metadata.
src/floorplan-card.ts Renders generated room edges.
src/editor.ts Implements rectangle gestures, resizing, coupling, and toggles.
src/editor-geometry.ts Adds rectangle and shared-edge geometry utilities.
src/editor-geometry.test.ts Tests rectangle geometry behavior.
src/editor-drag.browser.test.ts Tests rectangle interactions and resizing.
Review details

Suppressed comments (4)

src/editor-drag.browser.test.ts:445

  • This test does not exercise the claimed non-shared resize: it selects the first edge (the room's horizontal top edge) and moves only clientX, but horizontal edge resizing consumes only the y coordinate. rectAreaEdgeResize therefore leaves the geometry unchanged and the coupling assertions are vacuous; select the vertical right edge or move the top edge in y.
    const from = center(edge!);
    pointer(edge!, "pointerdown", from.x, from.y);
    pointer(edge!, "pointermove", from.x + 10, from.y);
    pointer(edge!, "pointerup", from.x + 10, from.y);

src/editor.ts:4526

  • The generated wall's double-click handler toggles the owning area's autoWalls without checking area.locked. The separate toggle path is hidden for locked rooms and _startDrag rejects them, but double-clicking the wall body still changes a locked Area. Return early when the owning area is locked.
              @dblclick=${(e: PointerEvent) => {
                if (!autoWallInfo) return;
                e.preventDefault();
                e.stopPropagation();
                this._toggleRectAreaSide(autoWallInfo.area, autoWallIndex);

src/editor.ts:1876

  • This vertex-resize path also emits the normalized result without checking rectAreaHasMinimumSize. Dragging a rectangle corner onto its opposite corner can therefore persist a zero-width or zero-height Area, even though the new geometry helper defines that as illegal. Apply the same minimum-size policy here before emitting the coupled areas.
      const delta = { dx: target.x - moving.points[idx]!.x, dy: target.y - moving.points[idx]!.y };
      const coupled = this._coupleRectAreaSharedEdges(drag.primary.id, points, delta, f.areas ?? []);
      this._emitFloor({ areas: coupled.areas });

src/editor.ts:1496

  • Any non-zero pointer jitter arms the 200 ms rectangle timer because this condition checks only !==. With grid snapping, a long press that moves a few virtual units can then produce a zero-size snapped draft; on pointerup the code takes the drag branch and drops the polygon point instead of treating it as a click. Apply the same virtual drag slop used elsewhere (and account for the snapped target) before starting the timer.
      if (!this._areaDragMoved && this._areaDragTimer === null && (current.x !== start.x || current.y !== start.y)) {
  • Files reviewed: 6/6 changed files
  • Comments generated: 10
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/editor.ts
Comment thread src/editor.ts
Comment thread src/editor.ts Outdated
Comment thread src/editor.ts Outdated
Comment thread src/editor.ts Outdated
Comment thread src/editor.ts
Comment thread src/editor.ts
Comment thread src/editor.ts
Comment thread src/editor.ts Outdated
Comment thread src/floorplan-card.ts Outdated

@nicosandller nicosandller left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of #307 — rectangle rooms

Thanks for the rework — most of the last round is genuinely fixed. The merge conflict is gone, the README and docs/configuration.md now describe the feature, and renaming autoWalls → sideWalls with a "persisted like any other field" comment settles the contradiction I flagged. The cleanup landed too: RECT_AREA_EPSILON, WALL_THICKNESS instead of thickness: 8, a shared dividerStrokeStyle(), and the sideWallLookup map instead of the per-wall rescan.

But the commit that addressed Copilot's feedback introduced a regression, and it's a bad one.

Payment links

Checked first: unchanged. .github/FUNDING.yml is byte-identical to main, and the README diff touches no donation line.

Tests

Green everywhere: CI passes, and locally npm run typecheck is clean, 1,660 unit tests and 138 browser tests pass, and the build succeeds. That's the problem — see below.


Please fix (blocking)

1. Dragging an area corner does nothing at all.

Select any room and drag a corner handle: the shape doesn't move. This works on main and is broken on this branch, so it's a regression, not a missing feature. It also breaks the sentence this PR just added to the README ("or a corner handle to reshape it"), and it's the half of #288 / #291 that asks to "edit the room by dragging a corner".

I verified it with a browser test against both branches. On main a corner drag moves the vertex; on 29f1121 the points come back byte-identical for both a rectangle and a 5-point polygon.

Two separate causes, both in the vertex branch of _applyDrag (src/editor.ts:1884-1913):

  • The resized geometry is thrown away. points is computed from rectAreaVertexResize, then never emitted — the emit uses coupled.areas, whose primary is seeded from moving.points (the pre-drag shape) and only changes if a neighbour happens to couple. A room with no adjoining neighbour therefore can't be resized at all. The edge branch just below gets this right; the vertex branch should end the same way:

    this._emitFloor({
      areas: coupled.areas.map((a) => (a.id === drag.primary.id ? { ...a, points } : a)),
    });
  • The minimum-size guard rejects every polygon. rectAreaHasMinimumSize opens with if (!isRectArea(points)) return false, so for any non-rectangle area the guard is always true, and the drag takes the revert path that re-emits moving.points. Ordinary polygon rooms lose vertex editing entirely. Gate it:

    if (isRectArea(moving.points) && !rectAreaHasMinimumSize(points)) {

While you're in there: the coupling loop restarts from f.areas on each iteration (_coupleRectAreaSharedEdges(..., f.areas ?? [], side)), so for a corner touching two shared sides the second side's result discards the first's. Passing coupled.areas chains them.

I confirmed both fixes: with them applied, a lone rectangle's corner resizes as a rectangle, a polygon vertex moves again, and all 1,660 unit + 138 browser tests still pass.

2. Nothing covers the vertex handle. The reason the above shipped green is that no test drags a circle.handle on an area. rectAreaVertexResize is unit-tested in editor-geometry.test.ts and is fine — it's the wiring that broke, and all 16 new browser tests go through .area-edge-hit instead. Please add a browser test that drags a corner: one for a lone rectangle, one for a plain polygon. Both would have caught this.

3. A locked room's walls can still be toggled. Copilot raised this and it's still live. The edge handles are correctly hidden behind selected && !a.locked, but the generated wall body and its diamond in _renderWall (src/editor.ts:4588 and 4621) call _toggleRectAreaSide with no lock check. Double-clicking a locked room's wall cycles it wall → divider → none; I checked, and sideWalls.top goes from wall to divider on a locked: true area. Simplest fix is an early return in _toggleRectAreaSide when a.locked, which covers both call sites at once.

Small stuff (not blocking)

  • Still no PR description, and the title is the branch name. Given how much is in here — press-drag rectangles, corner/edge resize, shared-edge coupling, wall/divider toggles — a few lines saying what it does and closing #290 / #288 / #291 would help. Thanks for the screencast, that part was useful.
  • 0.001 is still spelled out 4 times in editor.ts (lines 1928, 4574, 4792, 4810) even though RECT_AREA_EPSILON now exists in editor-geometry.ts. Worth exporting and reusing.
  • The corner → sides mapping in the vertex branch is a nested four-deep ternary. RECT_AREA_SIDES is right there — something like [RECT_AREA_SIDES[(idx + 3) % 4], RECT_AREA_SIDES[idx]] reads better and can't drift from the side order.

Everything else looks good, and the shared-edge coupling behaviour is nicely tested. Fix the corner drag and the lock check and I'm happy to approve. Happy to push the two-line fix myself if you'd rather — say the word.

Generated by Claude Code

@TruthOf42 TruthOf42 changed the title Feat/rectangle rooms Enhanced Room Editing Sep 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/editor.ts
Comment thread src/editor.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues affect resizing safety, rendering correctness, sunlight behavior, caching, and interaction hit-testing.

Review effort: Lite
Findings: None

Resolved since last review (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Rejected resize leaves coupled neighbors displaced

src/​editor.ts:1936

When a rectangle corner resize would fall below the minimum size, this restores only the primary room but keeps the already-coupled neighbors from the preceding loop. That leaves shared edges torn apart even though the resize is rejected. On the invalid path, emit the original f.areas unchanged (or otherwise roll back all coupled updates).

Medium severity Divider side walls are missing from card rendering

src/​floorplan-card.ts:1005

Filtering out divider segments here also removes them from roomWallSegments, which is the array used by the SVG render below. As a result, a configured sideWalls: { top: divider } is never drawn in the card, despite dividerStrokeStyle being selected at render time. Keep divider segments in the render list and only exclude them from the blocking-wall list.

Medium severity Generated room walls do not block sunlight

src/​floorplan-card.ts:1007

The generated room walls are included in blockingWallSegments for dead-space and lamp-light calculations, but the sunlight renderer still receives only wallsThatBlock(active.walls) later in the card. Consequently a configured rectangle side wall blocks lamps/dead space but does not block sunlight, contradicting the documented wall semantics. Pass this same generated blocking-wall collection to the sunlight calculation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Five unresolved moderate issues affect resizing, coupling, divider rendering, and lighting behavior.

Review effort: Lite
Findings: None

@nicosandller
nicosandller merged commit 13e972d into main Sep 21, 2026
7 checks passed
@nicosandller
nicosandller deleted the feat/rectangle-rooms branch September 21, 2026 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants