diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index 3f9c195c2..58eaa060d 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -243,7 +243,7 @@ fn add_keyframe(s: &mut Session, _: &Value) -> Result { } 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 { 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) })?; diff --git a/crates/engine/src/cmd/prop.rs b/crates/engine/src/cmd/prop.rs index 71ec02996..3c9ce42b8 100644 --- a/crates/engine/src/cmd/prop.rs +++ b/crates/engine/src/cmd/prop.rs @@ -48,7 +48,6 @@ fn with_prop( 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( } } 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 { 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(×) { 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 { fn delete_keys(s: &mut Session, _: &Value) -> Result { 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 { } 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)) diff --git a/crates/engine/src/session/comps.rs b/crates/engine/src/session/comps.rs index ed3810e79..4fda6628c 100644 --- a/crates/engine/src/session/comps.rs +++ b/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 = 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 = 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 diff --git a/crates/engine/src/tests/viewer.rs b/crates/engine/src/tests/viewer.rs index 1ff12237e..ef2bd8acc 100644 --- a/crates/engine/src/tests/viewer.rs +++ b/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"); + } +}