From f10e41409ddde31adf4e01111478ff44aaaed4ca Mon Sep 17 00:00:00 2001 From: Tanish Manku Date: Tue, 3 Nov 2026 16:52:11 -0700 Subject: [PATCH] Images: read no more than the limit, keep plot bounds to what a viewport shows, free textures with no images left Review fixes from CodeRabbit on #2749: - the image file is read through read_capped, so a file that grows after the size check can't exhaust memory - an image cut by a layout viewport widens the sheet's bounds only by the part inside it, as other primitives do - uploaded tiles of closed drawings are dropped also when the next list has no images --- crates/engine/src/cmd/image.rs | 6 +++++- crates/render/src/viewports.rs | 20 ++++++++++++-------- crates/render/tests/raster_image.rs | 3 +++ crates/ui-egui/src/canvas/images.rs | 26 ++++++++++++++++++-------- 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/cmd/image.rs b/crates/engine/src/cmd/image.rs index 240664891..daf46557b 100644 --- a/crates/engine/src/cmd/image.rs +++ b/crates/engine/src/cmd/image.rs @@ -79,7 +79,11 @@ fn read_file(path: &str, host: Option<&str>) -> std::result::Result, Str if len > MAX_IMAGE_BYTES { return Err(format!("the file is larger than {} MB", MAX_IMAGE_BYTES >> 20)); } - std::fs::read(&file).map_err(|e| e.to_string()) + // Read no more than the limit: the file can grow or change after the check above. + let limit = usize::try_from(MAX_IMAGE_BYTES).unwrap_or(usize::MAX); + crate::capped_read::read_capped(&file, limit).map_err(|e| { + if e.kind() == std::io::ErrorKind::InvalidData { format!("the file is larger than {} MB", MAX_IMAGE_BYTES >> 20) } else { e.to_string() } + }) } #[cfg(target_arch = "wasm32")] { diff --git a/crates/render/src/viewports.rs b/crates/render/src/viewports.rs index 6b501e840..f6e8c8cb0 100644 --- a/crates/render/src/viewports.rs +++ b/crates/render/src/viewports.rs @@ -255,20 +255,24 @@ fn append_clipped(dst: &mut DisplayList, src: &DisplayList, clip: &Clip) { /// Append image `img` (primitive `p`, corners `corners`) to `dst`, to be cut to `clip` when it /// is drawn: the picture is not cut here, the outline is added to those it carries. fn push_clipped_image(dst: &mut DisplayList, p: &DPrim, img: &DImage, corners: &[Vec2], clip: &Clip) { - let outline = match clip { - Clip::Rect(rect) => { - if !Bounds2::from_points(corners.iter().copied()).intersects(rect) { - return; - } - vec![rect.min, Vec2::new(rect.max.x, rect.min.y), rect.max, Vec2::new(rect.min.x, rect.max.y)] - } - Clip::Poly(c) => c.polygon().to_vec(), + let (outline, area) = match clip { + Clip::Rect(rect) => (vec![rect.min, Vec2::new(rect.max.x, rect.min.y), rect.max, Vec2::new(rect.min.x, rect.max.y)], *rect), + Clip::Poly(c) => (c.polygon().to_vec(), c.bounds()), }; + let own = Bounds2::from_points(corners.iter().copied()); + if !own.intersects(&area) { + return; + } let Ok(index) = u32::try_from(dst.images.len()) else { return }; let mut img = img.clone(); img.clips.push(outline); dst.images.push(img); + // The list's bounds take only the part inside the clip, as for the other primitives. + let before = dst.bounds; push_raw(dst, p, Kind::Image { index }, corners); + dst.bounds = before; + dst.bounds.add(own.min.max(area.min)); + dst.bounds.add(own.max.min(area.max)); } fn push_raw(dst: &mut DisplayList, p: &DPrim, kind: Kind, pts: &[Vec2]) { diff --git a/crates/render/tests/raster_image.rs b/crates/render/tests/raster_image.rs index 8cc2d09e8..bdf03dd44 100644 --- a/crates/render/tests/raster_image.rs +++ b/crates/render/tests/raster_image.rs @@ -269,6 +269,9 @@ fn a_layout_viewport_cuts_the_image_to_its_outline() { assert_eq!(img.clips[1], [Vec2::new(15.0, 10.0), Vec2::new(105.0, 10.0), Vec2::new(105.0, 110.0), Vec2::new(15.0, 110.0)]); let p = list.prims.iter().find(|p| matches!(p.kind, Kind::Image { index: 0 })).unwrap(); assert_eq!(list.points(p).len(), 4); + // The image (10..110 across) is wider than the viewport (15..105): only what shows counts + // towards the sheet's bounds, which a plot fits to. + assert!(list.bounds.min.x >= 15.0 - 1e-9 && list.bounds.max.x <= 105.0 + 1e-9, "{:?}", list.bounds); // Drawn: inside the viewport and the image's top row there is the picture; outside the // viewport, or in the clipped-away bottom row, there is none. diff --git a/crates/ui-egui/src/canvas/images.rs b/crates/ui-egui/src/canvas/images.rs index 4818c079d..afde7bee3 100644 --- a/crates/ui-egui/src/canvas/images.rs +++ b/crates/ui-egui/src/canvas/images.rs @@ -158,6 +158,11 @@ fn axis_rect(outline: &[Vec2]) -> Option { /// Draw the images of `list`. pub(super) fn draw(p: &egui::Painter, xf: &Xf, list: &DisplayList) { + // Pyramids whose pixels are gone (drawing closed, image undone) go, also when the list has + // no images left to draw. + if let Ok(mut pyramids) = textures(p.ctx()).0.lock() { + pyramids.retain(|_, pyramid| pyramid.pixels.strong_count() > 0); + } if list.images.is_empty() { return; } @@ -172,6 +177,11 @@ pub(super) fn draw(p: &egui::Painter, xf: &Xf, list: &DisplayList) { } } +/// The image tiles uploaded so far, kept between frames. +fn textures(ctx: &egui::Context) -> Textures { + ctx.data_mut(|d| d.get_temp_mut_or_default::(egui::Id::unique("cadcraft-raster-textures")).clone()) +} + /// Draw one image as textured meshes, one per tile on screen: its rectangle, or the outline it is /// clipped to. fn draw_one(p: &egui::Painter, xf: &Xf, img: &DImage, alpha: u8) { @@ -184,10 +194,8 @@ fn draw_one(p: &egui::Painter, xf: &Xf, img: &DImage, alpha: u8) { // Where in the raster the triangles are. let Some(tris) = triangles(img, cut.outline).iter().map(|q| img.pixel_of(*q)).collect::>>() else { return }; let color = Color32::from_white_alpha((img.opacity(alpha) * 255.0) as u8); - let textures = ctx.data_mut(|d| d.get_temp_mut_or_default::(egui::Id::unique("cadcraft-raster-textures")).clone()); + let textures = textures(ctx); let Ok(mut pyramids) = textures.0.lock() else { return }; - // Pyramids whose pixels are gone (drawing closed, image undone) go too. - pyramids.retain(|_, pyramid| pyramid.pixels.strong_count() > 0); let key = Arc::as_ptr(raster) as usize; let pyramid = match pyramids.entry(key) { Entry::Occupied(e) => e.into_mut(), @@ -484,15 +492,17 @@ mod tests { assert!(frame(&l) >= 1, "the first frame also uploads the font atlas"); assert_eq!(frame(&l), 0, "already uploaded"); // The pixels gone (drawing closed, image undone): the pyramid goes on the next draw. - let count = |ctx: &egui::Context| { - let t = ctx.data_mut(|d| d.get_temp_mut_or_default::(egui::Id::unique("cadcraft-raster-textures")).clone()); - t.0.lock().unwrap().len() - }; + let count = |ctx: &egui::Context| textures(ctx).0.lock().unwrap().len(); assert_eq!(count(&ctx), 1); drop(l); drop(r); - frame(&list(|_| {})); + let next = list(|_| {}); + frame(&next); assert_eq!(count(&ctx), 1, "only the new image's"); + // A list with no images still lets go of the pictures that are gone. + drop(next); + frame(&DisplayList::default()); + assert_eq!(count(&ctx), 0); } /// A known limit, kept visible here: on screen images are painted first, so they lie under