feat(bin-designer): add corrugated wall pattern#1291
Closed
Conversation
Add a sinusoidal corrugated wall pattern as a new option alongside honeycomb. Unlike honeycomb (which cuts holes), corrugated replaces flat walls with uniform-thickness wave profiles folding inward from the grid boundary for added structural strength. - New discriminated union registry (cut vs replace pattern modes) - New wallReplaceTargets pipeline concept (cut flat + fuse corrugated) - Phase-aligned wavelength ensures wave crests at wall corners - Size-adaptive parameters: amplitude = thickness × 0.4, wavelength scales with bin height - Minimum 1.6mm wall thickness enforced in UI (per-option disable) - Clear zones around cutouts/handles/ramps match honeycomb behavior - i18n: all 7 locales updated
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
area: i18n
tests
feature: generation
feature: bin-designer
labels
There was a problem hiding this comment.
Pull request overview
Adds a new corrugated wall pattern to the bin designer and generation pipeline, introducing a new “wall replacement” boolean pass (cut flat wall slab → fuse corrugated wall solid) alongside the existing cut-through pattern approach.
Changes:
- Introduces corrugated pattern math + geometry builders and routes pattern application through a new
wallReplaceTargetspipeline concept. - Extends the pattern registry to a discriminated union (
cutvsreplace) and updates the pipeline stages accordingly. - Updates the Walls UI to include corrugated selection, per-option disabling under 1.6mm wall thickness, and updates i18n strings across locales.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/i18n/locales/pt-BR.json | Adds corrugated pattern labels and thin-wall warning. |
| src/i18n/locales/nl.json | Adds corrugated pattern labels and thin-wall warning. |
| src/i18n/locales/nb.json | Adds corrugated pattern labels and thin-wall warning. |
| src/i18n/locales/fr.json | Adds corrugated pattern labels and thin-wall warning. |
| src/i18n/locales/es.json | Adds corrugated pattern labels and thin-wall warning. |
| src/i18n/locales/en.ts | Adds corrugated pattern labels and thin-wall warning (TS locale). |
| src/i18n/locales/en.json | Adds corrugated pattern labels and thin-wall warning (JSON locale). |
| src/i18n/locales/de.json | Adds corrugated pattern labels and thin-wall warning. |
| src/features/generation/worker/generators/pipeline/types.ts | Extends pipeline context with wallReplaceTargets. |
| src/features/generation/worker/generators/pipeline/stages/featuresStage.ts | Branches wall pattern building by registry mode (cut vs replace). |
| src/features/generation/worker/generators/pipeline/stages/booleanStage.ts | Applies the new wall replacement boolean pass and cleans up shapes. |
| src/features/generation/worker/generators/pipeline/context.ts | Initializes wallReplaceTargets in the initial context. |
| src/features/generation/worker/generators/patterns/registry.ts | Converts registry entries to a discriminated union and adds corrugated entry. |
| src/features/generation/worker/generators/patterns/index.ts | Re-exports corrugated helpers and adds getPatternMode. |
| src/features/generation/worker/generators/patterns/corrugatedPattern.ts | Implements corrugated spec computation and profile point generation. |
| src/features/generation/worker/generators/patterns/corrugatedPattern.test.ts | Adds unit tests for corrugated spec/point generation. |
| src/features/generation/worker/generators/corrugatedWallBuilder.ts | Builds corrugated wall solids + cut slabs with clear-zone clipping and caching. |
| src/features/generation/worker/generators/corrugatedWallBuilder.test.ts | Adds basic integration/spec tests for corrugated wall spec behavior. |
| src/features/bin-designer/types/index.ts | Extends WallPatternType to include corrugated. |
| src/features/bin-designer/components/panel/WallsSection/useWallsSection.ts | Adds per-pattern disable logic and auto-disables corrugated when too thin. |
| src/features/bin-designer/components/panel/WallsSection/WallsSection.tsx | Passes disabledPatterns into the pattern selector. |
| src/features/bin-designer/components/panel/WallsSection/PatternSelector.tsx | Adds corrugated option + icon and supports individually disabled options. |
| src/features/bin-designer/components/panel/WallsSection/PatternSelector.test.tsx | Updates tests for the new option and disabled-pattern behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+118
to
+135
| try { | ||
| const prev = bin; | ||
| bin = unwrap(cut(bin as ValidSolid, cutShape as ValidSolid)); | ||
| if (prev !== originalSolid) prev.delete(); | ||
| } catch (e: unknown) { | ||
| if (isAbortError(e)) throw e; | ||
| // If cut fails, skip this wall replacement | ||
| fuseShape.delete(); | ||
| cutShape.delete(); | ||
| continue; | ||
| } | ||
| try { | ||
| const prev = bin; | ||
| bin = unwrap(fuse(bin as ValidSolid, fuseShape as ValidSolid)); | ||
| if (prev !== originalSolid) prev.delete(); | ||
| } catch (e: unknown) { | ||
| if (isAbortError(e)) throw e; | ||
| // If fuse fails, keep the cut result (flat wall is removed, no corrugated added) |
There was a problem hiding this comment.
In the wall replacement loop, if the cut succeeds but the subsequent fuse fails, the code intentionally keeps the cut result, which can leave a missing wall section (a hole) in the final solid. To avoid producing invalid/unsafe geometry, keep the pre-cut solid unless both operations succeed (e.g., only delete/discard prev after a successful fuse, or roll back to prev on fuse failure).
Suggested change
| try { | |
| const prev = bin; | |
| bin = unwrap(cut(bin as ValidSolid, cutShape as ValidSolid)); | |
| if (prev !== originalSolid) prev.delete(); | |
| } catch (e: unknown) { | |
| if (isAbortError(e)) throw e; | |
| // If cut fails, skip this wall replacement | |
| fuseShape.delete(); | |
| cutShape.delete(); | |
| continue; | |
| } | |
| try { | |
| const prev = bin; | |
| bin = unwrap(fuse(bin as ValidSolid, fuseShape as ValidSolid)); | |
| if (prev !== originalSolid) prev.delete(); | |
| } catch (e: unknown) { | |
| if (isAbortError(e)) throw e; | |
| // If fuse fails, keep the cut result (flat wall is removed, no corrugated added) | |
| let cutResult: Shape3D | null = null; | |
| try { | |
| // Perform cut into a temporary solid; do not update bin yet. | |
| cutResult = unwrap(cut(bin as ValidSolid, cutShape as ValidSolid)); | |
| } catch (e: unknown) { | |
| if (isAbortError(e)) throw e; | |
| // If cut fails, skip this wall replacement entirely. | |
| // Eagerly dispose these shapes; final cleanup is defensive. | |
| try { | |
| fuseShape.delete(); | |
| } catch { | |
| /* already disposed */ | |
| } | |
| try { | |
| cutShape.delete(); | |
| } catch { | |
| /* already disposed */ | |
| } | |
| continue; | |
| } | |
| try { | |
| // Fuse the replacement wall into the cut result. | |
| const fused = unwrap(fuse(cutResult as ValidSolid, fuseShape as ValidSolid)); | |
| // Both cut and fuse succeeded: now atomically update bin. | |
| if (bin !== originalSolid) { | |
| try { | |
| bin.delete(); | |
| } catch { | |
| /* already disposed */ | |
| } | |
| } | |
| try { | |
| cutResult.delete(); | |
| } catch { | |
| /* already disposed */ | |
| } | |
| bin = fused; | |
| cutResult = null; | |
| } catch (e: unknown) { | |
| if (isAbortError(e)) throw e; | |
| // If fuse fails, discard the cut result and keep the original bin | |
| if (cutResult) { | |
| try { | |
| cutResult.delete(); | |
| } catch { | |
| /* already disposed */ | |
| } | |
| cutResult = null; | |
| } |
Comment on lines
+66
to
+92
| * Outer face is at Y=0 (flat line), inner face follows the sine wave. | ||
| */ | ||
| function buildCorrugatedProfile(spec: CorrugatedWallSpec): Drawing { | ||
| const innerPoints = generateInnerFacePoints(spec); | ||
| const halfSpan = spec.wallSpan / 2; | ||
|
|
||
| // In the bin's coordinate system, walls extend outward (negative Y from | ||
| // the inner face for the front wall). The profile Y axis is negated so | ||
| // the geometry occupies the wall region, not the bin cavity. | ||
| // | ||
| // Y=0 = inner face (wall anchor point) | ||
| // Y=-wallThickness = outer face (flat, at grid boundary) | ||
| // Y=-wallThickness-amplitude = deepest corrugation point | ||
|
|
||
| // Start at bottom-left of inner face | ||
| let pen = draw([-halfSpan, 0]); | ||
|
|
||
| // Inner face: left to right (flat line at Y=0, the inner face boundary) | ||
| pen = pen.lineTo([halfSpan, 0]); | ||
|
|
||
| // Right edge: step outward to outer sinusoidal face | ||
| const lastInner = innerPoints[innerPoints.length - 1]; | ||
| pen = pen.lineTo([lastInner[0], -lastInner[1]]); | ||
|
|
||
| // Outer face: right to left (sinusoidal wave, negated Y) | ||
| for (let i = innerPoints.length - 2; i >= 0; i--) { | ||
| pen = pen.lineTo([innerPoints[i][0], -innerPoints[i][1]]); |
There was a problem hiding this comment.
buildCorrugatedProfile/its comments describe the outer face as flat at the grid boundary, but the polygon being constructed makes the Y=0 edge flat and the other edge follow generateInnerFacePoints (negated). Since generateInnerFacePoints is defined as the inner face depth from a flat outer face, this effectively makes the outer wall boundary wavy and can change the bin’s external dimensions. Please realign the profile so the outer face remains fixed (matching the existing shell) and the corrugation folds inward.
Suggested change
| * Outer face is at Y=0 (flat line), inner face follows the sine wave. | |
| */ | |
| function buildCorrugatedProfile(spec: CorrugatedWallSpec): Drawing { | |
| const innerPoints = generateInnerFacePoints(spec); | |
| const halfSpan = spec.wallSpan / 2; | |
| // In the bin's coordinate system, walls extend outward (negative Y from | |
| // the inner face for the front wall). The profile Y axis is negated so | |
| // the geometry occupies the wall region, not the bin cavity. | |
| // | |
| // Y=0 = inner face (wall anchor point) | |
| // Y=-wallThickness = outer face (flat, at grid boundary) | |
| // Y=-wallThickness-amplitude = deepest corrugation point | |
| // Start at bottom-left of inner face | |
| let pen = draw([-halfSpan, 0]); | |
| // Inner face: left to right (flat line at Y=0, the inner face boundary) | |
| pen = pen.lineTo([halfSpan, 0]); | |
| // Right edge: step outward to outer sinusoidal face | |
| const lastInner = innerPoints[innerPoints.length - 1]; | |
| pen = pen.lineTo([lastInner[0], -lastInner[1]]); | |
| // Outer face: right to left (sinusoidal wave, negated Y) | |
| for (let i = innerPoints.length - 2; i >= 0; i--) { | |
| pen = pen.lineTo([innerPoints[i][0], -innerPoints[i][1]]); | |
| * Outer face is at Y=0 (flat line at the existing shell boundary), | |
| * inner face follows the sine wave inward (positive depth). | |
| */ | |
| function buildCorrugatedProfile(spec: CorrugatedWallSpec): Drawing { | |
| const innerPoints = generateInnerFacePoints(spec); | |
| const halfSpan = spec.wallSpan / 2; | |
| // Y=0 = outer face (flat, at grid boundary) | |
| // Y>0 = into wall thickness (towards bin interior), following corrugation | |
| // Start at left of outer face | |
| let pen = draw([-halfSpan, 0]); | |
| // Outer face: left to right (flat line at Y=0) | |
| pen = pen.lineTo([halfSpan, 0]); | |
| // Right edge: step inward to inner sinusoidal face | |
| const lastInner = innerPoints[innerPoints.length - 1]; | |
| pen = pen.lineTo([lastInner[0], lastInner[1]]); | |
| // Inner face: right to left (sinusoidal wave, depth from outer face) | |
| for (let i = innerPoints.length - 2; i >= 0; i--) { | |
| pen = pen.lineTo([innerPoints[i][0], innerPoints[i][1]]); |
Comment on lines
+452
to
+488
| // Cache key | ||
| const wallKey = compactKey( | ||
| buildCacheKey( | ||
| 'v1', | ||
| 'corrugated', | ||
| quantize(spec.amplitude), | ||
| quantize(spec.wavelength), | ||
| quantize(spec.patternH), | ||
| quantize(spec.bottomZ), | ||
| quantize(spec.wallSpan), | ||
| quantize(spec.wallThickness), | ||
| spec.waveCount, | ||
| quantize(wallPos.translateX), | ||
| quantize(wallPos.translateY), | ||
| wallPos.zRotation ?? 0, | ||
| // Include cutout/handle config in cache key for clear zones | ||
| params.walls.enabled && params.walls[wallPos.side]?.enabled | ||
| ? buildCacheKey( | ||
| 'clip', | ||
| params.walls.shape, | ||
| quantize(params.walls[wallPos.side].width), | ||
| quantize(params.walls[wallPos.side].depth), | ||
| params.walls[wallPos.side].alignment, | ||
| quantize(params.walls[wallPos.side].offset) | ||
| ) | ||
| : 'noclip', | ||
| params.handles.enabled && params.handles[wallPos.side]?.enabled | ||
| ? buildCacheKey( | ||
| 'hdl', | ||
| params.handles.shape, | ||
| params.handles.count, | ||
| quantize(params.handles[wallPos.side].width ?? params.handles.width), | ||
| quantize(params.handles[wallPos.side].height ?? params.handles.height), | ||
| quantize(params.handles.verticalPosition ?? 0) | ||
| ) | ||
| : 'nohdl' | ||
| ) |
There was a problem hiding this comment.
The cache key for corrugated walls doesn’t include several inputs that affect the final clipped geometry (e.g., ramp zones from computeRampZones, hasLip/lip overhang, compartment thickness used in clipExtrudeDepth, widthMm vs % width for wall cutouts, and the label.enabled condition that changes handle clipping on the back wall). This can cause stale cached shapes to be reused with incorrect clear zones or insufficient clip depth. Consider mirroring wallPatternBuilder’s approach: compute the effective clip params (including ramp zones + handle segments) and incorporate those derived values into the cache key (and bump the cache version).
Comment on lines
+90
to
+99
| // Phase-align: snap to integer number of complete waves | ||
| const waveCount = Math.max(1, Math.round(wallSpan / baseWavelength)); | ||
| const wavelength = wallSpan / waveCount; | ||
|
|
||
| // Must fit at least one half-wave vertically | ||
| if (patternH < wavelength / 2) return null; | ||
|
|
||
| // Wall span must be meaningful (need at least one full wave) | ||
| if (wallSpan < baseWavelength / 2) return null; | ||
|
|
There was a problem hiding this comment.
createCorrugatedSpec rejects cases where patternH < wavelength / 2, but wavelength is computed from the wall span (horizontal corrugation), while patternH is the vertical extrusion height used by the builder. Since the corrugation is extruded uniformly in Z (no vertical wave), this mixes unrelated dimensions and can disable corrugation for short bins based solely on wall span. Either remove/replace this constraint with a height-based minimum that matches the actual geometry, or update the implementation if the intent is a vertical waveform.
Comment on lines
+9
to
+10
| /** Minimum wall thickness for corrugated pattern (mm). Must match corrugatedPattern.ts. */ | ||
| const CORRUGATED_MIN_WALL_THICKNESS = 1.6; |
There was a problem hiding this comment.
The corrugated min wall thickness is duplicated here as a local constant with a “must match” comment, while the generation layer already exports CORRUGATED_MIN_WALL_THICKNESS. To prevent drift between UI gating and generator behavior, consider defining this threshold in a shared/constants module (importable by both UI and worker) and referencing it from both places instead of duplicating the numeric literal.
Greptile SummaryThis PR adds a corrugated (sinusoidal) wall pattern to the bin designer, complementing the existing honeycomb option. The feature introduces a new Key implementation highlights:
Two minor style issues found:
Confidence Score: 5/5Safe to merge — all findings are P2 style suggestions with no impact on runtime correctness or geometry output. The implementation is thorough and well-tested (14 unit tests + 5 integration tests). The math, memory management, pipeline integration, and UI constraint enforcement are all correct. Only two minor documentation/style issues were found, neither of which affects current behavior. No files require special attention; the two minor suggestions are in PatternSelector.tsx (hint message guard) and corrugatedWallBuilder.test.ts (test comment accuracy). Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/features/generation/worker/generators/corrugatedWallBuilder.test.ts
Line: 235-239
Comment:
**Misleading test comment attributes null to wrong check**
The comment says `Wavelength for height=4 is ~12mm, half = 6mm > 1.9mm → null`, implying the null return is because `wallSpan < baseWavelength / 2`. But `wallSpan = 80.8mm` easily passes that check — the actual rejection is `patternH = 1.9 < 2`. The second comment line is factually wrong and could mislead a future developer debugging why a spec returned null.
```suggestion
it('rejects very short walls', () => {
// Wall height of 5mm with 1.6mm bottom keepout and 1.5mm top = 1.9mm patternH
// patternH (1.9mm) < minimum required (2mm) → null
const spec = createCorrugatedSpec(1.6, 5, 80.8, 4);
expect(spec).toBeNull();
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/features/bin-designer/components/panel/WallsSection/PatternSelector.tsx
Line: 118-122
Comment:
**Hint message hardcoded to corrugated regardless of which patterns are disabled**
The `disabledPatterns` prop is typed as `ReadonlySet<WallPatternType>` (generic), but the rendered message is always `corrugatedThinWall`. If a future pattern is added to `disabledPatterns` for a different constraint, this will display the wrong explanation to users. Right now it works because only `corrugated` is ever added to the set, but the component contract doesn't express that coupling.
One approach is to guard more specifically:
```suggestion
{!disabled && disabledPatterns?.has('corrugated') && (
<p className="text-[11px] text-content-tertiary mt-1.5">
{t('binDesigner.walls.pattern.corrugatedThinWall')}
</p>
)}
```
This makes the trigger condition explicit and won't accidentally show the corrugated message if a different pattern is ever added to `disabledPatterns`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(corrugated): address PR review feedb..." | Re-trigger Greptile |
- Atomic cut+fuse: rollback to original bin if fuse fails (no holes) - Fix profile coordinates: outer face at Y=0, inner face positive Y - Complete cache key: add ramp zones, lip, compartment thickness, label - Replace misleading patternH < wavelength/2 with 2mm min height - Move CORRUGATED_MIN_WALL_THICKNESS to shared/constants/bin.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wallReplaceTargetspipeline concept: cut flat wall section, then fuse corrugated replacement (unlike honeycomb which is purely subtractive)cutvsreplacemodes) for clean extensibilityFiles
Created (4):
patterns/corrugatedPattern.ts— pure math: spec computation, wave profile pointspatterns/corrugatedPattern.test.ts— 14 unit testscorrugatedWallBuilder.ts— geometry: profile → solid, slab, clip zones, per-wall cachingcorrugatedWallBuilder.test.ts— 5 spec integration testsModified (13): types, registry, pipeline context/stages, constraints, UI selector, i18n
Test plan
pnpm run typecheck— passespnpm run lint— 0 errorspnpm run check:i18n— all 4 checks pass