address review: one paint allowance across items, derived paints counted, bounded trap pair scan, dash error names both limits

crates/effects/src/solids.rs

@@ -226,6 +226,13 @@ fn surface(n: &Node, facets: Vec<Facet>, 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<Vec<Arc<Node>>> {
+fn art_for(n: &Node, fx: &Effect, path: &PathData, rule: FillRule, items: &[&AppearanceItem], paint_bytes: &mut usize) -> Option<Vec<Arc<Node>>> {
     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<Node> {
     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<Effect>;
     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<Node> {
         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);

crates/effects/src/trap.rs

@@ -219,14 +219,25 @@ fn visible(sources: &[TrapSource]) -> Vec<PathData> {
 /// `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<Rect>], reach_x: f64, reach_y: f64) -> Option<Vec<(usize, usize)>> {
+    near_pairs_within(rects, reach_x, reach_y, MAX_NEAR_SCAN)
+}
+
+fn near_pairs_within(rects: &[Option<Rect>], reach_x: f64, reach_y: f64, max_scan: usize) -> Option<Vec<(usize, usize)>> {
     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<Rect>], reach_x: f64, reach_y: f64) -> Option<Vec<
     Some(pairs)
 }
 
+/// Most pairs the sweep in `near_pairs` examines, stored or not (a plain bounds check each).
+const MAX_NEAR_SCAN: usize = 1 << 27;
+
 /// Most candidate pairs one trap evaluation keeps; past it no traps are made (the art draws as is).
 /// Each pair costs a boolean trap band, so real art stays far below this.
 pub const MAX_NEAR_PAIRS: usize = 1 << 20;
@@ -247,12 +261,14 @@ pub const MAX_NEAR_PAIRS: usize = 1 << 20;
 /// different solid colours whose visible parts meet, the strip of the darker one's visible part
 /// next to the lighter one's (the other way round with [`TrapOptions::reverse`]), painted per
 /// [`trap_paint`]. Empty when no visible edges of different colours meet, or when there are more
-/// than [`MAX_NEAR_PAIRS`] candidate pairs (see [`try_traps`] to tell those apart).
+/// than [`MAX_NEAR_PAIRS`] candidate pairs or the pair scan is too long (see [`try_traps`] to tell
+/// those apart).
 pub fn traps(sources: &[TrapSource], o: &TrapOptions) -> Vec<TrapPiece> {
     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<Vec<TrapPiece>> {
     let colors: Vec<Option<Color>> = 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<Option<Rect>> = (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())];

crates/engine/src/cmd/addons/pathtools.rs

@@ -774,8 +774,9 @@ fn divide_dashes(s: &mut Session, p: &Value) -> Result<Value> {
                 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<NodeId> {
 
 fn expand_dashes(s: &mut Session, p: &Value) -> Result<Value> {
     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.

crates/engine/src/cmd/pathops.rs

@@ -723,7 +723,7 @@ fn trap(s: &mut Session, p: &Value) -> Result<Value> {
         }
         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
             )));
         };