Merge pull request #7060 from dotCooCoo/fix/dcs-regained-clip

Place: a DCS file that regains its clipping path clips the group that stayed

crates/engine/src/cmd/links.rs

@@ -629,9 +629,13 @@ fn clip_shape((frame, room): Clip) -> PathData {
 enum ClipTo {
     /// It stays as it is.
     Keep,
-    /// It is clipped to this path: the clipping path of the clip group holding just it changes,
-    /// else it goes into a new clip group.
+    /// It is clipped to this path (Placement Options' clip): the clipping path of the clip group
+    /// holding just it changes, else it goes into a new clip group.
     Path(PathData),
+    /// It is clipped to this path, the clipping path its DCS file draws it through: as [`ClipTo::Path`],
+    /// except that a plain group holding just it (as [`ClipTo::Off`] leaves one) becomes a clip group
+    /// again.
+    File(PathData),
     /// It goes into a new clip group of this path, inside the clip group that holds it.
     Nest(PathData),
     /// The clipping path of the clip group holding just it goes (a DCS file's clipping path that
@@ -693,7 +697,7 @@ fn clip_to(
     match img.clip.as_ref().map(|path| path.transformed(new_xf)) {
         // A clipping mask made in the document stays, with the file's clip group inside it.
         Some(path) if held.is_some() && !file_held => ClipTo::Nest(path),
-        Some(path) => ClipTo::Path(path),
+        Some(path) => ClipTo::File(path),
         None if file_held => ClipTo::Off,
         None => ClipTo::Keep,
     }
@@ -742,7 +746,7 @@ impl Nested<'_> {
         let mut new = Arc::new(new);
         self.update(&mut new, true);
         *n = match clip {
-            ClipTo::Path(path) | ClipTo::Nest(path) => Arc::new(self.clip_group(new, path)),
+            ClipTo::Path(path) | ClipTo::File(path) | ClipTo::Nest(path) => Arc::new(self.clip_group(new, path)),
             ClipTo::Keep | ClipTo::Off => new,
         };
     }
@@ -758,12 +762,16 @@ impl Nested<'_> {
         let mut slots: Vec<(usize, Arc<Node>)> = vec![];
         // The object a clip group that loses its clipping path holds alone.
         let mut declipped = None;
+        // The clipping path and object of a plain group holding just the object, which becomes a
+        // clip group again.
+        let mut reclipped = None;
         if let Some(children) = n.children() {
             // A clip group holding just the object: its clipping path changes.
             let clip_path = match (&n.kind, &children[..]) {
                 (NodeKind::Group { clip: true, .. }, [c, _]) if matches!(c.kind, NodeKind::Path { clipping: true, .. }) => Some(c),
                 _ => None,
             };
+            let plain = matches!((&n.kind, &children[..]), (NodeKind::Group { clip: false, .. }, [_]));
             for (i, c) in children.iter().enumerate() {
                 if !(nested && (self.pick)(c)) {
                     let mut new = Arc::clone(c);
@@ -782,14 +790,15 @@ impl Nested<'_> {
                 self.update(&mut new, true);
                 match (clip, clip_path) {
                     (ClipTo::Keep, _) => slots.push((i, new)),
-                    (ClipTo::Path(clip), Some(path)) if i == 1 => {
+                    (ClipTo::Path(clip) | ClipTo::File(clip), Some(path)) if i == 1 => {
                         let mut path = Node::clone(path);
                         if let NodeKind::Path { path: p, .. } = &mut path.kind {
                             *p = clip;
                         }
                         slots.extend([(0, Arc::new(path)), (1, new)]);
                     }
-                    (ClipTo::Path(clip) | ClipTo::Nest(clip), _) => {
+                    (ClipTo::File(clip), None) if plain => reclipped = Some((clip, new)),
+                    (ClipTo::Path(clip) | ClipTo::File(clip) | ClipTo::Nest(clip), _) => {
                         let group = self.clip_group(new, clip);
                         slots.push((i, Arc::new(group)));
                     }
@@ -798,13 +807,21 @@ impl Nested<'_> {
                 }
             }
         }
-        if mask.is_none() && slots.is_empty() && declipped.is_none() {
+        if mask.is_none() && slots.is_empty() && declipped.is_none() && reclipped.is_none() {
             return;
         }
+        let reclipped = reclipped.map(|(clip, object)| (Arc::new(self.clipping_path(clip)), object));
         let n = Arc::make_mut(n);
         if let (Some(art), Some(m)) = (mask, n.extras_if_any().and_then(|e| e.mask.as_mut())) {
             m.art = art;
         }
+        // A plain group of just the object that becomes a clip group again.
+        if let Some((path, object)) = reclipped {
+            if let NodeKind::Group { children, clip } = &mut n.kind {
+                (*children, *clip) = (vec![path, object], true);
+            }
+            return;
+        }
         // A clip group that stays as a plain group of just the object.
         if let Some(object) = declipped {
             if let NodeKind::Group { children, clip } = &mut n.kind {
@@ -823,15 +840,18 @@ impl Nested<'_> {
 
     /// A clip group of `object` clipped to `clip`, as [`clip_image_to`] makes one in the layers.
     fn clip_group(&mut self, object: Arc<Node>, clip: PathData) -> Node {
-        let (path_id, group_id) = (NodeId(self.next), NodeId(self.next + 1));
-        self.next += 2;
-        let none = vectorcraft_color::Paint::None;
-        let mut path = Node::path(path_id, clip, Appearance::basic(none.clone(), none, 0.0));
-        if let NodeKind::Path { clipping, .. } = &mut path.kind {
-            *clipping = true;
-        }
+        let path = self.clipping_path(clip);
+        let group_id = NodeId(self.next);
+        self.next += 1;
         Node::new(group_id, NodeKind::Group { children: vec![Arc::new(path), object], clip: true })
     }
+
+    /// A clipping path of `clip`, with the next node id.
+    fn clipping_path(&mut self, clip: PathData) -> Node {
+        let id = NodeId(self.next);
+        self.next += 1;
+        clipping_path(id, clip)
+    }
 }
 
 /// Placed documents `ids` show `src`, read from the file `link` points at, each placed as its
@@ -964,15 +984,41 @@ fn clip_image_to(d: &mut Document, id: NodeId, path: PathData) -> Result<()> {
     new_clip_group(d, id, path)
 }
 
+/// Clip object `id` of `d` to `path`, the clipping path its DCS file draws it through: the clipping
+/// path of the clip group holding just it changes, a plain group holding just it (as [`unclip`]
+/// leaves one) becomes a clip group again, else it goes into a new clip group.
+fn clip_file_to(d: &mut Document, id: NodeId, path: PathData) -> Result<()> {
+    if set_clip_path(d, id, path.clone()) {
+        return Ok(());
+    }
+    let plain = d.parent_of(id).filter(|g| match d.node(*g).map(|n| &n.kind) {
+        Some(NodeKind::Group { children, clip: false }) => matches!(&children[..], [o] if o.id == id),
+        _ => false,
+    });
+    let Some(g) = plain else { return new_clip_group(d, id, path) };
+    let clip = clipping_path(d.alloc_id(), path);
+    d.insert(Some(g), 0, clip)?;
+    if let Some(Node { kind: NodeKind::Group { clip, .. }, .. }) = d.node_mut(g) {
+        *clip = true;
+    }
+    Ok(())
+}
+
+/// A clipping path `id` of `path`, unpainted.
+fn clipping_path(id: NodeId, path: PathData) -> Node {
+    let none = vectorcraft_color::Paint::None;
+    let mut clip = Node::path(id, path, Appearance::basic(none.clone(), none, 0.0));
+    if let NodeKind::Path { clipping, .. } = &mut clip.kind {
+        *clipping = true;
+    }
+    clip
+}
+
 /// Put object `id` of `d` into a new clip group of `path`, where it is.
 fn new_clip_group(d: &mut Document, id: NodeId, path: PathData) -> Result<()> {
     let (par, index, _) = d.position(id).ok_or(EngineError::NoNode(id))?;
     let image = d.remove(id)?;
-    let none = vectorcraft_color::Paint::None;
-    let mut clip = Node::path(d.alloc_id(), path, Appearance::basic(none.clone(), none, 0.0));
-    if let NodeKind::Path { clipping, .. } = &mut clip.kind {
-        *clipping = true;
-    }
+    let clip = clipping_path(d.alloc_id(), path);
     let group = Node::new(d.alloc_id(), NodeKind::Group { children: vec![std::sync::Arc::new(clip), image], clip: true });
     d.insert(par, index, group)?;
     Ok(())
@@ -1037,6 +1083,7 @@ fn install(d: &mut Document, ids: &[NodeId], (link, img): FileRead, force: Optio
     clips.into_iter().try_for_each(|(id, clip)| match clip {
         ClipTo::Keep => Ok(()),
         ClipTo::Path(path) => clip_image_to(d, id, path),
+        ClipTo::File(path) => clip_file_to(d, id, path),
         ClipTo::Nest(path) => new_clip_group(d, id, path),
         ClipTo::Off => unclip(d, id),
     })

crates/engine/src/tests/dcs.rs

@@ -357,6 +357,42 @@ fn a_file_that_lost_its_clipping_path_unclips_the_image() {
     );
 }
 
+#[test]
+fn a_file_that_gains_its_clipping_path_again_clips_the_group_that_stayed() {
+    let dir = temp_dir("update-regained-clip");
+    let path = write(&dir, "pic.eps", &two_plates());
+    let mut s = session();
+    let r = s.execute("file.place", &json!({ "path": path, "at": [100, 100] })).unwrap();
+    let placed = NodeId(r["ids"][0].as_u64().unwrap());
+    let id = image_of(&s, &r["ids"][0]).id;
+    let other = s.execute("file.place", &json!({ "path": path, "at": [200, 100] })).unwrap();
+    s.execute("symbol.new", &json!({ "name": "Pic", "ids": [other["ids"][0]] })).unwrap();
+    let unclipped = String::from_utf8(two_plates()).unwrap().replacen("closepath clip newpath", "closepath newpath     ", 1);
+    // The symbol's art: how deep the image is.
+    let depth = |s: &Session| {
+        fn find(n: &Node, at: usize) -> Option<usize> {
+            if matches!(n.kind, NodeKind::Image(_)) {
+                return Some(at);
+            }
+            n.children().and_then(|ch| ch.iter().find_map(|c| find(c, at + 1)))
+        }
+        find(&s.doc().unwrap().doc.symbols.iter().find(|w| w.name == "Pic").expect("the symbol").art, 0)
+    };
+    let clipped_depth = depth(&s);
+    for _ in 0..2 {
+        std::fs::write(&path, unclipped.as_bytes()).unwrap();
+        s.execute("links.update", &json!({ "ids": [id.0] })).unwrap();
+        std::fs::write(&path, two_plates()).unwrap();
+        s.execute("links.update", &json!({ "ids": [id.0] })).unwrap();
+        let doc = &s.doc().unwrap().doc;
+        assert_eq!(doc.parent_of(id), Some(placed), "the placed group holds the image itself");
+        let NodeKind::Group { children, clip: true } = &doc.node(placed).unwrap().kind else { panic!("a clip group again") };
+        assert!(matches!(&children[..], [c, im] if matches!(c.kind, NodeKind::Path { clipping: true, .. }) && im.id == id));
+        assert_eq!(depth(&s), clipped_depth, "the symbol's image is not nested deeper");
+        assert!(symbol_clip_width(&s, "Pic").is_some(), "the symbol's image is clipped again");
+    }
+}
+
 /// The kind of group `g` is, as (clip, the kinds of its children are just one image).
 fn plain_group_of_image(g: &Node) -> Option<(bool, bool)> {
     match &g.kind {

crates/engine/src/tests/links.rs

@@ -576,6 +576,42 @@ fn update_links_reads_files_again_inside_symbols_patterns_and_masks() {
     assert!(images(&s).iter().all(|(_, _, _, size)| *size == (20, 10)), "{:?}", images(&s));
 }
 
+/// An image that Placement Options clip, alone in a group the user made, goes into a new clip group
+/// inside that group when its file grows, in the layers and in a symbol's art; the user's group
+/// stays a plain group.
+#[test]
+fn placement_options_clip_an_image_alone_in_a_group_inside_that_group() {
+    let dir = Folder::new("clip-in-group");
+    let photo = dir.file("photo.png");
+    write(&photo, &png(20, 10, RED));
+    let mut s = session();
+    let mut groups = vec![];
+    for _ in 0..2 {
+        let id = place(&mut s, &photo);
+        s.execute("links.placementOptions", &json!({"ids": [id.0], "preserve": "fill", "clip": true})).unwrap();
+        let g = NodeId(s.execute("object.group", &json!({"ids": [id.0]})).unwrap()["id"].as_u64().unwrap());
+        groups.push((g, id));
+    }
+    let (in_symbol, _) = groups[1];
+    s.execute("symbol.new", &json!({"name": "Mark", "ids": [in_symbol.0]})).unwrap();
+    write(&photo, &png(60, 20, BLUE));
+    s.execute("links.update", &json!({})).unwrap();
+    // A plain group holding just a clip group of a clipping path and an image.
+    let plain_of_clip = |n: &vectorcraft_doc::Node| match &n.kind {
+        NodeKind::Group { children, clip: false } => matches!(&children[..], [c] if matches!(&c.kind,
+            NodeKind::Group { children, clip: true } if matches!(&children[..], [p, im]
+                if matches!(p.kind, NodeKind::Path { clipping: true, .. }) && matches!(im.kind, NodeKind::Image(_))))),
+        _ => false,
+    };
+    let d = &s.doc().unwrap().doc;
+    let (g, id) = groups[0];
+    assert!(plain_of_clip(d.node(g).unwrap()), "{:?}", d.node(g));
+    assert_ne!(d.parent_of(id), Some(g), "the image is in a new clip group");
+    let mut found = false;
+    d.symbols[0].art.walk(&mut |n| found |= plain_of_clip(n));
+    assert!(found, "{:?}", d.symbols[0].art);
+}
+
 #[test]
 fn a_missing_link_renders_its_preview_and_relinks() {
     let dir = Folder::new("missing");

docs/mcp.md

@@ -6072,7 +6072,7 @@ while such an image that the art shows has a plate of its name; deleting the swa
 file's composite is drawn through clips the image (a clipping group, its path under the nonzero fill rule, an even-odd
 clipping path kept as the same area), and `file.place` answers the image's id as
 `image`, since the Links commands take the image and not the group. `links.update` and `links.relink` clip the image
-by the file's clipping path again (in a new clipping group when none holds it, and inside a clipping mask made in the
+by the file's clipping path again (a plain group holding just the image becomes its clipping group, a new clipping group holds it when no group does, and it goes inside a clipping mask made in the
 document, whose path is kept), unless Placement Options clip the image, the same way for the images in symbols, graph designs, patterns and opacity masks; when the new file has no
 clipping path, a clipping group holding just the image whose path is the old file's clipping path loses that path and
 stays as a plain group of the image, with its id, name, visibility and transparency (one whose path was given the