Comments for future updates
1. Code duplication — calc_cirseg_mq and calc_cirseg_bbox re-implemented verbatim
Both are static in gerber.c so they can't be called from drill.c. The PR duplicates:
- Full body of
calc_cirseg_mq() (~30 lines of math + comments)
- Most of
calc_cirseg_bbox() (~25 lines) reimplemented inline in the bbox block
If a bug is ever found in calc_cirseg_mq, it needs fixing in two places. The correct structural fix is to move both functions to a shared location (e.g., gerb_image.c) and export them. That's a larger refactor — acceptable to defer, but worth a TODO comment or a follow-up issue.
2. Bounding box uses floor() — differs from calc_cirseg_bbox but is actually more correct
The PR's middle-point loop:
for (step_pi_2 = (floor(ang1 / M_PI_2) + 1) * M_PI_2; ...)
calc_cirseg_bbox in gerber.c uses:
for (step_pi_2 = (ang1/M_PI_2 + 1)*M_PI_2; ...)
For non-axis-aligned ang1 (e.g. 45°), gerber.c gives start = 1.5×(π/2) = 135° — which skips the 90° axis crossing. The PR's floor(0.5)=0 gives start = 1×(π/2) = 90° — which is correct (first axis crossing after 45°). The PR accidentally fixes a latent imprecision in the original bbox algorithm. No action needed here, but it's worth documenting.
3. delta_cp_x/y not reset on T-code tool change (drill.c:887)
case 'T':
drill_parse_T_code(fd, state, image, file_line);
state->route_mode = DRILL_G_DRILL;
state->tool_down = FALSE;
/* delta_cp_x/y NOT reset here */
break;
In practice this is harmless — both fields are cleared after every arc segment and on tool-up, so by the time a T-code is seen they're already 0. But for defensive consistency with the route_mode/tool_down resets, adding state->delta_cp_x = state->delta_cp_y = 0; here would be better hygiene.
Great patch, will merge it and drop an issue for future reference.
Originally posted by @spe-ciellt in #298 (comment)
Comments for future updates
1. Code duplication —
calc_cirseg_mqandcalc_cirseg_bboxre-implemented verbatimBoth are
staticingerber.cso they can't be called fromdrill.c. The PR duplicates:calc_cirseg_mq()(~30 lines of math + comments)calc_cirseg_bbox()(~25 lines) reimplemented inline in the bbox blockIf a bug is ever found in
calc_cirseg_mq, it needs fixing in two places. The correct structural fix is to move both functions to a shared location (e.g.,gerb_image.c) and export them. That's a larger refactor — acceptable to defer, but worth a TODO comment or a follow-up issue.2. Bounding box uses
floor()— differs fromcalc_cirseg_bboxbut is actually more correctThe PR's middle-point loop:
calc_cirseg_bboxingerber.cuses:For non-axis-aligned
ang1(e.g. 45°),gerber.cgives start =1.5×(π/2)= 135° — which skips the 90° axis crossing. The PR'sfloor(0.5)=0gives start =1×(π/2)= 90° — which is correct (first axis crossing after 45°). The PR accidentally fixes a latent imprecision in the original bbox algorithm. No action needed here, but it's worth documenting.3.
delta_cp_x/ynot reset on T-code tool change (drill.c:887)In practice this is harmless — both fields are cleared after every arc segment and on tool-up, so by the time a T-code is seen they're already 0. But for defensive consistency with the
route_mode/tool_downresets, addingstate->delta_cp_x = state->delta_cp_y = 0;here would be better hygiene.Great patch, will merge it and drop an issue for future reference.
Originally posted by @spe-ciellt in #298 (comment)