Merge #1810: Shift Channels: After Effects' Take … From menu order, so 10 is Full Off (#1797) (@Anubhav-Rai)

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();

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]

crates/gpu/src/fx_key.rs

@@ -438,8 +438,10 @@ fn refine(e: &mut Enc, ctx: &EffectCtx, b: GBuf, hard: bool) -> Option<GBuf> {
 
 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<GBuf> {
             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" => {

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] {