fix(paint): sample photos from their on-screen size instead of nearest neighbour - #468
Merged
Merged
Conversation
…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
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.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
draw_image_rectwith a defaultPaintsamples 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::photodecides 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 offcanvas.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 arehypot(scale_x, skew_y)rather thanscale_xalone, since the raw component reports a pure rotation as a shrink; anddraw_phototakesimpl 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,lottieandsvgeach had several, andtransition::render_layeris reached throughsurface.canvas().iconjoins 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 ofdraw_image, which places the bitmap at 1:1 so the sampling is never consulted: the full-frame composites intransition, the layer snapshots inpaint_passandrender/scene, andengine/text/cosmic.rs, which CLAUDE.md records as not wired into rendering.paint_passis 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 whaticonhad 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
imagecomponent and was confirmed red before the fix at mean 255.0, the figure the issue reports. Pluscargo fmt --all --check,cargo clippy --workspace --all-targets --features rustmotion/studio -- -D warnings, andcargo 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
mainand its diff is this change alone.Closes #465