Merge pull request #2632 from nanox333/codex/polystar-review-followup

Polystar: synchronize inner controls after expressions and key edits

crates/engine/src/cmd/animation.rs

@@ -243,7 +243,7 @@ fn add_keyframe(s: &mut Session, _: &Value) -> Result<Value> {
     }
     let cid = s.active_comp_id().ok_or(EngineError::NoComp)?;
     let t = s.time();
-    let n = s.edit("Add Keyframe", None, |proj, _| {
+    let n = s.edit("Add Keyframe", None, |proj, state| {
         let mut n = 0;
         for (lid, uid) in &targets {
             let l = layer_mut(proj, cid, *lid)?;
@@ -259,6 +259,9 @@ fn add_keyframe(s: &mut Session, _: &Value) -> Result<Value> {
                 pr.set_value_at(lt, v);
             }
             n += 1;
+            if pr.match_id == "type" {
+                super::prop::sync_polystar_property(proj, state, cid, *lid, *uid, t)?;
+            }
         }
         Ok(n)
     })?;

crates/engine/src/cmd/prop.rs

@@ -48,7 +48,6 @@ fn with_prop<T>(
     let t = s.time_of(cid);
     let linear = s.prefs.general.default_spatial_linear;
     s.edit(label, merge, |proj, state| {
-        let mut next = proj.next_id;
         let l = layer_mut(proj, cid, lid)?;
         let lt = l.layer_time(t);
         let pr = l.props.find_mut(uid).ok_or_else(|| bad("prop", "property vanished"))?;
@@ -64,17 +63,37 @@ fn with_prop<T>(
             }
         }
         if pr.match_id == "type" {
-            let removed = sync_polystar_type(l, uid, lt, &mut next)?;
-            if !removed.is_empty() {
-                state.selected_props.retain(|(layer, prop)| *layer != lid || !removed.contains(prop));
-                state.selected_keys.retain(|key| key.layer != lid || !removed.contains(&key.prop));
-            }
+            sync_polystar_property(proj, state, cid, lid, uid, t)?;
         }
-        proj.next_id = next;
         Ok(r)
     })
 }
 
+/// Keep Polystar controls and their selection references in the property's edit transaction.
+pub(super) fn sync_polystar_property(
+    proj: &mut effectcraft_project::Project,
+    state: &mut crate::EditorState,
+    cid: ItemId,
+    lid: LayerId,
+    uid: Uid,
+    time: Tick,
+) -> Result<()> {
+    let mut next = proj.next_id;
+    let layer = layer_mut(proj, cid, lid)?;
+    let removed = sync_polystar_type(layer, uid, layer.layer_time(time), &mut next)?;
+    if !removed.is_empty() {
+        if state.active_comp == Some(cid) {
+            state.selected_props.retain(|(layer, prop)| *layer != lid || !removed.contains(prop));
+            state.selected_keys.retain(|key| key.layer != lid || !removed.contains(&key.prop));
+        } else if let Some(selection) = state.comp_selections.get_mut(&cid) {
+            selection.props.retain(|(layer, prop)| *layer != lid || !removed.contains(prop));
+            selection.keys.retain(|key| key.layer != lid || !removed.contains(&key.prop));
+        }
+    }
+    proj.next_id = next;
+    Ok(())
+}
+
 /// A static Polygon has no inner controls; switching to Star creates them in schema order
 /// with AE's switch defaults (#1685). Animated/expression-driven types need the controls at
 /// every time, including when the current value is Polygon.
@@ -664,12 +683,21 @@ fn set_expr(s: &mut Session, p: &Value) -> Result<Value> {
             s.project.comp(*cid).and_then(|c| c.layer(*lid).and_then(|l| super::link::reference(c, l, l, *uid, true))).unwrap_or_else(|| "value".into())
         })
         .collect();
-    let results = s.edit("Expression", merge_p(p), |proj, _| {
+    let times: Vec<_> = targets.iter().map(|(cid, _, _)| s.time_of(*cid)).collect();
+    let results = s.edit("Expression", merge_p(p), |proj, state| {
         let mut out = vec![];
-        for ((cid, lid, uid), def) in targets.iter().zip(&defs) {
+        let mut changed_types = vec![];
+        for (((cid, lid, uid), def), time) in targets.iter().zip(&defs).zip(&times) {
             let pr = layer_mut(proj, *cid, *lid)?.props.find_mut(*uid).ok_or_else(|| bad(C, "property vanished"))?;
             apply_expr(pr, &text, enabled, def);
             out.push(json!(pr.expr.as_ref().map(|e| e.text.clone())));
+            if pr.match_id == "type" {
+                changed_types.push((*cid, *lid, *uid, *time));
+            }
+        }
+        // All selected targets must be edited before a Type change can remove inner controls.
+        for (cid, lid, uid, time) in changed_types {
+            sync_polystar_property(proj, state, cid, lid, uid, time)?;
         }
         Ok(out)
     })?;
@@ -1045,9 +1073,11 @@ fn move_keys(s: &mut Session, p: &Value) -> Result<Value> {
 fn delete_keys(s: &mut Session, _: &Value) -> Result<Value> {
     let cid = s.active_comp_id().ok_or(EngineError::NoComp)?;
     let sel = s.state.selected_keys.clone();
+    let time = s.time();
     s.edit("Delete Keyframes", None, |proj, st| {
         let comp = proj.comp_mut(cid).ok_or(EngineError::NoComp)?;
         let mut removed = 0usize;
+        let mut changed = std::collections::BTreeSet::new();
         for k in &sel {
             if let Some(pr) = comp.layer_mut(k.layer).and_then(|l| l.props.find_mut(k.prop))
                 && let Some(i) = key_at(&pr.keys, k.time)
@@ -1057,9 +1087,15 @@ fn delete_keys(s: &mut Session, _: &Value) -> Result<Value> {
                 }
                 pr.keys.remove(i);
                 removed += 1;
+                if pr.match_id == "type" {
+                    changed.insert((k.layer, k.prop));
+                }
             }
         }
         st.selected_keys.clear();
+        for (layer, prop) in changed {
+            sync_polystar_property(proj, st, cid, layer, prop, time)?;
+        }
         // The keys actually removed: a selected key that is gone (or listed twice) isn't counted,
         // so a caller can tell that a delete changed nothing.
         Ok(json!(removed))

crates/engine/src/session/comps.rs

@@ -28,6 +28,9 @@ impl Session {
 
     /// Drop selections that no longer exist. Each comp keeps its own selection (#1001); a layer id
     /// counts only inside the comp that owns it, so the active comp never sees another comp's layers.
+    /// Validate property UIDs too: undo can remove a control and rewind the allocator, so keeping
+    /// its selection could silently select a different property when the UID is reused. Group
+    /// selections remain valid even though groups are not leaf properties.
     pub fn sanitize_state(&mut self) {
         let p = self.project.clone();
         self.state.open_comps.retain(|c| p.comp(*c).is_some());
@@ -35,8 +38,8 @@ impl Session {
             let Some(c) = p.comp(*cid) else { return false };
             let ids: Vec<LayerId> = c.layers.iter().map(|l| l.id).collect();
             sel.layers.retain(|l| ids.contains(l));
-            sel.props.retain(|(l, _)| ids.contains(l));
-            sel.keys.retain(|k| ids.contains(&k.layer));
+            sel.props.retain(|(lid, uid)| c.layer(*lid).is_some_and(|l| l.props.find(*uid).is_some() || l.props.find_group(*uid).is_some()));
+            sel.keys.retain(|k| c.layer(k.layer).is_some_and(|l| l.props.find(k.prop).is_some()));
             sel.vertices.retain(|v| ids.contains(&v.layer));
             !sel.is_empty()
         });
@@ -49,11 +52,11 @@ impl Session {
                 self.recall_selection(id);
             }
         }
-        if let Some(c) = self.active_comp() {
+        if let Some(c) = self.state.active_comp.and_then(|cid| p.comp(cid)) {
             let ids: Vec<LayerId> = c.layers.iter().map(|l| l.id).collect();
             self.state.selected_layers.retain(|l| ids.contains(l));
-            self.state.selected_props.retain(|(l, _)| ids.contains(l));
-            self.state.selected_keys.retain(|k| ids.contains(&k.layer));
+            self.state.selected_props.retain(|(lid, uid)| c.layer(*lid).is_some_and(|l| l.props.find(*uid).is_some() || l.props.find_group(*uid).is_some()));
+            self.state.selected_keys.retain(|k| c.layer(k.layer).is_some_and(|l| l.props.find(k.prop).is_some()));
             self.state.selected_vertices.retain(|v| ids.contains(&v.layer));
         }
         if let Some((l, t)) = self.state.current_track

crates/engine/src/tests/viewer.rs

@@ -697,3 +697,166 @@ fn polystar_animated_type_keeps_the_inner_controls_needed_by_other_frames() {
     assert_eq!(star.get("type").unwrap().value_at(Tick::from_seconds_f64(1.0)), KV::Enum(1));
     assert_eq!(star.get("innerRadius").unwrap().value, KV::Scalar(27.0));
 }
+
+#[test]
+fn polystar_expression_routes_synchronize_controls_in_the_same_undo_step() {
+    for explicit in [false, true] {
+        let mut s = comp();
+        let lid = s.execute("layer.newShape", json!({"kind": "polygon"})).unwrap()["layer"].as_u64().unwrap();
+        let group = |s: &Session| layer(s, lid).props.group("contents/group/contents/star").unwrap().clone();
+        let before = group(&s);
+        let uid = before.get("type").unwrap().uid;
+        s.state.selected_props = vec![(LayerId(lid), uid)];
+        let params = if explicit { json!({"layer": lid, "prop": uid, "expression": "1"}) } else { json!({"expression": "1"}) };
+        s.execute("prop.setExpression", params).unwrap();
+        let animated = group(&s);
+        assert!(animated.get("innerRadius").is_some() && animated.get("innerRoundness").is_some());
+        assert!(s.undo());
+        assert_eq!(group(&s), before);
+        assert!(s.redo());
+        assert_eq!(group(&s), animated);
+        let inner = animated.get("innerRadius").unwrap().uid;
+        s.state.selected_props = vec![(LayerId(lid), inner)];
+        s.execute("prop.setExpression", json!({"layer": lid, "prop": uid, "expression": ""})).unwrap();
+        assert!(group(&s).get("innerRadius").is_none());
+        assert!(s.state.selected_props.is_empty());
+        assert!(s.undo());
+        assert_eq!(group(&s), animated);
+        assert!(s.redo());
+        assert!(group(&s).get("innerRadius").is_none());
+    }
+}
+
+#[test]
+fn polystar_animation_menu_adds_inner_controls_and_undoes() {
+    let mut s = comp();
+    let lid = s.execute("layer.newShape", json!({"kind": "polygon"})).unwrap()["layer"].as_u64().unwrap();
+    let group = |s: &Session| layer(s, lid).props.group("contents/group/contents/star").unwrap().clone();
+    let before = group(&s);
+    s.state.selected_props = vec![(LayerId(lid), before.get("type").unwrap().uid)];
+    s.execute("anim.addKeyframe", json!({})).unwrap();
+    let animated = group(&s);
+    assert_eq!(animated.get("type").unwrap().keys.len(), 1);
+    assert!(animated.get("innerRadius").is_some() && animated.get("innerRoundness").is_some());
+    assert!(s.undo());
+    assert_eq!(group(&s), before);
+    assert!(s.redo());
+    assert_eq!(group(&s), animated);
+}
+
+#[test]
+fn deleting_the_last_polystar_type_key_removes_stale_controls_and_selection() {
+    for command in ["keys.delete", "edit.clear"] {
+        let mut s = comp();
+        let lid = s.execute("layer.newShape", json!({"kind": "polygon"})).unwrap()["layer"].as_u64().unwrap();
+        let group = |s: &Session| layer(s, lid).props.group("contents/group/contents/star").unwrap().clone();
+        let type_uid = group(&s).get("type").unwrap().uid;
+        s.execute("prop.addKey", json!({"layer": lid, "prop": type_uid, "time": 0, "value": 1})).unwrap();
+        let animated = group(&s);
+        let inner = animated.get("innerRadius").unwrap().uid;
+        s.state.selected_props = vec![(LayerId(lid), inner)];
+        s.state.selected_keys = vec![crate::KeyRef { layer: LayerId(lid), prop: type_uid, time: Tick::ZERO }];
+        s.execute(command, json!({})).unwrap();
+        let polygon = group(&s);
+        assert!(polygon.get("innerRadius").is_none() && polygon.get("innerRoundness").is_none());
+        assert!(s.state.selected_props.is_empty() && s.state.selected_keys.is_empty());
+        assert!(s.undo());
+        assert_eq!(group(&s), animated);
+        assert!(s.redo());
+        assert_eq!(group(&s), polygon);
+    }
+}
+
+#[test]
+fn polystar_sync_cleans_the_edited_compositions_parked_selection() {
+    let mut s = comp();
+    let first = s.active_comp_id().unwrap();
+    let lid = s.execute("layer.newShape", json!({"kind": "polygon"})).unwrap()["layer"].as_u64().unwrap();
+    let path = "contents/group/contents/star";
+    let type_uid = layer(&s, lid).props.group(path).unwrap().get("type").unwrap().uid;
+    s.execute("prop.addKey", json!({"layer": lid, "prop": type_uid, "time": 0, "value": 1})).unwrap();
+    let before = layer(&s, lid).props.group(path).unwrap().clone();
+    let inner = before.get("innerRadius").unwrap().uid;
+    s.state.selected_props = vec![(LayerId(lid), inner)];
+    s.state.selected_keys = vec![crate::KeyRef { layer: LayerId(lid), prop: inner, time: Tick::ZERO }];
+    s.execute("comp.new", json!({"width": 100, "height": 100, "duration": 2})).unwrap();
+    let second = s.active_comp_id().unwrap();
+    let other = s.execute("layer.newSolid", json!({})).unwrap()["layer"].as_u64().unwrap();
+    let other_prop = layer(&s, other).props.prop("transform/rotation").unwrap().uid;
+    s.execute("prop.addKey", json!({"layer": other, "prop": other_prop, "time": 0, "value": 15})).unwrap();
+    s.state.selected_props = vec![(LayerId(other), other_prop)];
+    s.state.selected_keys = vec![crate::KeyRef { layer: LayerId(other), prop: other_prop, time: Tick::ZERO }];
+    let live = (s.state.selected_props.clone(), s.state.selected_keys.clone());
+    s.execute("prop.toggleAnimation", json!({"comp": first.0, "layer": lid, "prop": type_uid, "value": false})).unwrap();
+    assert_eq!(s.active_comp_id(), Some(second));
+    assert_eq!((s.state.selected_props.clone(), s.state.selected_keys.clone()), live);
+    let parked = s.state.comp_selections.get(&first).unwrap();
+    assert!(parked.props.is_empty() && parked.keys.is_empty(), "the inactive composition must not keep removed control references");
+    let after = s.project.comp(first).unwrap().layer(LayerId(lid)).unwrap().props.group(path).unwrap().clone();
+    assert!(after.get("innerRadius").is_none());
+    assert!(s.undo());
+    assert_eq!(s.project.comp(first).unwrap().layer(LayerId(lid)).unwrap().props.group(path).unwrap(), &before);
+    assert!(s.redo());
+    assert_eq!(s.project.comp(first).unwrap().layer(LayerId(lid)).unwrap().props.group(path).unwrap(), &after);
+}
+
+#[test]
+fn polystar_multi_property_expression_clear_resolves_targets_before_removing_controls() {
+    let mut s = comp();
+    let lid = s.execute("layer.newShape", json!({"kind": "polygon"})).unwrap()["layer"].as_u64().unwrap();
+    let group = |s: &Session| layer(s, lid).props.group("contents/group/contents/star").unwrap().clone();
+    let type_uid = group(&s).get("type").unwrap().uid;
+    s.execute("prop.setExpression", json!({"layer": lid, "prop": type_uid, "expression": "1"})).unwrap();
+    let inner = group(&s).get("innerRadius").unwrap().uid;
+    s.execute("prop.setExpression", json!({"layer": lid, "prop": inner, "expression": "37"})).unwrap();
+    let before = group(&s);
+    s.state.selected_props = vec![(LayerId(lid), type_uid), (LayerId(lid), inner)];
+    assert_eq!(s.execute("prop.setExpression", json!({"expression": ""})).unwrap(), json!([null, null]));
+    let after = group(&s);
+    assert!(after.get("type").unwrap().expr.is_none());
+    assert!(after.get("innerRadius").is_none());
+    assert_eq!(s.state.selected_props, vec![(LayerId(lid), type_uid)]);
+    assert!(s.undo());
+    assert_eq!(group(&s), before);
+    assert!(s.redo());
+    assert_eq!(group(&s), after);
+}
+
+/// Undo must discard removed controls before their UIDs can name newly allocated properties.
+#[test]
+fn polystar_undo_sanitizes_live_and_parked_property_selections_before_uid_reuse() {
+    for parked in [false, true] {
+        let mut s = comp();
+        let cid = s.active_comp_id().unwrap();
+        let lid = s.execute("layer.newShape", json!({"kind": "polygon"})).unwrap()["layer"].as_u64().unwrap();
+        let polygon = layer(&s, lid).props.group("contents/group/contents/star").unwrap().clone();
+        let type_uid = polygon.get("type").unwrap().uid;
+        s.execute("comp.new", json!({"width": 100, "height": 100, "duration": 2})).unwrap();
+        let other = s.active_comp_id().unwrap();
+        s.activate_comp(cid);
+        s.state.selected_props = vec![(LayerId(lid), type_uid)];
+        s.execute("anim.addKeyframe", json!({})).unwrap();
+        let inner = layer(&s, lid).props.group("contents/group/contents/star").unwrap().get("innerRadius").unwrap().uid;
+        let valid = vec![(LayerId(lid), type_uid), (LayerId(lid), polygon.uid)];
+        s.state.selected_props = valid.iter().copied().chain([(LayerId(lid), inner)]).collect();
+        s.state.selected_keys = vec![crate::KeyRef { layer: LayerId(lid), prop: inner, time: Tick::ZERO }];
+        if parked {
+            s.activate_comp(other);
+        }
+        assert!(s.undo());
+        let (props, keys) = if parked {
+            let selection = s.state.comp_selections.get(&cid).unwrap();
+            (&selection.props, &selection.keys)
+        } else {
+            (&s.state.selected_props, &s.state.selected_keys)
+        };
+        assert_eq!(props, &valid, "remove stale leaves while preserving property and group selections");
+        assert!(keys.is_empty());
+        s.activate_comp(cid);
+        s.execute("prop.separateDimensions", json!({"layer": lid, "value": true})).unwrap();
+        let position_z = layer(&s, lid).props.prop("transform/positionZ").unwrap().clone();
+        assert_eq!(position_z.uid, inner, "the removed control's UID was actually reused");
+        s.execute("prop.setExpression", json!({"expression": "1"})).unwrap();
+        assert_eq!(layer(&s, lid).props.prop("transform/positionZ").unwrap(), &position_z, "the reused UID must not become an implicit expression target");
+    }
+}