Skip to content

fix(paint): sample photos from their on-screen size instead of nearest neighbour - #468

Merged
LeadcodeDev merged 4 commits into
mainfrom
fix/image-sampling-465
Oct 1, 2026
Merged

LeadcodeDev merged 4 commits into
mainfrom
fix/image-sampling-465

Conversation

@LeadcodeDev

@LeadcodeDev LeadcodeDev commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

draw_image_rect with a default Paint samples with Skia's defaults: nearest neighbour, no mipmaps. A photo drawn smaller than its source keeps one pixel out of N and drops the rest, so hair and glasses alias; drawn larger it is enlarged pixel by pixel into blocks. On the issue's 640px checkerboard the 64px case is scale 1/10 exactly, every sample lands on the same parity, and the image renders solid white — measured at mean 255, min 255, max 255.

renderer::photo decides from the size the bitmap is finally drawn at rather than from its layout box: mipmapped linear when either axis minifies, Mitchell cubic otherwise, because cubic does not consult mipmaps and so is only correct when enlarging. The scale is read off canvas.local_to_device_as_3x3(), which already carries the camera, so a 640px rect under a camera at 0.1 is correctly treated as a 64px draw. Two details the diff does not explain: the axis scales are hypot(scale_x, skew_y) rather than scale_x alone, since the raw component reports a pure rotation as a shrink; and draw_photo takes impl AsRef<Image> to match Skia's own signature, which made the twenty call sites a textual swap with no borrow reasoning at each one.

Twenty sites now go through it — every place a raster bitmap is scaled into a destination rect. That is more than the issue listed: video, lottie and svg each had several, and transition::render_layer is reached through surface.canvas(). icon joins them too; it already asked for Mitchell, but unconditionally, so a shrunk icon had no mipmaps. The sites deliberately left alone all use the point form of draw_image, which places the bitmap at 1:1 so the sampling is never consulted: the full-frame composites in transition, the layer snapshots in paint_pass and render/scene, and engine/text/cosmic.rs, which CLAUDE.md records as not wired into rendering. paint_pass is the one of those that does sit under a scaling matrix, so it has the same defect — a snapshot of already-rasterised content is a different question from a photo, and it is left for one.

Cost, best of three over 300 frames at 1920x1080. Twelve 640px photos drawn at 52px: 1.01s to 1.13s. One 640px source filling the frame: 1.79s to 2.59s, which is 8.6ms a frame and is where cubic is paid for. Mipmaps are built once per image and cached on the SkImage, which is why the shrinking case barely moves. Cubic was kept rather than plain bilinear because it is what the issue asked for and what icon had already established, but the enlargement figure is the one to argue with if that trade is wrong.

Verification — six unit tests on the helper, two of which measure the old path so the fix cannot become vacuous: nearest neighbour reads 255/255 at scale 1/10 and 0-to-255 noise at 120/640, the helper reads mean 127 in both, and a 640px rect on a canvas scaled to 0.1 confirms the camera is read rather than the layout size. One integration test renders the issue's scenario through the image component and was confirmed red before the fix at mean 255.0, the figure the issue reports. Plus cargo fmt --all --check, cargo clippy --workspace --all-targets --features rustmotion/studio -- -D warnings, and cargo test --workspace --features rustmotion/studio: 2153 passed, 0 failed, 10 ignored across 53 suites.

The toolchain pin this needed is merged (#467), so this targets main and its diff is this change alone.

Closes #465

…ted From impl

`redundant_field_names` now fires on the two `#[from]` variants whose field is
named `source`: the derive generates `Self::JsonParse { source: source }`, and
1.99 attributes that to our field span. rustc 1.98.1 did not report it.

The allow sits on the module, not on the variants, because the generated
`impl From` is a sibling item of the enum rather than part of it: an attribute
on the variant leaves the lint exactly where it was, which is what the first
attempt measured. Renaming the field is not an option either way, since
`source` is what thiserror reads to implement `Error::source` and what the
`#[error("...{source}")]` strings interpolate.
CI installed the floating `stable` channel, so the version it tested was
whatever had shipped by the time the job ran. On 2026-10-01 that turned main red
with no commit in between: rustc moved 1.98.1 -> 1.99.0, clippy gained a lint on
code the repository had not touched, and the schema PR that was green on its own
branch failed once merged.

`rust-toolchain.toml` is now the single place the version is written.
`rustup toolchain install` with no argument reads that file, including its
`components`, so there is no second copy in the workflow to drift out of step
and no way for CI to check a version the working tree does not. It replaces
`dtolnay/rust-toolchain` in all four CI jobs and in publish: that action's
`toolchain` input is required and defaults to `stable`, and it does not read
the manifest, so keeping it would have meant writing the version five more
times. Runners ship rustup, and dropping the action removes a third-party
dependency from every job.

Relying on rustup's implicit auto-install was the alternative and is rejected:
it works today, but rustup prints a deprecation for it and says it may stop
working, which is the same class of delayed breakage this commit exists to
remove.

The `audit` job stays as it is. It is designed to fail on a new advisory, and
the ten `--ignore` entries are reviewed on a date written next to them. Making
that job unconditionally green would mean not auditing.
…t neighbour

`draw_image_rect` with a default `Paint` samples with Skia's defaults, which are
nearest neighbour and no mipmaps. A photo drawn smaller than its source kept one
source pixel out of N and dropped the rest, so it aliased; drawn larger it was
enlarged pixel by pixel into blocks. On the issue's 640px checkerboard the 64px
case is scale 1/10 exactly, every sample lands on the same parity, and the image
renders solid white.

`renderer::photo` decides from the size the bitmap is finally drawn at, camera
included: mipmapped linear when either axis minifies, Mitchell cubic otherwise,
since cubic does not consult mipmaps and is only right when enlarging. The scale
comes off `canvas.local_to_device_as_3x3()`, so a layout size of 640 under a
camera at 0.1 is correctly read as a 64px draw. The axis scales are
`hypot(scale_x, skew_y)` rather than `scale_x` alone: taking the raw component
would report a pure rotation as a shrink.

Twenty call sites now go through `draw_photo`, which is every place a raster
bitmap is scaled into a destination rect — the eleven components the issue
listed plus `transition::render_layer` and the `sheet` command. `icon` joins
them: it already asked for Mitchell, but unconditionally, so a shrunk icon still
had no mipmaps. The sites left alone all use the point form of `draw_image`,
which places the bitmap at 1:1 and never consults the sampling: the full-frame
composites in `transition`, the layer snapshots in `paint_pass` and
`render/scene`, and the cosmic-text bridge that is not wired into rendering.

Cost, best of three over 300 frames at 1920x1080: twelve 640px photos drawn at
52px go from 1.01s to 1.13s, and one 640px source filling the frame goes from
1.79s to 2.59s. The enlargement is where cubic is paid for, at 8.6ms a frame.
Mipmaps are built once per image and cached on the `SkImage`, which is why the
shrinking case barely moves.

Closes #465
@LeadcodeDev LeadcodeDev added the bug Something isn't working label Oct 1, 2026
Base automatically changed from fix/ci-pin-toolchain to main October 1, 2026 15:31
@LeadcodeDev LeadcodeDev self-assigned this Oct 1, 2026
Placed above the subpixel_font rule rather than below it so this and the
font-weight note on the other branch merge without touching the same lines.
@LeadcodeDev
LeadcodeDev merged commit 4f421c4 into main Oct 1, 2026
4 checks passed
@LeadcodeDev
LeadcodeDev deleted the fix/image-sampling-465 branch October 1, 2026 15:48
@LeadcodeDev LeadcodeDev mentioned this pull request Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Photos alias when shrunk and turn blocky under camera zoom: image and video sample with nearest neighbour and no mipmaps

1 participant