From 18556f2a843e54898a3f661b414bb2fb2bd27bea Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 05:59:18 +0000 Subject: [PATCH] address review: one paint allowance across items, derived paints counted, bounded trap pair scan, dash error names both limits --- crates/effects/src/solids.rs | 21 ++++++++++------ crates/effects/src/trap.rs | 30 ++++++++++++++++++++--- crates/engine/src/cmd/addons/pathtools.rs | 7 +++--- crates/engine/src/cmd/pathops.rs | 2 +- 4 files changed, 46 insertions(+), 14 deletions(-) diff --git a/crates/effects/src/solids.rs b/crates/effects/src/solids.rs index 193f47c91..7348c9890 100644 --- a/crates/effects/src/solids.rs +++ b/crates/effects/src/solids.rs @@ -226,6 +226,13 @@ fn surface(n: &Node, facets: Vec, paint: &Paint, opacity: f32, blend: Ble group } +/// Paint copies `surface` makes for `facets`: two per facet (fill and seam stroke), six when colored +/// lights give each facet derived paints (`lighting::solid_surface`). +fn facet_copies(facets: &[Facet], paint: &Paint, colored: bool) -> usize { + let derived = colored && paint.color().is_some() && facets.iter().any(|f| f.rgb_gradient.is_some()); + facets.len().saturating_mul(if derived { 6 } else { 2 }) +} + fn paint_of(item: &AppearanceItem) -> (Paint, f32, BlendMode) { match item { AppearanceItem::Fill(f) => (f.paint.clone(), f.opacity, f.blend), @@ -249,21 +256,20 @@ fn region(path: &PathData, rule: FillRule, item: &AppearanceItem) -> (PathData, /// a surface one composite or masks it, their faces are drawn in one back-to-front order (where /// depths tie, as for the caps turned toward the viewer, in the order of the appearance); /// otherwise each solid is a surface of its own, in that order. -fn art_for(n: &Node, fx: &Effect, path: &PathData, rule: FillRule, items: &[&AppearanceItem]) -> Option>> { +fn art_for(n: &Node, fx: &Effect, path: &PathData, rule: FillRule, items: &[&AppearanceItem], paint_bytes: &mut usize) -> Option>> { let b = path.bounds()?; let p = crate::merged_params(&fx.id, &fx.params); let colored = view_options(&p).has_colored_lights(); if fx.id == ROTATE { // Every part turns about the centre of the object's own bounds, so they stay together. let mut out = vec![]; - let mut paint_bytes = 0usize; for item in items { let (paint, opacity, blend) = paint_of(item); let (area, rule) = region(path, rule, item); let f = facets(ROTATE, &area, rule, b, &p).ok()?; // Each facet and its seam stroke copy the paint; past the allowance (all items together) // the source path is drawn. - if !crate::revolve::paint_copies_add(&mut paint_bytes, f.len().saturating_mul(2), &paint) { + if !crate::revolve::paint_copies_add(paint_bytes, facet_copies(&f, &paint, colored), &paint) { return None; } out.push(Arc::new(surface(n, f, &paint, opacity, blend, colored))); @@ -291,11 +297,10 @@ fn art_for(n: &Node, fx: &Effect, path: &PathData, rule: FillRule, items: &[&App parts.iter().map(|(_, area, rule)| facets(&fx.id, area, *rule, bounds, &p)).collect() }; let mut solids = vec![]; - let mut paint_bytes = 0usize; for ((item, _, _), f) in parts.iter().zip(built) { let item: &AppearanceItem = item; match f { - Ok(f) if !crate::revolve::paint_copies_add(&mut paint_bytes, f.len().saturating_mul(2), &paint_of(item).0) => return None, + Ok(f) if !crate::revolve::paint_copies_add(paint_bytes, facet_copies(&f, &paint_of(item).0, colored), &paint_of(item).0) => return None, Ok(f) => solids.push((item, f)), // A stroke's outline past the limits is left out; the fill's solid still draws. Err(_) if matches!(item, AppearanceItem::Stroke(_)) => {} @@ -354,11 +359,13 @@ pub fn solid_art(n: &Node) -> Option { let ctx = GeomContext::of(n); let paints = solid_paints(n); let painted: Vec<&AppearanceItem> = paints.iter().collect(); + // One paint-copy allowance for every item's solid together. + let mut paint_bytes = 0usize; let mut children = vec![]; let remaining: Vec; if let Some((index, fx)) = solid(&n.appearance.effects) { let path = geometry(&path, n.appearance.effects.get(..index)?, &ctx); - children.extend(art_for(n, fx, &path, rule, &painted)?); + children.extend(art_for(n, fx, &path, rule, &painted, &mut paint_bytes)?); // Later geometry effects act on the projected art as a whole. remaining = n .appearance @@ -374,7 +381,7 @@ pub fn solid_art(n: &Node) -> Option { for item in &painted { if let Some((index, fx)) = solid(item.effects()) { let p = geometry(&path, item.effects().get(..index)?, &ctx.item(item)); - for mut art in art_for(n, fx, &p, rule, &[*item])? { + for mut art in art_for(n, fx, &p, rule, &[*item], &mut paint_bytes)? { Arc::make_mut(&mut art).appearance.effects = item.effects().iter().skip(index + 1).filter(|e| !is_three_d(&e.id)).cloned().collect(); children.push(art); diff --git a/crates/effects/src/trap.rs b/crates/effects/src/trap.rs index 98287a3b7..b2721ed0f 100644 --- a/crates/effects/src/trap.rs +++ b/crates/effects/src/trap.rs @@ -219,14 +219,25 @@ fn visible(sources: &[TrapSource]) -> Vec { /// `reach_x` of each other across x and `reach_y` across y. A sweep over the rects sorted by their /// left edge, so shapes spread over the page cost about one check each rather than one per pair. /// `None` when there are more than [`MAX_NEAR_PAIRS`] (many shapes all within reach of each other -/// make the pair list grow with the square of their count). +/// make the pair list grow with the square of their count) or the sweep examines more than +/// `MAX_NEAR_SCAN` pairs (many shapes sharing an x range, far apart in y). fn near_pairs(rects: &[Option], reach_x: f64, reach_y: f64) -> Option> { + near_pairs_within(rects, reach_x, reach_y, MAX_NEAR_SCAN) +} + +fn near_pairs_within(rects: &[Option], reach_x: f64, reach_y: f64, max_scan: usize) -> Option> { let mut order: Vec<(usize, Rect)> = rects.iter().enumerate().filter_map(|(i, r)| r.map(|r| (i, r))).collect(); order.sort_unstable_by(|a, b| a.1.x0.total_cmp(&b.1.x0).then(a.0.cmp(&b.0))); let mut pairs = vec![]; + let mut scanned = 0usize; for (k, &(i, a)) in order.iter().enumerate() { // Later rects start at or right of `a`, so only how far right they start can rule them out. for &(j, b) in order[k + 1..].iter().take_while(|(_, b)| b.x0 <= a.x1 + reach_x) { + // Rects sharing an x range but far apart in y are examined without being stored. + scanned += 1; + if scanned > max_scan { + return None; + } if b.y0 <= a.y1 + reach_y && a.y0 <= b.y1 + reach_y { if pairs.len() >= MAX_NEAR_PAIRS { return None; @@ -239,6 +250,9 @@ fn near_pairs(rects: &[Option], reach_x: f64, reach_y: f64) -> Option Vec { try_traps(sources, o).unwrap_or_default() } -/// [`traps`], but `None` when there are more than [`MAX_NEAR_PAIRS`] candidate pairs, as opposed to +/// [`traps`], but `None` when there are more than [`MAX_NEAR_PAIRS`] candidate pairs or the scan for +/// them is too long (`MAX_NEAR_SCAN` pairs examined), as opposed to /// `Some` of an empty list when no edges meet. pub fn try_traps(sources: &[TrapSource], o: &TrapOptions) -> Option> { let colors: Vec> = sources.iter().map(|s| trap_color(&s.fill)).collect(); @@ -396,6 +412,14 @@ mod tests { assert!(near(po::area(&t[0].path, FillRule::NonZero), 22.0 * 22.0 - 400.0)); } + #[test] + fn the_pair_scan_is_bounded_even_when_no_pair_is_near() { + // Eleven rects share an x range but lie far apart in y: 55 pairs are examined, none stored. + let rects: Vec> = (0..11).map(|k| Some(Rect::new(0.0, k as f64 * 1000.0, 10.0, k as f64 * 1000.0 + 10.0))).collect(); + assert_eq!(near_pairs_within(&rects, 1.0, 1.0, 100), Some(vec![])); + assert_eq!(near_pairs_within(&rects, 1.0, 1.0, 54), None); + } + #[test] fn no_trap_without_adjoining_edges_of_different_colours() { let apart = vec![src(Rect::new(0.0, 0.0, 10.0, 10.0), yellow()), src(Rect::new(30.0, 0.0, 40.0, 10.0), blue())]; diff --git a/crates/engine/src/cmd/addons/pathtools.rs b/crates/engine/src/cmd/addons/pathtools.rs index 5d0e5723f..9ddcc4355 100644 --- a/crates/engine/src/cmd/addons/pathtools.rs +++ b/crates/engine/src/cmd/addons/pathtools.rs @@ -774,8 +774,9 @@ fn divide_dashes(s: &mut Session, p: &Value) -> Result { continue; } let only = (segs.len() < sp.segment_count()).then_some(&segs); - let out = pt::divide_dashes(sp, only, spec, MAX_PIECES.saturating_sub(made)) - .ok_or_else(|| bad(C, "that makes too many pieces (at most 100000): make the dashes longer"))?; + let out = pt::divide_dashes(sp, only, spec, MAX_PIECES.saturating_sub(made)).ok_or_else(|| { + bad(C, "that makes too many pieces (at most 100000) or dash steps: simplify the dash pattern or make the dashes longer") + })?; made += out.len(); for (piece, kind) in out.into_iter().filter(|(_, k)| !(remove_gaps && *k == pt::Piece::Gap)) { dashes += usize::from(kind == pt::Piece::Dash); @@ -825,7 +826,7 @@ fn dashed_paths(d: &Document, roots: &[NodeId]) -> Vec { fn expand_dashes(s: &mut Session, p: &Value) -> Result { const C: &str = "addon.pathtools.expandDashes"; - let too_many = || bad(C, "that makes too many pieces (at most 100000): make the dashes longer"); + let too_many = || bad(C, "that makes too many pieces (at most 100000) or dash steps: simplify the dash pattern or make the dashes longer"); let roots = checked_ids_or_selection(s, p, C)?; let d = &s.doc()?.doc; // Each dashed path and its dashes, as the canvas draws them. diff --git a/crates/engine/src/cmd/pathops.rs b/crates/engine/src/cmd/pathops.rs index 04887e291..1d701e13f 100644 --- a/crates/engine/src/cmd/pathops.rs +++ b/crates/engine/src/cmd/pathops.rs @@ -723,7 +723,7 @@ fn trap(s: &mut Session, p: &Value) -> Result { } let Some(made) = try_traps(&sources, &options) else { return Err(EngineError::Other(format!( - "Trap: more than {} pairs of objects lie close enough to trap; trap fewer objects at a time", + "Trap: more than {} pairs of objects lie close enough to trap, or too many objects share a horizontal range; trap fewer objects at a time", vectorcraft_render::effects::trap::MAX_NEAR_PAIRS ))); };