From 358eb317d25ae95d5e5640bbf4b9c617ba2cf66a Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Tue, 15 Sep 2026 19:51:51 -0700 Subject: [PATCH] Add a "Use Tint" toggle to the 'Black & White' node and give its tint the Luminosity blend's clipping (#4540) * Add a 'Use Tint' toggle to the 'Black & White' node and give its tint the Luminosity blend's clipping * Fix the Black & White migration for a wired tint and gray out the tint when unused --- .../document/node_graph/node_properties.rs | 37 +++++++- .../messages/portfolio/document_migration.rs | 16 ++++ node-graph/graph-craft/src/document/value.rs | 14 ++-- node-graph/nodes/raster/src/adjustments.rs | 84 +++++++++++++++---- 4 files changed, 129 insertions(+), 22 deletions(-) diff --git a/editor/src/messages/portfolio/document/node_graph/node_properties.rs b/editor/src/messages/portfolio/document/node_graph/node_properties.rs index 1590a0ae5d..8adc52eb2b 100644 --- a/editor/src/messages/portfolio/document/node_graph/node_properties.rs +++ b/editor/src/messages/portfolio/document/node_graph/node_properties.rs @@ -1075,6 +1075,40 @@ pub fn optional_f64_widget(parameter_widgets_info: ParameterWidgetsInfo, bool_in widgets } +/// `parameter_widgets_info` is for the color parameter. `bool_input_index` is the input index of the bool parameter, drawn as a checkbox in front of the color. +/// A color row gated by the bool input at `bool_input_index`, whose checkbox takes the assist slot after the label like the +/// Opacity node's toggles, so the caller passes `blank_assist = false`. An exposed color shows neither, as in that node. +pub fn optional_color_widget(parameter_widgets_info: ParameterWidgetsInfo, bool_input_index: usize, color_button: ColorInput) -> LayoutGroup { + let node_id = parameter_widgets_info.node_id; + let enabled = parameter_widgets_info + .document_node + .and_then(|document_node| document_node.inputs.get(bool_input_index)) + .and_then(|input| input.as_non_exposed_value()) + .and_then(|value| if let TaggedValue::Bool(enabled) = value { Some(*enabled) } else { None }); + let label_count = start_widgets(¶meter_widgets_info).len(); + let exposed = parameter_widgets_info.is_exposed(); + + let LayoutGroup::Row(mut row) = color_widget(parameter_widgets_info, color_button.disabled(enabled == Some(false))) else { + return LayoutGroup::row(Vec::new()); + }; + if let Some(enabled) = enabled + && !exposed + { + let checkbox = [ + Separator::new(SeparatorStyle::Unrelated).widget_instance(), + Separator::new(SeparatorStyle::Related).widget_instance(), + CheckboxInput::new(enabled) + .on_update(update_value_at_index(|x: &CheckboxInput| TaggedValue::Bool(x.checked), node_id, bool_input_index)) + .on_commit(commit_value) + .widget_instance(), + Separator::new(SeparatorStyle::Related).widget_instance(), + ]; + row.widgets.splice(label_count..label_count, checkbox); + } + + LayoutGroup::Row(row) +} + pub fn number_widget(parameter_widgets_info: ParameterWidgetsInfo, number_props: NumberInput) -> Vec { let mut widgets = start_widgets(¶meter_widgets_info); @@ -2157,7 +2191,8 @@ pub(crate) fn black_and_white_properties(node_id: NodeId, context: &mut NodeProp let number_input = NumberInput::default().mode_increment().unit("%").min(-200.).max(300.); - let tint = color_widget(ParameterWidgetsInfo::new(node_id, TintInput, true, context), ColorInput::default()); + let use_tint: ParameterRef = UseTintInput.into(); + let tint = optional_color_widget(ParameterWidgetsInfo::new(node_id, TintInput, false, context), use_tint.input_index, ColorInput::default()); let mut layout = vec![tint]; let params: &[(ParameterRef, Color, f64)] = &[ diff --git a/editor/src/messages/portfolio/document_migration.rs b/editor/src/messages/portfolio/document_migration.rs index 2635368393..4a58e2743e 100644 --- a/editor/src/messages/portfolio/document_migration.rs +++ b/editor/src/messages/portfolio/document_migration.rs @@ -2266,6 +2266,22 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId], inputs_count = 51; } + // Black & White gained a Use Tint toggle ahead of its tint color; a non-black tint used to be the only way to tint + if reference == DefinitionIdentifier::ProtoNode(graphene_std::raster::black_and_white::IDENTIFIER) && inputs_count == 8 { + let mut node_template = resolve_document_node_type(&reference)?.default_node_template(); + document.network_interface.replace_implementation(node_id, network_path, &mut node_template); + let old_inputs = document.network_interface.replace_inputs(node_id, network_path, &mut node_template)?; + document.network_interface.set_input(&InputConnector::node_at_index(*node_id, 0), old_inputs[0].clone(), network_path); + for (index, input) in old_inputs.iter().enumerate().skip(1).take(7) { + document.network_interface.set_input(&InputConnector::node_at_index(*node_id, index + 1), input.clone(), network_path); + } + let use_tint = !matches!(old_inputs[1].as_value(), Some(TaggedValue::Color(color)) if *color == Color::BLACK); + document + .network_interface + .set_input(&InputConnector::node_at_index(*node_id, 1), NodeInput::value(TaggedValue::Bool(use_tint), false), network_path); + inputs_count = 9; + } + if reference == DefinitionIdentifier::ProtoNode(graphene_std::repeat::repeat_on_points::IDENTIFIER) && inputs_count == 2 { let mut node_template = resolve_document_node_type(&reference)?.default_node_template(); document.network_interface.replace_implementation(node_id, network_path, &mut node_template); diff --git a/node-graph/graph-craft/src/document/value.rs b/node-graph/graph-craft/src/document/value.rs index 215849c8d3..f7db948073 100644 --- a/node-graph/graph-craft/src/document/value.rs +++ b/node-graph/graph-craft/src/document/value.rs @@ -644,9 +644,8 @@ impl TaggedValue { }); } - // Hex syntax (e.g. "000000ff"), which a string literal default reaches here without its quotes - let hex = input.trim().trim_matches('"').trim().trim_start_matches('#'); - let color = SRGBA8::from_hex_str(hex).map(Color::from); + // Hex syntax (e.g. "#1cd1ad", or "#1cd1ad70" with alpha), which a string literal default reaches here without its quotes + let color = input.trim().trim_matches('"').trim().strip_prefix('#').and_then(SRGBA8::from_hex_str).map(Color::from); if color.is_none() { log::error!("Invalid default value color string: {input}"); } @@ -654,7 +653,7 @@ impl TaggedValue { } fn to_gradient(input: &str) -> Option { - // String syntax: (e.g. "000000ff, ff0000ff") + // String syntax: (e.g. "#000000ff, #ff0000ff") let stops = input.split(',').filter_map(|s| to_color(s.trim())).collect::>(); match stops.len() { 0 => { @@ -1039,16 +1038,17 @@ mod paint_default_parsing { ); } - /// A hex string default reaches the parser without the quotes its literal had in the node signature, and must still parse. + /// A hex string default reaches the parser without the quotes its literal had in the node signature, and must carry its hash prefix. #[test] - fn hex_string_color_default_parses_without_quotes() { + fn hex_string_color_default_requires_its_hash_prefix() { let tint = Some(TaggedValue::Color(Color::from(SRGBA8::new(225, 211, 179, 255)))); - assert_eq!(TaggedValue::from_primitive_string("e1d3b3", &item!(Color)), tint, "a bare hex default should resolve"); assert_eq!( TaggedValue::from_primitive_string("\"#e1d3b3\"", &item!(Color)), tint, "a quoted, hash-prefixed hex default should resolve" ); + assert_eq!(TaggedValue::from_primitive_string("#e1d3b3ff", &item!(Color)), tint, "an alpha-suffixed hex default should resolve"); + assert_eq!(TaggedValue::from_primitive_string("e1d3b3", &item!(Color)), None, "a bare hex default should be rejected"); } /// Table-era documents stored the red-slash "no paint" fill as an empty color table, which must keep diff --git a/node-graph/nodes/raster/src/adjustments.rs b/node-graph/nodes/raster/src/adjustments.rs index 36bad6da5a..8f1b57ffc4 100644 --- a/node-graph/nodes/raster/src/adjustments.rs +++ b/node-graph/nodes/raster/src/adjustments.rs @@ -622,7 +622,8 @@ fn black_and_white>( #[implementations(Raster, Color, Gradient)] #[gpu_image] image: Item, - #[default(Color::BLACK)] tint: Item, + use_tint: Item, + #[default("#e1d3b3")] tint: Item, #[default(40.)] #[range] #[soft(-200..300)] @@ -650,6 +651,7 @@ fn black_and_white>( ) -> Item { let mut image = image; let tint = tint.into_element(); + let use_tint = use_tint.into_element(); let reds = reds.into_element(); let yellows = yellows.into_element(); let greens = greens.into_element(); @@ -685,18 +687,17 @@ fn black_and_white>( yellow_part * yellows + (red_part - yellow_part) * reds + (green_part - yellow_part) * greens }; - let luminance = gray_base + additional; + let luminance = (gray_base + additional).clamp(0., 1.); + if !use_tint { + return Color::from_gamma_srgb_channels(luminance, luminance, luminance, alpha_part); + } - // TODO: Fix "Color" blend mode implementation so it matches the expected behavior perfectly (it's currently close) - // Apply luminance substitution in gamma space - let [tr, tg, tb, _] = tint.to_gamma_srgb_channels(); - let tint_luma_rec_601 = 0.3 * tr + 0.59 * tg + 0.11 * tb; - let delta = luminance - tint_luma_rec_601; - let result_r = (tr + delta).clamp(0., 1.); - let result_g = (tg + delta).clamp(0., 1.); - let result_b = (tb + delta).clamp(0., 1.); + // The tint takes on the gray's luminosity the way the Luminosity blend mode would + let [tint_r, tint_g, tint_b, _] = tint.to_gamma_srgb_channels(); + let tint_luma = luma_rec_601_fixed_point(tint_r, tint_g, tint_b); + let [tinted_r, tinted_g, tinted_b] = set_luminosity(tint_r, tint_g, tint_b, tint_luma, luminance); - Color::from_gamma_srgb_channels(result_r, result_g, result_b, alpha_part) + Color::from_gamma_srgb_channels(tinted_r, tinted_g, tinted_b, alpha_part) }); image } @@ -1911,7 +1912,7 @@ fn photo_filter>( #[implementations(Raster, Color, Gradient)] #[gpu_image] image: Item, - #[default("ec8a00")] color: Item, + #[default("#ec8a00")] color: Item, #[default(25.)] density: Item, #[default(true)] preserve_luminosity: Item, ) -> Item { @@ -1937,7 +1938,7 @@ fn photo_filter>( let mut b = linear_to_srgb(filtered[2].clamp(0., 1.)); if preserve_luminosity { - [r, g, b] = set_luminosity(r, g, b, luma_rec_601_fixed(r, g, b), luma_rec_601_fixed(r_in, g_in, b_in)); + [r, g, b] = set_luminosity(r, g, b, luma_rec_601_fixed_point(r, g, b), luma_rec_601_fixed_point(r_in, g_in, b_in)); } Color::from_gamma_srgb_channels(r, g, b, alpha) @@ -1959,7 +1960,7 @@ fn multiply_matrix(matrix: &[[f32; 3]; 3], vector: [f32; 3]) -> [f32; 3] { } /// The Rec. 601 luma in the 14-bit fixed point that PSD interop depends on. -fn luma_rec_601_fixed(r: f32, g: f32, b: f32) -> f32 { +fn luma_rec_601_fixed_point(r: f32, g: f32, b: f32) -> f32 { (4915. * r + 9667. * g + 1802. * b) / 16384. } @@ -2160,6 +2161,61 @@ mod tests { } } + /// Runs Black & White with the default sliders on one gamma-space RGB value (0..255) and returns the gamma-space result on the same scale. + fn run_black_and_white(input: [f32; 3], tint: [f32; 3]) -> [f32; 3] { + let pixel = Color::from_gamma_srgb_channels(input[0] / 255., input[1] / 255., input[2] / 255., 1.); + let tint = Color::from_gamma_srgb_channels(tint[0] / 255., tint[1] / 255., tint[2] / 255., 1.); + let result = black_and_white( + (), + Item::new_from_element(pixel), + true.into(), + tint.into(), + 40_f32.into(), + 60_f32.into(), + 40_f32.into(), + 60_f32.into(), + 20_f32.into(), + 80_f32.into(), + ); + let [r, g, b, _] = result.into_element().to_gamma_srgb_channels(); + [r * 255., g * 255., b * 255.] + } + + #[test] + fn black_and_white_tint_takes_the_grays_luminosity() { + for (input, tint, expected) in [ + ([200., 200., 200.], [225., 211., 179.], [213., 199., 167.]), + ([50., 50., 50.], [225., 211., 179.], [63., 49., 17.]), + ([200., 100., 50.], [225., 211., 179.], [133., 119., 87.]), + ([200., 200., 200.], [30., 60., 120.], [176., 202., 255.]), + ([50., 50., 50.], [30., 60., 120.], [22., 52., 112.]), + ([200., 100., 50.], [30., 60., 120.], [92., 122., 182.]), + ] { + let actual = run_black_and_white(input, tint); + for (actual, expected) in actual.iter().zip(expected) { + assert!((actual - expected).abs() <= 1., "{input:?} tinted {tint:?}: expected {expected}, got {actual}"); + } + } + } + + #[test] + fn black_and_white_clipped_channels_are_pulled_toward_the_luminosity() { + // A pure red tint over grays, where the shifted channels run out of range + for (gray, expected) in [ + (1., [3.33, 0., 0.]), + (38., [126.67, 0., 0.]), + (75., [250.01, 0., 0.]), + (78., [255., 2.15, 2.15]), + (129., [255., 75., 75.]), + (200., [255., 176.43, 176.43]), + ] { + let actual = run_black_and_white([gray, gray, gray], [255., 0., 0.]); + for (actual, expected) in actual.iter().zip(expected) { + assert!((actual - expected).abs() <= 0.51, "gray {gray} tinted red: expected {expected}, got {actual}"); + } + } + } + /// Runs the node on one gamma-space RGB value (0..255) with the master sliders, colorize, and one range's sliders at /// its default range values, returning the gamma-space result on the same scale. fn run_hue_saturation(input: [f32; 3], master: [f32; 3], colorize: Option<[f32; 3]>, range: Option<(HueSaturationRange, [f32; 3])>) -> [f32; 3] {