Skip to content

PNG cache ignored --resolution, and a multi-page cache hit crashed - #23

Merged
set-soft merged 3 commits into
INTI-CMNB:masterfrom
mike-shevchenko:fix-png-cache-resolution
Sep 21, 2026
Merged

set-soft merged 3 commits into
INTI-CMNB:masterfrom
mike-shevchenko:fix-png-cache-resolution

Conversation

@mike-shevchenko

@mike-shevchenko mike-shevchenko commented Sep 19, 2026 •

Copy link
Copy Markdown

Two independent fixes in pdf2png(), one commit each.

1. A multi-page cache hit crashes

The branch that returns a cached set of multi-page PNGs tests isfile(destm) but then reads
getmtime(dest1):

if isfile(destm) and getmtime(dest1) > source_mtime:

When only the multi-page files are present, dest1 does not exist and getmtime raises
FileNotFoundError instead of using the cache.

2. The PNG cache ignores --resolution

A cache hit is decided from the file times alone, and the resolution is not part of the name,
so PNGs rasterized at one resolution are reused at another and --resolution is silently
discarded. Same command, same board, only the cache differs:

cache requested resulting page
fresh 400 DPI 4678 x 3308
populated at 150 DPI 400 DPI 1754 x 1241

The second row is the bug: the run takes ~9 s instead of ~58 s and quietly produces 150 DPI
output. It is easy to hit, because the natural workflow is to keep one --cache_dir and vary
the resolution per invocation.

Why the PNG names, and not the options file

cur_pcb_ops already invalidates the cache when KiCad or --zones changes, and adding
resolution there would be a smaller diff. But the layer PDFs are vector and do not depend on
the resolution at all - only the rasterization does. Invalidating on resolution would therefore
throw away the expensive plot to redo the cheap part.

Tagging only the PNG names keeps one plot shared by every resolution, so switching
re-rasterizes but never re-plots, and caches for several resolutions coexist in one directory:

0_1.pdf          <- one plot, resolution independent
0_1.r150.png     <- rasters, one per resolution
0_1.r400.png

Verified on a real board: against the cache that previously broke it, --resolution 400 now
gives 4678 x 3308 and --resolution 150 gives 1754 x 1241, both served from the same PDFs.

Not addressed

The schematic path has the same flaw. svg2png() rasterizes at the same resolution, but its
PNG names double as sheet keys in layer_names, so tagging them is not a local change and
seemed better left to your judgement.

Untagged PNGs in an existing cache are simply never read again. They are not cleaned up.

Note on #22

This touches the same two files as #22, though a different function. The two are independent
and can be taken in either order; whichever merges second will conflict on the [Unreleased]
heading, where both bullets should be kept.

Mike Shevchenko added 2 commits September 19, 2026 04:17
The branch that returns a cached set of multi-page PNGs checks isfile(destm)
but then reads getmtime(dest1). When only the multi-page files exist, dest1
is absent and getmtime raises FileNotFoundError instead of using the cache.
pdf2png decided a cache hit from the file times alone, so a cache populated
at one resolution was reused at another and --resolution was silently
discarded. Diffing at 400 DPI against a cache built at 150 gave 150 DPI
images.

The PNG names now carry the resolution. The layer PDFs they come from are
vector, so they stay shared: changing the resolution re-rasterizes but never
re-plots, and caches for several resolutions coexist in one directory.

The schematic path has the same flaw and is not touched here. svg2png
rasterizes at the same resolution, but its PNG names double as sheet keys in
layer_names, so tagging them is not a local change. Old untagged PNGs left in
a cache are simply never read again.
@mike-shevchenko mike-shevchenko changed the title Fix png cache resolution PNG cache ignored --resolution, and a multi-page cache hit crashed Sep 19, 2026
@set-soft

Copy link
Copy Markdown
Member

Ok, I'm taking the patch, but next time please separate them and tag WIP one if incomplete.
Thanks!

@set-soft
set-soft merged commit 46f3c60 into INTI-CMNB:master Sep 21, 2026
1 of 2 checks passed
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.

2 participants