From 3b9d243ac516c790aa4e7905b6e0cd003426b157 Mon Sep 17 00:00:00 2001 From: Anubhav-Rai Date: Sun, 18 Oct 2026 22:46:28 +0530 Subject: [PATCH] =?UTF-8?q?Shift=20Channels:=20After=20Effects'=20Take=20?= =?UTF-8?q?=E2=80=A6=20From=20menu=20order,=20so=2010=20is=20Full=20Off=20?= =?UTF-8?q?(#1797)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The menu had an extra "Half On" at position 10, where After Effects has "Full Off", so a project or script written for After Effects set a channel to 50 % grey instead of turning it off, with no error: the RGB-split recipe (three copies, each keeping one channel and setting the other two to Full Off, blended with Add) rendered pastel instead of pure red, green and blue. After Effects' ten options now come first, in its order, and "Half On" sits last as an EffectCraft extra, so values 1 to 10 mean what they mean in After Effects and nothing is lost. Instances saved with the old order keep the option they chose: upgrade_instance already remaps popup values by label. The GPU path keeps its own copy of the table, so it is reordered to match; a GPU-vs-CPU parity case now covers the constant sources at the end of the menu, where the two tables could drift apart unnoticed. Both paths clamped the index to a hardcoded 10 and now use SHIFT_ORDER.len() - 1, so the clamp cannot go stale if the list changes again. Co-Authored-By: Claude Opus 5 --- crates/effects/src/channel.rs | 27 ++++++++++++++++++++++++--- crates/effects/src/migrate.rs | 16 ++++++++++++++++ crates/gpu/src/fx_key.rs | 7 +++++-- crates/gpu/src/tests_fx_key.rs | 3 +++ 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/crates/effects/src/channel.rs b/crates/effects/src/channel.rs index b763ac21..44e9dd89 100644 --- a/crates/effects/src/channel.rs +++ b/crates/effects/src/channel.rs @@ -49,12 +49,15 @@ fn set_channels(ctx: &EffectCtx, mut b: Buf) -> Buf { b } +/// Shift Channels' Take … From menu. After Effects' ten options come first, in its order, so a +/// project or script that sets 10 ("Full Off" there) turns the channel off here too; "Half On" is +/// an EffectCraft extra and sits last. const SHIFT_ORDER: [Src; 11] = - [Src::Alpha, Src::Red, Src::Green, Src::Blue, Src::Luminance, Src::Hue, Src::Lightness, Src::Saturation, Src::Full, Src::Half, Src::Off]; -const SHIFT_NAMES: [&str; 11] = ["Alpha", "Red", "Green", "Blue", "Luminance", "Hue", "Lightness", "Saturation", "Full On", "Half On", "Full Off"]; + [Src::Alpha, Src::Red, Src::Green, Src::Blue, Src::Luminance, Src::Hue, Src::Lightness, Src::Saturation, Src::Full, Src::Off, Src::Half]; +const SHIFT_NAMES: [&str; 11] = ["Alpha", "Red", "Green", "Blue", "Luminance", "Hue", "Lightness", "Saturation", "Full On", "Full Off", "Half On"]; fn shift_channels(ctx: &EffectCtx, mut b: Buf) -> Buf { - let s = ["takeRedFrom", "takeGreenFrom", "takeBlueFrom", "takeAlphaFrom"].map(|id| SHIFT_ORDER[(ctx.params.e(id) as usize).min(10)]); + let s = ["takeRedFrom", "takeGreenFrom", "takeBlueFrom", "takeAlphaFrom"].map(|id| SHIFT_ORDER[(ctx.params.e(id) as usize).min(SHIFT_ORDER.len() - 1)]); route(&mut b, s); b } @@ -548,6 +551,24 @@ mod tests { assert!(close(out.get(7, 0), [0.25, 0.25, 0.45, 0.5])); } + /// #1797: the Take … From menu keeps After Effects' order, where the tenth option (`Enum(9)` + /// here, 10 to a script) is Full Off, not Half On. + #[test] + fn shift_channels_menu_follows_after_effects_order() { + assert_eq!(SHIFT_NAMES[8..], ["Full On", "Full Off", "Half On"]); + // The sample's right-hand pixels are half transparent, so a straight 0.0 and 0.5 are + // stored as 0.0 and 0.25. + let off = run("ec.channel.shiftchannels", &[("takeGreenFrom", Value::Enum(9))], sample()); + assert!(close(off.get(0, 0), [0.2, 0.0, 0.9, 1.0]), "{:?}", off.get(0, 0)); + assert!(close(off.get(7, 0), [0.1, 0.0, 0.45, 0.5]), "{:?}", off.get(7, 0)); + let half = run("ec.channel.shiftchannels", &[("takeGreenFrom", Value::Enum(10))], sample()); + assert!(close(half.get(0, 0), [0.2, 0.5, 0.9, 1.0]), "{:?}", half.get(0, 0)); + assert!(close(half.get(7, 0), [0.1, 0.25, 0.45, 0.5]), "{:?}", half.get(7, 0)); + // A value past the last option stays on the last one instead of panicking. + let past = run("ec.channel.shiftchannels", &[("takeGreenFrom", Value::Enum(99))], sample()); + assert_eq!(past.get(0, 0), half.get(0, 0)); + } + #[test] fn channel_combiner_hls_round_trip() { let img = sample(); diff --git a/crates/effects/src/migrate.rs b/crates/effects/src/migrate.rs index 0e037022..b5976b76 100644 --- a/crates/effects/src/migrate.rs +++ b/crates/effects/src/migrate.rs @@ -499,6 +499,22 @@ mod tests { assert_eq!(pr.ui, popup.ui); } + /// #1797: Shift Channels' menu put Half On before Full Off; a project saved then keeps the + /// option it chose, by label. + #[test] + fn shift_channels_half_on_and_full_off_are_remapped_by_label() { + const OLD: [&str; 11] = ["Alpha", "Red", "Green", "Blue", "Luminance", "Hue", "Lightness", "Saturation", "Full On", "Half On", "Full Off"]; + for (saved, want) in [(9u32, 10u32), (10, 9), (1, 1)] { + let mut next = 1; + let (spec, mut g) = instance("ec.channel.shiftchannels", &mut next); + let pr = g.get_mut("takeGreenFrom").unwrap(); + pr.ui = ParamUi::Popup { options: OLD.iter().map(|s| s.to_string()).collect() }; + pr.value = Value::Enum(saved); + upgrade_instance(spec, &mut g, &mut Ids(&mut next), SIZE); + assert_eq!(g.get("takeGreenFrom").unwrap().value, Value::Enum(want), "saved {saved}"); + } + } + /// #514: Apply Color LUT's LUT was hidden (set only by commands) and the OCIO files were /// plain text; old projects get the Choose… file parameter, keeping the path. #[test] diff --git a/crates/gpu/src/fx_key.rs b/crates/gpu/src/fx_key.rs index 1099bccd..3bb5a87a 100644 --- a/crates/gpu/src/fx_key.rs +++ b/crates/gpu/src/fx_key.rs @@ -438,8 +438,10 @@ fn refine(e: &mut Enc, ctx: &EffectCtx, b: GBuf, hard: bool) -> Option { const SET_CHANNEL_LAYERS: [&str; 4] = ["sourceLayer1", "sourceLayer2", "sourceLayer3", "sourceLayer4"]; +/// Shift Channels' Take … From menu, in the order of `effects`' `SHIFT_ORDER`: the two must agree, +/// or the source depends on whether the effect renders on the GPU. const SHIFT_ORDER: [Src; 11] = - [Src::Alpha, Src::Red, Src::Green, Src::Blue, Src::Luminance, Src::Hue, Src::Lightness, Src::Saturation, Src::Full, Src::Half, Src::Off]; + [Src::Alpha, Src::Red, Src::Green, Src::Blue, Src::Luminance, Src::Hue, Src::Lightness, Src::Saturation, Src::Full, Src::Off, Src::Half]; const MATTE_ORDER: [Src; 10] = [Src::Red, Src::Green, Src::Blue, Src::Alpha, Src::Luminance, Src::Hue, Src::Lightness, Src::Saturation, Src::Full, Src::Off]; @@ -527,7 +529,8 @@ fn point(e: &mut Enc, id: &str, ctx: &EffectCtx, b: GBuf) -> Option { p.f[0] = [level, pr.f("softness").clamp(0.0, 100.0) as f32 / 100.0, 0.0, 0.0]; } "ec.channel.shiftchannels" => { - let s = ["takeRedFrom", "takeGreenFrom", "takeBlueFrom", "takeAlphaFrom"].map(|id| src_index(SHIFT_ORDER[(pr.e(id) as usize).min(10)])); + let s = ["takeRedFrom", "takeGreenFrom", "takeBlueFrom", "takeAlphaFrom"] + .map(|id| src_index(SHIFT_ORDER[(pr.e(id) as usize).min(SHIFT_ORDER.len() - 1)])); return route(e, b, s); } "ec.channel.removecolormatting" => { diff --git a/crates/gpu/src/tests_fx_key.rs b/crates/gpu/src/tests_fx_key.rs index 5ece27e6..35303bb0 100644 --- a/crates/gpu/src/tests_fx_key.rs +++ b/crates/gpu/src/tests_fx_key.rs @@ -155,6 +155,9 @@ fn refine_mattes() { fn channel_effects() { effect_case("ec.channel.setchannels", &[("setRedTo", e(2)), ("setGreenTo", e(4)), ("setBlueTo", e(5)), ("setAlphaTo", e(6))]); effect_case("ec.channel.shiftchannels", &[("takeRedFrom", e(3)), ("takeGreenFrom", e(7)), ("takeAlphaFrom", e(4))]); + // #1797: the constant sources at the end of the menu, where the GPU's order could drift from + // the CPU's (Full On, Full Off, Half On). + effect_case("ec.channel.shiftchannels", &[("takeRedFrom", e(8)), ("takeGreenFrom", e(9)), ("takeBlueFrom", e(10))]); effect_case("ec.channel.removecolormatting", &[("backgroundColor", c(0.3, 0.5, 0.7))]); effect_case("ec.channel.removecolormatting", &[("backgroundColor", c(0.8, 0.2, 0.1)), ("clipHdr", off())]); for op in [0, 2, 3, 4, 8, 10, 12] {