Skip to content

Split CNC layouts by thickness (v2509P) - #18

Merged
dorkmo merged 14 commits into
masterfrom
copilot/split-cnc-layouts-by-thickness
Nov 3, 2025
Merged

dorkmo merged 14 commits into
masterfrom
copilot/split-cnc-layouts-by-thickness

Conversation

Copilot AI commented Nov 2, 2025 •

Copy link
Copy Markdown
Contributor

Split CNC layouts by thickness for v2509P ✅

Successfully implemented the requirement to split CNC layout files by material thickness for v2509P.

Completed Tasks:

  • Analyze repository structure and part thickness definitions
  • Create separate layout files for each thickness:
    • ~CNCLAYOUT05.scad for 0.5" parts (d, f, a parts) - 287 lines
    • ~CNCLAYOUT0125.scad for 0.125" parts (h parts) - 37 lines
  • Update GitHub workflow to generate SVGs for all layout files
  • Update README to reference all layout files and explain naming convention
  • Fix positioning bugs found by code review in CNCLAYOUT05.scad
  • Run security check (CodeQL) - No issues found
  • Fix workflow detached HEAD issue for PR builds
  • Address PR review feedback (echo quotes, sed readability, code comments)

File Naming Convention (per requirements):
The thickness is appended to the filename in decimal format without the decimal point:

  • 05 represents 0.5 inches
  • 0125 represents 0.125 inches

Parts Distribution:

  • 0.5" thickness (CNCLAYOUT05):

    • 12 drawer parts (d1-d12)
    • 11 frame parts (f1-f6, f8-f9, f11-f14)
    • 6 arm parts (a1-a6)
    • Total: 29 parts
  • 0.125" thickness (CNCLAYOUT0125):

    • 4 guard/hopper parts (h1, h2, h7, h8)

Bug Fixes:
Fixed positioning errors discovered during code review in the 0.5" layout file:

  • Corrected a3 (2/2) positioning (removed extra a2x+ps)
  • Updated all subsequent parts to reflect corrected positions
  • Fixed f14 to use f13y instead of f13x

Workflow Updates:

  • The GitHub Actions workflow now generates three SVG files
  • Fixed checkout to use proper branch ref to avoid detached HEAD issues on PR builds
  • Improved echo statement quoting to display inch symbols correctly
  • Refactored long sed commands for better maintainability

Code Quality:

  • Added comments explaining intentional 0.5 spacing for a5/a6 parts

Security Summary:

  • CodeQL security scan completed: 0 vulnerabilities found
  • No security issues introduced by changes
Original prompt

This section details on the original issue you should resolve

<issue_title>Split CNC layouts by thickness</issue_title>
<issue_description>for v2509P, split up the CNC layout into multiple files, include only parts with the same thickness in each layout file. Append to the end of the CNC layoutfile the thickness in decimals but leave out the decimal point so that it doesnt interfere with the file type.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits November 2, 2025 23:41
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
@dorkmo

dorkmo commented Nov 2, 2025

Copy link
Copy Markdown
Collaborator

@copilot
The failure is due to attempting to git push from a detached HEAD state, which is not allowed by default. The log shows:

fatal: You are not currently on a branch.
To push the history leading to the current (detached HEAD) state now, use
git push origin HEAD:
Process completed with exit code 128.

Solution:

  1. Before pushing, check out the correct branch instead of operating on a detached HEAD. You can do this by adding a step to check out the branch associated with the pull request or your target branch.
  2. Alternatively, if you must push from a detached HEAD, specify the remote branch explicitly as suggested in the error (not recommended for CI workflows).

Code Suggestion:
Add this step before any git push commands in your workflow:

- name: Checkout branch
  run: |
    git fetch origin $GITHUB_HEAD_REF
    git checkout $GITHUB_HEAD_REF

Or, if using the official checkout action:

- uses: actions/checkout@v4
  with:
    ref: ${{ github.head_ref }}

This will ensure you are on the correct branch and not in a detached HEAD state, allowing git push to work as intended.

Summary: Update your workflow to check out the branch before running git push. This will resolve the exit code 128 error.

Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Copilot AI changed the title [WIP] Split CNC layouts into files by thickness Split CNC layouts by thickness (v2509P) Nov 2, 2025
Copilot AI requested a review from dorkmo November 2, 2025 23:54
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>

Copilot AI commented Nov 2, 2025

Copy link
Copy Markdown
Contributor Author

@copilot
The failure is due to attempting to git push from a detached HEAD state, which is not allowed by default. The log shows:

...

Fixed in commit 778b358. Added ref: ${{ github.head_ref || github.ref_name }} to the checkout action to ensure PRs check out the actual branch instead of detached HEAD. This allows the workflow to push generated SVGs back to the PR branch.

@dorkmo
dorkmo requested a review from Copilot November 3, 2025 02:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request introduces separated CNC layout files organized by material thickness to streamline the manufacturing process. Instead of having a single layout file with all parts, the project now has dedicated layout files for 0.5" thickness parts (~CNCLAYOUT05.scad) and 0.125" thickness parts (~CNCLAYOUT0125.scad), while maintaining the original comprehensive layout file.

Key changes:

  • Added ~CNCLAYOUT05.scad for 0.5" thickness parts (d, f, and a parts)
  • Added ~CNCLAYOUT0125.scad for 0.125" thickness parts (h parts)
  • Updated CI/CD workflow to generate SVGs for all three layout files
  • Enhanced README documentation to explain the thickness-based layout organization

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
v2509P/~CNCLAYOUT05.scad New layout file containing only 0.5" thick parts (drawer, frame, and arm components)
v2509P/~CNCLAYOUT0125.scad New layout file containing only 0.125" thick parts (guard/hopper components)
README.md Updated documentation to explain the new thickness-based layout structure with separate SVG references
.github/workflows/generate-cnclayout-svg.yml Extended workflow to generate and commit SVGs for all three layout files, added checkout ref configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread v2509P/~CNCLAYOUT05.scad Outdated
Comment thread v2509P/~CNCLAYOUT05.scad Outdated
use<a3p.scad>;
use<a4p.scad>;
use<a5p.scad>;
use<a6p.scad>;

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after 'use' keyword in module import statements. For consistency with lines 1-2 which use use <modus.scad>, add a space after 'use' (e.g., use <a1p.scad>;).

Suggested change
use<a6p.scad>;
use <a6p.scad>;

Copilot uses AI. Check for mistakes.
Comment thread v2509P/~CNCLAYOUT05.scad Outdated
Comment on lines +7 to +38
use<d1p.scad>;
use<d2p.scad>;
use<d3p.scad>;
use<d4p.scad>;
use<d5p.scad>;
use<d6p.scad>;
use<d7p.scad>;
use<d8p.scad>;
use<d9p.scad>;
use<d10p.scad>;
use<d11p.scad>;
use<d12p.scad>;

use<a1p.scad>;
use<a2p.scad>;
use<a3p.scad>;
use<a4p.scad>;
use<a5p.scad>;
use<a6p.scad>;

use<f1p.scad>;
use<f2p.scad>;
use<f3p.scad>;
use<f4p.scad>;
use<f5p.scad>;
use<f6p.scad>;
use<f8p.scad>;
use<f9p.scad>;
use<f11p.scad>;
use<f12p.scad>;
use<f13p.scad>;
use<f14p.scad>;

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after 'use' keyword in module import statements. For consistency with lines 1-2 which use use <modus.scad>, add a space after 'use' (e.g., use <f1p.scad>;).

Suggested change
use<d1p.scad>;
use<d2p.scad>;
use<d3p.scad>;
use<d4p.scad>;
use<d5p.scad>;
use<d6p.scad>;
use<d7p.scad>;
use<d8p.scad>;
use<d9p.scad>;
use<d10p.scad>;
use<d11p.scad>;
use<d12p.scad>;
use<a1p.scad>;
use<a2p.scad>;
use<a3p.scad>;
use<a4p.scad>;
use<a5p.scad>;
use<a6p.scad>;
use<f1p.scad>;
use<f2p.scad>;
use<f3p.scad>;
use<f4p.scad>;
use<f5p.scad>;
use<f6p.scad>;
use<f8p.scad>;
use<f9p.scad>;
use<f11p.scad>;
use<f12p.scad>;
use<f13p.scad>;
use<f14p.scad>;
use <d1p.scad>;
use <d2p.scad>;
use <d3p.scad>;
use <d4p.scad>;
use <d5p.scad>;
use <d6p.scad>;
use <d7p.scad>;
use <d8p.scad>;
use <d9p.scad>;
use <d10p.scad>;
use <d11p.scad>;
use <d12p.scad>;
use <a1p.scad>;
use <a2p.scad>;
use <a3p.scad>;
use <a4p.scad>;
use <a5p.scad>;
use <a6p.scad>;
use <f1p.scad>;
use <f2p.scad>;
use <f3p.scad>;
use <f4p.scad>;
use <f5p.scad>;
use <f6p.scad>;
use <f8p.scad>;
use <f9p.scad>;
use <f11p.scad>;
use <f12p.scad>;
use <f13p.scad>;
use <f14p.scad>;

Copilot uses AI. Check for mistakes.
Comment thread v2509P/~CNCLAYOUT0125.scad Outdated
Comment thread v2509P/~CNCLAYOUT05.scad Outdated

//a5 (1/1)
projection(cut = true)
translate([d1y+ps+d2y+ps+d3y+ps+d4y+ps+d5y+ps+d6y+ps+d7y+ps+d8y+ps+d9y+ps+d10y+ps+d11y+ps+d12y+ps+a1x+ps+a1x+ps+a2x+ps+a2x+ps+a3x+ps+a3x+ps+a4x+0.5+a5x,0,0])

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number 0.5 appears in the translate calculation without explanation. This inconsistency with the 'ps' variable (partspacing) used elsewhere could lead to confusion. Consider adding a comment explaining why 0.5 is used here instead of 'ps', or define it as a named constant if this represents a different spacing requirement.

Copilot uses AI. Check for mistakes.
Comment thread v2509P/~CNCLAYOUT05.scad Outdated

//a6 (1/1)
projection(cut = true)
translate([d1y+ps+d2y+ps+d3y+ps+d4y+ps+d5y+ps+d6y+ps+d7y+ps+d8y+ps+d9y+ps+d10y+ps+d11y+ps+d12y+ps+a1x+ps+a1x+ps+a2x+ps+a2x+ps+a3x+ps+a3x+ps+a4x+0.5+a5x+ps+a6x,a6x/2,0])

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number 0.5 appears in the translate calculation without explanation. This inconsistency with the 'ps' variable (partspacing) used elsewhere could lead to confusion. Consider adding a comment explaining why 0.5 is used here instead of 'ps', or define it as a named constant if this represents a different spacing requirement.

Copilot uses AI. Check for mistakes.
dorkmo and others added 3 commits November 2, 2025 20:17
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@dorkmo
dorkmo marked this pull request as ready for review November 3, 2025 02:22
@dorkmo
dorkmo requested a review from Copilot November 3, 2025 02:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread README.md Outdated
xvfb-run -a openscad --render --imgsize=2048,1536 --colorscheme=Tomorrow --projection=ortho --camera=0,0,0,0,0,0,1000 -o cnclayout05.svg ~CNCLAYOUT05.scad

if [ -f cnclayout05.svg ]; then
echo "✅ CNC Layout 0.5\" SVG generated successfully"

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The escaped quote \" in the echo statement will display as a backslash in the output. Consider using single quotes around the entire message or alternative quoting to properly display the inch symbol.

Copilot uses AI. Check for mistakes.
xvfb-run -a openscad --render --imgsize=2048,1536 --colorscheme=Tomorrow --projection=ortho --camera=0,0,0,0,0,0,1000 -o cnclayout0125.svg ~CNCLAYOUT0125.scad

if [ -f cnclayout0125.svg ]; then
echo "✅ CNC Layout 0.125\" SVG generated successfully"

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The escaped quote \" in the echo statement will display as a backslash in the output. Consider using single quotes around the entire message or alternative quoting to properly display the inch symbol.

Copilot uses AI. Check for mistakes.
if grep -q "assembly.png" README.md; then
# Add after the assembly image
sed -i '/assembly.png/a\\n## CNC Cutting Layout\n\n![CNC Cutting Layout](v2509P/cnclayout.svg)\n' README.md
sed -i '/assembly.png/a\\n## CNC Cutting Layouts\n\n### All Parts\n![CNC Cutting Layout - All Parts](v2509P/cnclayout.svg)\n\n### 0.5" Thickness (Drawer, Frame, Arm Parts)\n![CNC Cutting Layout - 0.5" Thickness](v2509P/cnclayout05.svg)\n\n### 0.125" Thickness (Guard/Hopper Parts)\n![CNC Cutting Layout - 0.125" Thickness](v2509P/cnclayout0125.svg)\n' README.md

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sed command is extremely long and difficult to maintain. Consider extracting the multi-line markdown content into a heredoc or separate file, or breaking it into multiple sed commands for better readability.

Copilot uses AI. Check for mistakes.
else
# Add after the title if no assembly image exists
sed -i '/# Compressed Earth Block Press/a\\n## CNC Cutting Layout\n\n![CNC Cutting Layout](v2509P/cnclayout.svg)\n' README.md
sed -i '/# Compressed Earth Block Press/a\\n## CNC Cutting Layouts\n\n### All Parts\n![CNC Cutting Layout - All Parts](v2509P/cnclayout.svg)\n\n### 0.5" Thickness (Drawer, Frame, Arm Parts)\n![CNC Cutting Layout - 0.5" Thickness](v2509P/cnclayout05.svg)\n\n### 0.125" Thickness (Guard/Hopper Parts)\n![CNC Cutting Layout - 0.125" Thickness](v2509P/cnclayout0125.svg)\n' README.md

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sed command is extremely long and difficult to maintain. Consider extracting the multi-line markdown content into a heredoc or separate file, or breaking it into multiple sed commands for better readability.

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@dorkmo

dorkmo commented Nov 3, 2025

Copy link
Copy Markdown
Collaborator

@copilot apply changes based on the comments in this thread

… add comments for magic numbers

Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>

Copilot AI commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

@copilot apply changes based on the comments in this thread

Applied in commit 5824183. Changes include:

  • Fixed echo statements to use single quotes for proper inch symbol display
  • Refactored long sed commands into maintainable variables
  • Added comments explaining the intentional 0.5 spacing for a5/a6 parts (carried over from original layout)

@dorkmo
dorkmo requested a review from Copilot November 3, 2025 02:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread v2509P/~CNCLAYOUT05.scad Outdated
Comment thread v2509P/~CNCLAYOUT0125.scad Outdated
Comment thread .github/workflows/generate-cnclayout-svg.yml
dorkmo and others added 3 commits November 2, 2025 20:38
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@dorkmo
dorkmo merged commit 329a1f9 into master Nov 3, 2025
2 checks passed
@dorkmo
dorkmo deleted the copilot/split-cnc-layouts-by-thickness branch November 3, 2025 02:44
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.

Split CNC layouts by thickness

4 participants