Skip to content

fix: start new cairo subpath before drawing aperture holes#380

Merged
spe-ciellt merged 1 commit intogerbv:developfrom
SourceParts:fix/annular-ring-subpath
Mar 14, 2026
Merged

fix: start new cairo subpath before drawing aperture holes#380
spe-ciellt merged 1 commit intogerbv:developfrom
SourceParts:fix/annular-ring-subpath

Conversation

@rampageservices
Copy link
Copy Markdown
Contributor

Summary

Without cairo_new_sub_path(), Cairo connects the hole subpath to the outer aperture shape with an implicit line segment. This causes rendering artifacts on annular ring pads and any aperture with a drilled hole.

  • Add cairo_new_sub_path() call in gerbv_draw_aperture_hole() before drawing the hole shape, so the hole is rendered as an independent subpath
  • Regenerate 10 affected golden test images

Attribution

Extracted from #163 (@meantaipan).

Test plan

  • cmake --preset linux-gnu-gcc && cmake --build build — clean build
  • ctest -C Debug — 94/105 pass (11 pre-existing mismatches, unchanged from develop)
  • 10 golden images regenerated for tests that now render correctly

Without cairo_new_sub_path(), Cairo connects the hole subpath to
the outer aperture shape with an implicit line segment. This causes
rendering artifacts on annular ring pads and any aperture with a
drilled hole.

Add cairo_new_sub_path() in gerbv_draw_aperture_hole() so the hole
is drawn as an independent subpath, matching the expected winding-
rule cutout behavior.

Regenerate 10 affected golden test images.

Extracted from gerbv#163.
@spe-ciellt spe-ciellt merged commit 42ed1af into gerbv:develop Mar 14, 2026
3 checks passed
@spe-ciellt
Copy link
Copy Markdown
Contributor

Cosmetic

gerbv_draw_polygon has a now-redundant workaround with a misleading comment

/* skip first point, since we've moved there already */
/* include last point, since we may be drawing an aperture hole next
   and cairo may not correctly close the path itself */
for (i = 1; i <= (int)numberOfSidesInteger; i++) {
    ...
    cairo_line_to(...);  /* deliberately repeats the first vertex */
}

This comment (and the loop bound i <= numberOfSidesInteger instead of
i < numberOfSidesInteger) exists precisely because of the bug this PR fixes:
the author previously closed the polygon back to the start vertex so that the
implicit line Cairo would draw to the hole-arc start would be zero-length and
invisible. With cairo_new_sub_path() now in place, the workaround is no
longer needed and the comment is misleading.

The loop bound change would be a separate correctness question (closing vs.
not closing the polygon explicitly), but the comment should at minimum be
removed or updated so future readers are not confused about why the polygon is
drawn the way it is. Cosmetic, but likely to cause confusion in code review
later.

rampageservices added a commit to SourceParts/gerbv that referenced this pull request Mar 16, 2026
The loop in gerbv_draw_polygon ran i <= numberOfSidesInteger (inclusive)
to repeat the first vertex as a zero-length closing segment. The comment
explained this was needed because cairo might draw a spurious line from
the polygon endpoint to the start of an aperture hole subpath.

PR gerbv#380 fixed the root cause by adding cairo_new_sub_path() in
gerbv_draw_aperture_hole(), which starts a fresh subpath before drawing
any hole — severing the implicit connection entirely.

With that fix in place the extra vertex is dead code. Remove it and the
now-inaccurate comment. No functional change; golden images are unaffected.

Closes gerbv#433.
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.

3 participants