Skip to content

fix(layout): manualEdits position no longer double-counts parent group anchor#2282

Open
gsdali wants to merge 2 commits intotscircuit:mainfrom
gsdali:fix-manual-edits-position-double-counting
Open

fix(layout): manualEdits position no longer double-counts parent group anchor#2282
gsdali wants to merge 2 commits intotscircuit:mainfrom
gsdali:fix-manual-edits-position-double-counting

Conversation

@gsdali
Copy link
Copy Markdown

@gsdali gsdali commented May 9, 2026

Summary

Closes #2281.

A component placed via manualEdits.pcb_placements inside a positioned <group pcbX={N}> landed at (N + manual.x, manual.y) instead of (manual.x, manual.y) — the group's anchor was added on top of the absolute manual position. From the user's perspective, every IDE drag inside a regional group ended up shifted by that group's anchor on the next rebuild.

Both _computePcbGlobalTransformBeforeLayout (initial render) and applyPackOutput (post-pack write-back) had the same shape:

compose(parent_or_group_transform, translate(manualPlacement), ...)

manualPlacement is the result of applyToPoint(subcircuit.globalTransform, position.center). For the top-level board (transform = identity) it equals position.center, which the IDE writes in board-absolute coordinates. Composing the parent group's transform on top of that translates by the group anchor a second time.

Repro

tests/pcb-packing/manual-edits-position-no-double-count.test.tsx:

const manualEdits = {
  pcb_placements: [
    { selector: "C1", center: { x: 11.4, y: 4.3 }, relative_to: "group_center" },
  ],
}

<board width="40mm" height="20mm" manualEdits={manualEdits}>
  <group name="region" pcbX={16} pcbY={0}>
    <chip name="U1" footprint="soic8" pinLabels={{ pin1: "VCC", pin8: "GND" }} />
    <capacitor name="C1" capacitance="100nF" footprint="0402"
      connections={{ pin1: "U1.VCC", pin2: "U1.GND" }} />
  </group>
</board>

Pre-fix: c1.center = (27.4, 4.3) (the +16 region anchor was added on top of the absolute manual position).

Post-fix: c1.center = (11.4, 4.3) (lands exactly where the user dragged it).

A second test asserts a group at pcbX={0} pcbY={0} continues to work — no regression for the existing-correct case.

Fix

Two edits, same root cause:

1. _computePcbGlobalTransformBeforeLayout (PrimitiveComponent.ts):
when manualPlacement is set, return JUST the manual translate + rotate, without composing the parent's transform.

if (manualPlacement && this.props.pcbX === undefined && this.props.pcbY === undefined) {
  // `manualPlacement` is already in subcircuit-global coordinates;
  // composing parent transform on top double-counts any positioned
  // ancestor group.
  const rotation = this._getPcbRotationBeforeLayout() ?? 0
  return compose(
    translate(manualPlacement.x, manualPlacement.y),
    rotate((rotation * Math.PI) / 180),
  )
}

2. applyPackOutput (Group_doInitialPcbLayoutPack/):
when the component being written back has a matching entry in the subcircuit's manualEdits.pcb_placements, skip the pack transform — the component's child pcb primitives are already at the absolute manual position from initial render. position_mode is still flipped to "packed" so downstream code sees a consistent state.

const manualEdits = group.getSubcircuit?.()?._parsedProps?.manualEdits
if (manualEdits?.pcb_placements && sourceComponent?.name) {
  const hit = manualEdits.pcb_placements.find(
    (p) => p.selector === sourceComponent.name,
  )
  if (hit) {
    db.pcb_component.update(componentId, { position_mode: "packed" })
    continue
  }
}

Stacked on #2280

This PR is stacked on #2280 (packer respects manualEdits as static). Without that fix, the packer overrides the manual position with its own (different, also wrong) value, masking this bug. Together both fixes make the IDE drag-and-pin flow behave correctly end-to-end.

Test plan

  • Two new repro tests in tests/pcb-packing/manual-edits-position-no-double-count.test.tsx (positioned group + zero-anchored group)
  • Static-marking repro from fix(layout): respect manualEdits placements during auto-pack #2280 still passes
  • Wider suites: tests/pcb-packing (3 tests), tests/components/pcb (47), tests/groups (2), tests/features/pcb-pack-layout (1) — all pass when run individually; two unrelated timing-sensitive tests in the wider parallel run flaked twice but green when re-run alone
  • CI green
  • Reviewer: confirm the schematic equivalent (_getSchematicGlobalManualPlacementTransform) at line ~890 has the same shape and may want a parallel fix in a follow-up PR

🤖 Generated with Claude Code

gsdali and others added 2 commits May 9, 2026 18:46
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>
…p anchor

A component placed via `manualEdits.pcb_placements` inside a positioned
`<group pcbX={N}>` landed at `(N + manual.x, manual.y)` instead of
`(manual.x, manual.y)` — the group's anchor was added on top of the
absolute manual position. Both `_computePcbGlobalTransformBeforeLayout`
(initial render) and `applyPackOutput` (post-pack write-back) had the
same shape:

  compose(parent_or_group_transform, translate(manualPlacement), ...)

`manualPlacement` is the result of
`applyToPoint(subcircuit.globalTransform, position.center)` — for the
top-level board (transform = identity) it equals `position.center`,
which the IDE writes in board-absolute coordinates. Composing the
parent group's transform on top of that translates by the group anchor
a second time.

Fix in two places:

1. `_computePcbGlobalTransformBeforeLayout` (PrimitiveComponent.ts):
   when manualPlacement is set, return JUST the manual translate +
   rotate, without composing the parent's transform.

2. `applyPackOutput`: when the component being written back has a
   matching entry in the subcircuit's manualEdits.pcb_placements,
   skip the pack transform entirely — its child pcb primitives are
   already at the absolute manual position from initial render. The
   `position_mode` is still flipped to "packed" so downstream code
   sees a consistent state.

Companion to fix tscircuit#2280 (packer respects manualEdits as static). Both
together make the IDE drag-and-pin flow behave correctly for
components inside positioned regions.

Repro test: `tests/pcb-packing/manual-edits-position-no-double-count.test.tsx`
- Cap manually placed at (11.4, 4.3) inside `<group pcbX={16}>`.
- Pre-fix: cap lands at (27.4, 4.3). Post-fix: (11.4, 4.3).
- Second test asserts no regression for a group at (0, 0).

Both new tests + the static-marking repro from tscircuit#2280 pass. Wider PCB /
group / packing suites run clean (53 pass, 2 pre-existing timing-flaky
tests pass when run in isolation).

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 9:00am

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.

manualEdits position double-counts the parent group's anchor for components inside positioned regions

1 participant