Merge pull request #6836 from dotCooCoo/fix/symbol-redefine-drops-undrawn-paints

Symbols: redefining a dynamic symbol drops paints its new objects do not draw

crates/engine/src/cmd/brushsym.rs

@@ -36,7 +36,7 @@ use brushes::{
     brush_apply, brush_delete, brush_duplicate, brush_freehand, brush_get, brush_list, brush_move, brush_new, brush_options, brush_place_art,
     brush_remove, brush_replace_art, brush_selection_art, brush_set_current, brush_unused, expand_brush,
 };
-pub(crate) use dynamic::{edit_part, part_only, selected_part};
+pub(crate) use dynamic::{edit_part, fit_parts, part_only, selected_part};
 use dynamic::{symbol_instance_reset, symbol_instance_select_part, symbol_instance_set_appearance};
 use symbols::selected_instances;
 pub(crate) use symbols::{

crates/engine/src/cmd/brushsym/dynamic.rs

@@ -2,7 +2,7 @@
 //! appearance of objects in the symbol's art and stays linked to the symbol
 //! (`symbol.instance.setAppearance`, `symbol.instance.reset`). The changes live on the instance
 //! ([`vectorcraft_doc::symbol_overrides`]); editing the symbol keeps those whose object is still
-//! there ([`fit_overrides`]).
+//! there and draws them ([`fit_overrides`]).
 
 use std::sync::Arc;
 
@@ -27,7 +27,7 @@ pub(super) fn type_id(dynamic: bool) -> &'static str {
 }
 
 /// Fit the appearance changes of instances `ids` to their symbols: an instance of a static (or
-/// missing) symbol keeps none, an instance of a dynamic one those naming an object of its art.
+/// missing) symbol keeps none, an instance of a dynamic one those its art still draws ([`fit_parts`]).
 pub(super) fn fit_overrides(d: &mut Document, ids: &[NodeId]) {
     for id in ids {
         let art = match d.node(*id) {
@@ -40,7 +40,7 @@ pub(super) fn fit_overrides(d: &mut Document, ids: &[NodeId]) {
         let Some(n) = d.node_mut(*id) else { continue };
         let mut e = n.extras_mut();
         match art {
-            Some(art) => symbol_overrides::retain_parts(&mut e.symbol_overrides, &art),
+            Some(art) => fit_parts(&mut e.symbol_overrides, &art),
             None => e.symbol_overrides.clear(),
         }
     }
@@ -177,17 +177,42 @@ fn check_paint_kinds(p: &Value, art: &Node, path: &[u32], cmd: &str) -> Result<(
 fn solid_fill_check(p: &Value, art: &Node, path: &[u32], fill: Option<&vectorcraft_color::Paint>, cmd: &str) -> Result<()> {
     let asked = p.get("fill").is_some_and(|v| !v.is_null());
     let is_instance = symbol_overrides::part(art, path).is_some_and(|n| matches!(n.kind, NodeKind::SymbolInstance { .. }));
-    let draws_nothing = |f: &vectorcraft_color::Paint| match f {
-        vectorcraft_color::Paint::Gradient(_) => true,
-        vectorcraft_color::Paint::Pattern { .. } => true,
-        vectorcraft_color::Paint::None | vectorcraft_color::Paint::Solid { .. } => false,
-    };
-    if asked && is_instance && fill.is_some_and(draws_nothing) {
+    if asked && is_instance && fill.is_some_and(|f| !stains(f)) {
         return Err(bad(cmd, "fill on a symbol instance must be a solid color or none"));
     }
     Ok(())
 }
 
+/// Can a symbol instance's fill be `f`? An instance stains its art with a solid fill only.
+fn stains(f: &vectorcraft_color::Paint) -> bool {
+    match f {
+        vectorcraft_color::Paint::Gradient(_) => false,
+        vectorcraft_color::Paint::Pattern { .. } => false,
+        vectorcraft_color::Paint::None | vectorcraft_color::Paint::Solid { .. } => true,
+    }
+}
+
+/// Keep the appearance changes `overrides` that name an object of `art` (a symbol's new art), each
+/// without a fill or stroke that object does not draw ([`paint_refusal`], [`stains`]); a change
+/// left with nothing in it is dropped.
+pub(crate) fn fit_parts(overrides: &mut Vec<SymbolOverride>, art: &Node) {
+    symbol_overrides::retain_parts(overrides, art);
+    for o in overrides.iter_mut() {
+        let Some(n) = symbol_overrides::part(art, &o.path) else { continue };
+        let instance = matches!(n.kind, NodeKind::SymbolInstance { .. });
+        if paint_refusal(n, "fill", FIT).is_some() || (instance && o.fill.as_ref().is_some_and(|f| !stains(f))) {
+            o.fill = None;
+        }
+        if paint_refusal(n, "stroke", FIT).is_some() {
+            o.stroke = None;
+        }
+    }
+    overrides.retain(|o| !o.is_empty());
+}
+
+/// The command name [`fit_parts`] gives [`paint_refusal`], whose refusal it does not show.
+const FIT: &str = "symbol.update";
+
 /// `fill` or `stroke` of `p` for override `o`: absent leaves it, null clears it, a colour string or
 /// an object of `paint.setFill`'s paint params sets it.
 fn paint_change(s: &Session, p: &Value, key: &str, cmd: &str) -> Result<Option<Option<vectorcraft_color::Paint>>> {

crates/engine/src/cmd/symboledit.rs

@@ -19,7 +19,7 @@ use vectorcraft_doc::symbol_overrides::SymbolOverride;
 use vectorcraft_doc::{Document, Node, NodeId, NodeKind, Selection, SymbolEdit, SymbolRegistration};
 use vectorcraft_geom::{Affine, Point};
 
-use super::brushsym::{natural_size, place_xf};
+use super::brushsym::{fit_parts, natural_size, place_xf};
 use super::{EngineError, Result, Session, bad, number_or, object_param, ok, str_param};
 
 /// The name of the undo step that enters symbol editing mode.
@@ -425,8 +425,8 @@ fn each_changed_instance(n: &mut Node, name: &str, f: &mut dyn FnMut(&mut Vec<Sy
 }
 
 /// Each instance's appearance changes to objects of symbol `name`'s art follow those objects
-/// through an edit of the editing layer (see [`follow_paths`]), and keep only the changes to
-/// objects the art still has. That covers the instances on the layers and in the art of the
+/// through an edit of the editing layer (see [`follow_paths`]), and keep only the changes the art
+/// still draws ([`fit_parts`]). That covers the instances on the layers and in the art of the
 /// document's other symbols, graph designs, pattern tiles (the tiles pattern editing's Cancel puts
 /// back included) and brushes, opacity-mask art included. The editing layer holds no instance of
 /// the symbol, since [`sync`] refuses one.
@@ -435,7 +435,7 @@ fn follow_objects(d: &mut Document, name: &str, old: (&[Arc<Node>], bool), new:
     let mut follow = |o: &mut Vec<SymbolOverride>| {
         follow_paths(o, old, new);
         match &art {
-            Some(art) => vectorcraft_doc::symbol_overrides::retain_parts(o, art),
+            Some(art) => fit_parts(o, art),
             None => o.clear(),
         }
     };

crates/engine/src/tests/dynamic_symbols.rs

@@ -432,6 +432,30 @@ fn redefining_the_symbol_keeps_the_changes_whose_objects_remain() {
     assert_eq!(overrides(&s, inst), 0);
 }
 
+#[test]
+fn redefining_the_symbol_drops_paints_its_new_objects_do_not_draw() {
+    let mut s = session();
+    let inst = pair(&mut s, "dynamic");
+    let p = json!({"id": inst.0, "path": [1], "fill": "#ff0000", "stroke": "#ff0000", "opacity": 50});
+    s.execute("symbol.instance.setAppearance", &p).unwrap();
+    // The second object is now an image, which draws no fill or stroke of its own.
+    let a = square(&mut s, 10.0);
+    place_image(&mut s);
+    let image = top(&s);
+    s.execute("symbol.update", &json!({"name": "Pair", "ids": [a.0, image.0]})).unwrap();
+    let o = node(&s, inst).extras().symbol_overrides.clone();
+    assert_eq!(o.len(), 1, "the opacity change stays: {o:?}");
+    assert_eq!((&o[0].fill, &o[0].stroke, o[0].opacity), (&None, &None, Some(0.5)));
+    // A change of paint alone goes with its paint: the first object is now a blend.
+    s.execute("symbol.instance.setAppearance", &json!({"id": inst.0, "path": [0], "stroke": "#ff0000"})).unwrap();
+    let (b, c) = (square(&mut s, 10.0), square(&mut s, 60.0));
+    let blend = id(&s.execute("object.blend.make", &json!({"ids": [b.0, c.0], "steps": 1})).unwrap());
+    let d = square(&mut s, 120.0);
+    s.execute("symbol.update", &json!({"name": "Pair", "ids": [blend.0, d.0]})).unwrap();
+    let o = node(&s, inst).extras().symbol_overrides.clone();
+    assert_eq!(o.iter().map(|o| o.path.clone()).collect::<Vec<_>>(), [vec![1]], "{o:?}");
+}
+
 #[test]
 fn replace_symbol_drops_the_changes_made_for_the_old_symbol() {
     let mut s = session();

docs/mcp.md

@@ -2615,8 +2615,9 @@ already walks Image Trace. Short agent entry points:
   locked or sits in a hidden or locked group or layer. Choosing another selection tool selects the instance whole again.
   `symbol.instance.reset {ids?}` drops the selected instances' changes (the Properties
   panel's Reset). Redefining the symbol (`symbol.update`) reshapes every instance and keeps each change whose path the
-  new art still has; `symbol.breakLink` bakes the changes into the expanded art; `symbol.edit` edits the symbol's own
-  art, and each edit there keeps the changes whose path the art still has; making the symbol static drops them, and
+  new art still has, less a fill or stroke the object now at that path does not draw (a change left empty is dropped);
+  `symbol.breakLink` bakes the changes into the expanded art; `symbol.edit` edits the symbol's own art, and each edit
+  there keeps the changes the same way; making the symbol static drops them, and
   so does `symbol.replace` with a different symbol (a replace with the instance's own symbol keeps them).
 
 ```json