Skip to content

feat: add imperial unit support for measurements in UI controls and t…#310

Closed
PRAVEEN7230 wants to merge 8 commits into
pascalorg:mainfrom
PRAVEEN7230:main
Closed

feat: add imperial unit support for measurements in UI controls and t…#310
PRAVEEN7230 wants to merge 8 commits into
pascalorg:mainfrom
PRAVEEN7230:main

Conversation

@PRAVEEN7230
Copy link
Copy Markdown

@PRAVEEN7230 PRAVEEN7230 commented May 15, 2026

What does this PR do?

This PR fixes the bug where toggling the unit only affected the display of dimensions and not the input itself, specifically in the object editing panels. It also adds keyboard-based dimensional input to the Wall tool during drafting, fully integrated into the new nodes registry refactor.

Fixes #308

Specific changes:

  • Centralized Measurement Parsing: Added a new measurements.ts utility to handle parsing and formatting between metric and imperial values (e.g., converting 10'6" to meters). Exported via packages/editor/src/index.tsx.
  • Unit Respect in SliderControl: Updated SliderControl (used in the Properties panels across all node kinds) to fetch the active unit state. It now properly formats values and parses imperial strings so that dimensions update correctly instead of silently reverting to metric.
  • Keyboard Dimension Input for Walls: Added direct drafting input to the new packages/nodes/src/wall/tool.tsx. While drawing a wall, users can now type dimensions (e.g., 10') and press Enter to snap the wall length in the current pointer direction exactly.
  • Refactor Alignment: Safely migrated these changes into the latest upstream structure without duplicating the wall-tool or breaking existing architectural boundaries.

How to test

  1. Verify Unit Toggle in Properties Panel: Select an existing wall and change your global unit to Imperial. Select the wall again, and try changing the length via the properties panel using a string like 15'2". Ensure the wall length updates accurately.
  2. Verify Wall Drafting Input: Select the Wall tool and click to start drawing. Before completing the wall, type 10' (a text input overlay should appear near your cursor). Press Enter and verify the wall is created at exactly 10' length in the direction of the cursor.
  3. Verify Escape/Cancel Flow: While typing a dimension in the Wall tool, press Escape to ensure it clears the text input without prematurely canceling the wall drawing itself.

Screenshots / screen recording

Screenshot 2026-05-15 at 11 09 34 PM Screenshot 2026-05-15 at 11 10 20 PM

Checklist

  • I've tested this locally with bun dev
  • My code follows the existing code style (run bun check to verify)
  • I've updated relevant documentation (if applicable)
  • This PR targets the main branch

boboidvtw pushed a commit to boboidvtw/editor that referenced this pull request May 17, 2026
…ss (pascalorg#284)

Backport from monorepo PR pascalorg#310 (fix/april-pass):

1. WebGPU renderer race on Canvas remount
   Cache the in-flight WebGPURenderer promise per canvas element so
   concurrent configure() calls from R3F's useLayoutEffect await the
   same init() instead of creating duplicate renderers. Fixes
   intermittent 'resolve target size does not match' errors when
   navigating between projects and home.

2. Duplicate item loses its scale
   Add scale to the field set in useDraftNode.commit() so duplicated
   items preserve their original scale instead of falling back to
   [1,1,1].

Co-authored-by: Pascal <open@pascal.app>
@Aymericr
Copy link
Copy Markdown
Contributor

Aymericr commented Jun 1, 2026

Thanks for putting in the work here, @PRAVEEN7230 — and for cross-referencing #308 to help us connect the dots.

After reviewing both PRs in depth, we're planning to go with #321's approach for the core panel-display bug. Here's the reasoning:

Architecture concern with SliderControl
The change to SliderControl that detects unit === "m" as a signal to auto-convert to imperial is the main issue. SliderControl is a generic presentation component used in ~100+ places across the codebase (roof, door, window, column, stair, fence panels, etc.). Wiring unit-awareness into it via unit === "m" as a proxy creates a hidden contract that every future caller of SliderControl has to be aware of — and it misfires on controls with negative ranges (like wall curve offset) where the negative imperial formatting is incorrect.

#321 vs #310 on the core bug
#321 keeps the conversion at the panel/call-site level: the wall panel reads meters from the scene, converts to feet for display, and converts back on commit. That's explicit and doesn't affect any other control.

Unrelated change
The .env.example deletion shouldn't be included — that file is helpful for new contributors and should be restored.

What's genuinely new in your PR: the keyboard dimension input 🎉
The wall-tool keyboard entry feature (type 10' → Enter → wall snaps to that length) is a distinct and valuable piece of work that isn't in #321. We'd love to see that land — just as a clean, standalone PR.

If you want to extract that feature into a new PR, here are a few things to address before it's ready:

  1. stopDrafting() needs to clear draftInputRef.current and call setDraftInput(null) — otherwise typed state leaks into the next wall segment
  2. parseMeasurement("10", "imperial") currently returns null (the raw-number branch is unreachable because the regex requires ' or " after digits) — a user typing just 10 won't get a wall at 10 feet
  3. formatMeasurement(value, 'metric') returns bare numbers like 3.00 without the m suffix, which regresses the metric HUD label display
  4. Would be great to add a few tests for parseMeasurement (the regex edge cases especially)

None of that is criticism of the idea — it's a great UX addition. Happy to review a follow-up PR for it!

Thanks again for contributing and for helping clarify the relationship between these two issues.

@Aymericr
Copy link
Copy Markdown
Contributor

Aymericr commented Jun 1, 2026

Follow-up adversarial pass (Codex 5.5 / GPT-5.5 / xhigh reasoning) with the full codebase — more specific findings:


🐛 Blocker: unit === "m" breaks signed meter controls in imperial mode

I ran the adversarial grep across the full codebase. Here are the concrete controls that break when isLengthMeasurement = unit === 'm' fires globally:

File Label min Breaks because
spawn/panel.tsx:99 X/Y/Z pos position ± 2 signed, formatted as imperial feet
door/panel.tsx:681 X wall -10 to +10 signed position
stair-segment/panel.tsx:238 X/Y/Z -50 to +50 signed position
reference-panel.tsx:298 X/Y/Z pos -50 to +50 signed position
slab/panel.tsx:161 Height (elevation) -1 to +1 signed elevation
fence/inspector-editors.tsx:75 Curve -maxCurveOffset signed curve offset

For example: -0.1524m = -0.5ft. PR #310's formatMeasurement returns -1'6" for this. Typing -6" returns null from parseMeasurement. Every signed unit="m" control becomes unusable in imperial mode.


🐛 Blocker: parseMeasurement("10", "imperial") returns null

Verified by running the parser directly:

parseMeasurement("10", "imperial")      => null  ← raw number branch unreachable
parseMeasurement("5.5", "imperial")     => null  ← same
parseMeasurement("10'", "imperial")     => 3.048 ✓
parseMeasurement("10' 6\"", "imperial") => 3.200 ✓
parseMeasurement("-6\"", "imperial")    => null  ← negative rejected

The regex /^\s*(?:(\d+(?:\.\d+)?)\s*')?\s*(?:(\d+(?:\.\d+)?)\s*")?\s*$/ requires at least one ' or " to match — a bare number like "10" doesn't satisfy it so match is null. The raw-number fallback inside the if (match) block is dead code.

Fix: check for a bare decimal number before the feet/inches regex.


🐛 Blocker: typed input leaks into the next wall segment after mouse-click commit

Repro:

  1. Start drawing a wall (click to place first point)
  2. Type 10' — draft overlay shows 10'
  3. Click to place the wall (don't press Enter)
  4. Wall commits, new segment starts — but draftInputRef.current and draftInput still contain 10'
  5. New segment shows stale 10' as label; further keystrokes append to it

Root cause: the mouse pointerup commit path in wall/tool.tsx:454 doesn't clear draftInputRef.current / setDraftInput(null). Also, stopDrafting() itself doesn't clear it, so any future cancel path can leave state around.

Fix: add draftInputRef.current = null; setDraftInput(null) to both stopDrafting() and the pointer commit path.


All other points from the previous comment stand. Looking forward to the standalone keyboard-input PR with these fixed!

@Aymericr
Copy link
Copy Markdown
Contributor

Aymericr commented Jun 1, 2026

Hey @PRAVEEN7230 — thanks for the work on this, and for flagging the duplicate on #322. You spotted a real overlap.

After reviewing both PRs in depth, we're going to land #321 first. Its approach is a better fit for the codebase right now: a single shared measurements.ts that's reused across wall labels, site edge labels, item placement guides, floorplan panel, and the wall panel — versus patching individual components. Less surface area for future drift.

That said, your PR has something #321 doesn't: the wall-tool keyboard dimension input — the ability to type 10' while drafting and have the wall snap to that exact length. That's a genuinely useful UX addition and we'd like to keep it.

Would you be open to extracting just that keyboard input piece into a new, focused PR targeting main after #321 lands? It would be a much smaller diff and easier to review in isolation. If you'd rather not, no worries at all — just let us know and we can take it from here with full credit to you.

Either way, thank you for the contribution and for doing the detective work on the duplicate.

@Aymericr
Copy link
Copy Markdown
Contributor

Aymericr commented Jun 1, 2026

Closing in favor of #321, which takes a cleaner panel-level approach to imperial unit display.

The core panel-display bug is covered by #321 — it adds a shared measurements.ts utility and converts at each explicit call site rather than wiring unit-awareness into the generic SliderControl component.

What's unique to this PR worth preserving: the keyboard dimension input during wall drafting (type 10' → Enter → snap to length) is a genuinely useful UX feature not in #321. If you'd like to land that feature, a standalone PR targeting just wall/tool.tsx would be very welcome — with the fixes noted in the review:

  1. Clear draftInputRef.current / setDraftInput(null) from both stopDrafting() and the pointer commit path (typed state leaks into the next segment on click-commit)
  2. Fix the raw-number branch in parseMeasurement"10" currently returns null (the regex requires a ' or " suffix)
  3. Restore .env.example (it was deleted but is still tracked and useful for new contributors)

Thanks for contributing, @PRAVEEN7230!

@Aymericr Aymericr closed this Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unit toggle only affects the display of dimensions, not the input

2 participants