Skip to content

fix(layout): respect manualEdits placements during auto-pack#2280

Open
gsdali wants to merge 1 commit intotscircuit:mainfrom
gsdali:fix-manual-edits-respect-by-packer
Open

fix(layout): respect manualEdits placements during auto-pack#2280
gsdali wants to merge 1 commit intotscircuit:mainfrom
gsdali:fix-manual-edits-respect-by-packer

Conversation

@gsdali
Copy link
Copy Markdown

@gsdali gsdali commented May 9, 2026

Summary

The board's manualEdits.pcb_placements populates a component's
position via _computePcbGlobalTransformBeforeLayout, but the
auto-packer in each group then re-positions the same component,
silently undoing the user's pin. From the user's perspective, dragging
a component in the IDE writes to manual-edits.json but the rebuilt
board ignores almost everything they did.

The cause is in Group_doInitialPcbLayoutPack: it marks a child as
"static" (skip-repack) only if isRelativelyPositioned() returns true,
and that helper only checks pcbX / pcbY / pcbLeftEdgeX / etc. It
doesn't consult manualEdits, so any component placed via the IDE
drag-and-pin flow that has no explicit prop coordinates gets repacked
along with the auto-placed siblings.

Repro

tests/pcb-packing/manual-edits-respected-by-packer.test.tsx mirrors
the failing case in a downstream board: a chip plus 4 caps inside an
anchored region, with manualEdits pinning each cap to a different
corner.

<board width="20mm" height="10mm" manualEdits={{
  pcb_placements: [
    { selector: "C1", center: { x:  5, y:  3 }, relative_to: "group_center" },
    { selector: "C2", center: { x: -5, y:  3 }, relative_to: "group_center" },
    { selector: "C3", center: { x:  5, y: -3 }, relative_to: "group_center" },
    { selector: "C4", center: { x: -5, y: -3 }, relative_to: "group_center" },
  ],
}}>
  <group name="region" pcbX={0} pcbY={0}>
    <chip name="U1" footprint="soic8" .../>
    <capacitor name="C1" .../> {/* and C2-C4, all auto-placed */}
  </group>
</board>

Pre-fix on main: every cap collapses to (0, 0) — the chip's
network-minimum point — and the test assertion c1.center.x ≈ 5 fails
with Received: 0.

Post-fix: all four caps land at the requested coordinates and the test
passes.

Fix

Walk every descendant of the group being packed, ask the nearest
subcircuit if it resolves a manual placement for that descendant, and
add the pcb_component_id to staticPcbComponentIds if so. The packer
then leaves it where the manual placement put it.

const collectManuallyPlacedDescendants = (comp: any) => {
  const subcircuit = comp?.getSubcircuit?.()
  const manualPlacement =
    subcircuit?._getPcbManualPlacementForComponent?.(comp)
  if (manualPlacement && comp?.pcb_component_id) {
    staticPcbComponentIds.add(comp.pcb_component_id)
  }
  if (comp?.children) comp.children.forEach(collectManuallyPlacedDescendants)
}
collectManuallyPlacedDescendants(group)

Two-line change in
lib/components/primitive-components/Group/Group_doInitialPcbLayoutPack/Group_doInitialPcbLayoutPack.ts,
plus a focused recursion helper. No effect on components that already
have pcbX/pcbY (those are already marked static via the existing
isRelativelyPositioned() path).

Related (out of scope here)

There is a separate, complementary bug in
_computePcbGlobalTransformBeforeLayout where manualPlacement (which
is already in subcircuit-absolute coordinates) is composed with the
parent group's transform, double-counting the group anchor for any
component inside an anchored region. A non-zero-anchored group like
<group pcbX={16} pcbY={0}> containing a manually-placed cap at
(11.4, 4.3) lands the cap at (27.4, 4.3) instead of (11.4, 4.3). Filed
separately as issue #TBD — that fix is needed for the IDE drag flow to
produce visually-correct positions, but is independent of this PR.
This PR makes the static-marking honest; the position-interpretation
fix lets it land where the user dragged.

Test plan

  • New repro test fails on main (cap at x=0), passes with fix
    (cap at x=5)
  • tests/pcb-packing/ (2 tests) — pass
  • tests/components/pcb tests/groups tests/features/pcb-pack-layout
    (51 tests) — pass
  • CI green
  • Reviewer: confirm the recursion guard is appropriate (the helper
    walks every child, which is consistent with collectMargins a
    few lines above; alternative would be to scan db.toArray() once
    for pcb_component rows, but that's the same big-O for typical
    boards and reads worse)

🤖 Generated with Claude Code

The board's manualEdits.pcb_placements positions a component via
_computePcbGlobalTransformBeforeLayout, but the auto-packer in each
group then re-positioned the same component, silently undoing the
user's pin.

Group_doInitialPcbLayoutPack marked a child as static (skip-repack)
only if isRelativelyPositioned() returned true — and that helper
checks pcbX/pcbY/pcbLeftEdgeX/etc., not manualEdits. Components placed
via the IDE drag-and-pin flow weren't covered.

Walk every descendant of the group being packed and ask the nearest
subcircuit for a manual placement. If one exists, add the
pcb_component_id to staticPcbComponentIds — the packer then leaves it
where the manual edit pinned it.

Repro test in tests/pcb-packing/ — chip + 4 caps inside an anchored
region, manualEdits pinning each cap to a different corner. Without
the fix every cap collapses to the chip's network-minimum (x=0); with
the fix they land at the requested coordinates.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
tscircuit-core-benchmarks Ready Ready Preview, Comment May 9, 2026 6:45am

Request Review

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.

1 participant