Skip to content

feat(core): add ellipse ring and path repeat - #118

Merged
madawei2699 merged 2 commits into
mainfrom
codex/issue-117-ellipse-path-repeat
Sep 27, 2026
Merged

madawei2699 merged 2 commits into
mainfrom
codex/issue-117-ellipse-path-repeat

Conversation

@madawei2699

Copy link
Copy Markdown
Contributor

Summary

  • Add additive EllipseRing support for deterministic true-ellipse voxel shells, partial arcs, bounds, budget estimation, and ordinary provenance/support behavior.
  • Add ellipse-only PathRepeat for reusable assemblies with stable closed/open sampling, optional analytic tangent orientation, conservative rotated-envelope budgets and bounds, and source-aware support diagnostics.
  • Document the new ComponentPlan schema and authoring guidance.

Fixtures and validation

  • Add a true ellipse shell fixture, 24-bay two-level tangent arcade, nine-sample open half-ellipse endpoint fixture, circular regression coverage, and bounds/budget checks.
  • MinePilot local visual review: ellipse shell is visibly oval in top view; the 24-bay arcade follows the oval tangent with its seam and bay gaps visible in top/front/right views. No public catalog content was created.
  • pnpm test: passed; build passed; 207 tests passed across core (188), importer-schem (3), exporter-schem (6), and CLI (10).
  • pnpm typecheck: passed.
  • pnpm lint: passed.
  • git diff --check: passed.

Closes #117

Copy link
Copy Markdown
Contributor Author

Implementation direction looks good and CI is green, but I found one stable-API blocker before merge:

PathRepeat has no vertical placement / Y offset.

The new placement carries only an XZ ellipse path and expands the source assembly at local Y=0. Because PathRepeat references an assembly directly rather than an anchored Instance, authors cannot cleanly reuse the same arcade/module at an arbitrary global Y without baking that Y into every assembly member (which inflates assembly bounds/budgets) or moving the whole construct into a section.

Please add an explicit vertical placement semantic before this becomes stable schema—e.g. placement.y as a non-negative integer, applied as y * unit to every sample shift—with bounds/budget handling, docs, and a regression proving the same assembly can be reused at non-zero Y without modifying the assembly definition.

While touching the API, please also review closed-path phase semantics. Today a full closed ellipse effectively has to be expressed as 0→360, so authors cannot phase-shift the sample pattern (for example, start the 24 bays at 7.5°) without changing the path geometry. This is secondary to the missing Y offset, but now is the cheapest point to make the v1 sampling contract intentional.

No objection to the bounded EllipseRing + ellipse-only PathRepeat design itself; keep CircleRing/RadialRepeat behavior unchanged.

Copy link
Copy Markdown
Contributor Author

Follow-up review of head 9101f1f: the previous PathRepeat API blocker is resolved.

Verified from the actual diff/head:

  • placement.y is a required non-negative integer;
  • expansion applies y * unitBlocks as the global vertical shift while keeping assembly-local Y unchanged;
  • emitted voxels and the declared rotated assembly envelope are both checked after the vertical shift;
  • closed loops now support phase via equal start/end angles while preserving legacy 0→360;
  • the new tests cover multi-level reuse of the same assembly, phased closed loops, shifted envelope/voxel bounds, and existing circular regressions;
  • CI run 36306268368 is green with 210 tests reported locally.

I do not see a remaining blocker in #118. Keep it unmerged until the normal human merge decision; after #116 lands first, refresh/rebase #118 only if main moved in a way that requires it.

@madawei2699
madawei2699 marked this pull request as ready for review September 27, 2026 08:49
@madawei2699
madawei2699 merged commit bda45ae into main Sep 27, 2026
1 check passed
@madawei2699
madawei2699 deleted the codex/issue-117-ellipse-path-repeat branch September 27, 2026 08:49
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.

Implement bounded EllipseRing and ellipse PathRepeat

2 participants