Update #[min/max] node macro attributes to #[soft/hard]_[min/max] and make them clamp their input data (#2464)

* Fix min and max macro not enforcing limits when data flows

* Use trait based clamping

* Remove min/max from testing

* cargo fmt

* Resolve into min, and hard_min

* cargo fmt

* fix traits

* cargo fmt

* fix tests

* rename as soft_x

* Add validation code

* Clean up (not compiling because of DVec2 clamping)

* Avoid needing to add trait bounds to node definitions

* Code review

---------

Co-authored-by: Dennis Kobert <[email protected]>
Co-authored-by: Keavon Chambers <[email protected]>

Author: 3 people

node-graph/README.md

@@ -102,7 +102,7 @@ Instead of manually implementing the `Node` trait with complex generics, one can
 
 ```rs
 #[node_macro::node(category("Raster: Adjustments"))]
-fn opacity(_input: (), #[default(424242)] color: Color,#[min(0.1)] opacity_multiplier: f64) -> Color {
+fn opacity(_input: (), #[default(424242)] color: Color, #[soft_min(0.1)] opacity_multiplier: f64) -> Color {
 	let opacity_multiplier = opacity_multiplier as f32 / 100.;
 	Color::from_rgbaf32_unchecked(color.r(), color.g(), color.b(), color.a() * opacity_multiplier)
 }

node-graph/gcore/src/lib.rs

@@ -9,6 +9,7 @@ use core::future::Future;
 #[cfg(feature = "log")]
 extern crate log;
 pub use crate as graphene_core;
+pub use num_traits;
 
 #[cfg(feature = "reflections")]
 pub use ctor;
@@ -19,6 +20,7 @@ pub mod context;
 pub mod generic;
 pub mod instances;
 pub mod logic;
+pub mod misc;
 pub mod ops;
 pub mod structural;
 #[cfg(feature = "std")]

node-graph/gcore/src/misc.rs

@@ -0,0 +1,62 @@
+// TODO(TrueDoctor): Replace this with the more idiomatic approach instead of using `trait Clampable`.
+
+/// A trait for types that can be clamped within a min/max range defined by f64.
+pub trait Clampable: Sized {
+	/// Clamps the value to be no less than `min`.
+	fn clamp_hard_min(self, min: f64) -> Self;
+	/// Clamps the value to be no more than `max`.
+	fn clamp_hard_max(self, max: f64) -> Self;
+}
+
+// Implement for common numeric types
+macro_rules! impl_clampable_float {
+	($($ty:ty),*) => {
+		$(
+			impl Clampable for $ty {
+				#[inline(always)]
+				fn clamp_hard_min(self, min: f64) -> Self {
+					self.max(min as $ty)
+				}
+				#[inline(always)]
+				fn clamp_hard_max(self, max: f64) -> Self {
+					self.min(max as $ty)
+				}
+			}
+		)*
+	};
+}
+impl_clampable_float!(f32, f64);
+
+macro_rules! impl_clampable_int {
+	($($ty:ty),*) => {
+		$(
+			impl Clampable for $ty {
+				#[inline(always)]
+				fn clamp_hard_min(self, min: f64) -> Self {
+					// Using try_from to handle potential range issues safely, though min should ideally be valid.
+					// Consider using a different approach if f64 precision vs integer range is a concern.
+					<$ty>::try_from(min.ceil() as i64).ok().map_or(self, |min_val| self.max(min_val))
+				}
+				#[inline(always)]
+				fn clamp_hard_max(self, max: f64) -> Self {
+					<$ty>::try_from(max.floor() as i64).ok().map_or(self, |max_val| self.min(max_val))
+				}
+			}
+		)*
+	};
+}
+// Add relevant integer types (adjust as needed)
+impl_clampable_int!(u32, u64, i32, i64);
+
+// Implement for DVec2 (component-wise clamping)
+use glam::DVec2;
+impl Clampable for DVec2 {
+	#[inline(always)]
+	fn clamp_hard_min(self, min: f64) -> Self {
+		self.max(DVec2::splat(min))
+	}
+	#[inline(always)]
+	fn clamp_hard_max(self, max: f64) -> Self {
+		self.min(DVec2::splat(max))
+	}
+}

node-graph/gcore/src/raster/adjustments.rs

@@ -1399,7 +1399,7 @@ async fn posterize<T: Adjust<Color>>(
 	)]
 	mut input: T,
 	#[default(4)]
-	#[min(2.)]
+	#[hard_min(2.)]
 	levels: u32,
 ) -> T {
 	input.adjust(|color| {
@@ -1435,6 +1435,7 @@ async fn exposure<T: Adjust<Color>>(
 	offset: f64,
 	#[default(1.)]
 	#[range((0.01, 10.))]
+	#[hard_min(0.0001)]
 	gamma_correction: f64,
 ) -> T {
 	input.adjust(|color| {

node-graph/gcore/src/raster/color.rs

@@ -930,6 +930,8 @@ impl Color {
 
 	#[inline(always)]
 	pub fn gamma(&self, gamma: f32) -> Color {
+		let gamma = gamma.max(0.0001);
+
 		// From https://www.dfstudios.co.uk/articles/programming/image-programming-algorithms/image-processing-algorithms-part-6-gamma-correction/
 		let inverse_gamma = 1. / gamma;
 		self.map_rgb(|c: f32| c.powf(inverse_gamma))

node-graph/gcore/src/vector/generator_nodes.rs

@@ -101,7 +101,7 @@ fn regular_polygon<T: AsU64>(
 	_: impl Ctx,
 	_primary: (),
 	#[default(6)]
-	#[min(3.)]
+	#[hard_min(3.)]
 	#[implementations(u32, u64, f64)]
 	sides: T,
 	#[default(50)] radius: f64,
@@ -116,7 +116,7 @@ fn star<T: AsU64>(
 	_: impl Ctx,
 	_primary: (),
 	#[default(5)]
-	#[min(2.)]
+	#[hard_min(2.)]
 	#[implementations(u32, u64, f64)]
 	sides: T,
 	#[default(50)] radius: f64,
@@ -153,7 +153,7 @@ fn grid<T: GridSpacing>(
 	_: impl Ctx,
 	_primary: (),
 	grid_type: GridType,
-	#[min(0.)]
+	#[hard_min(0.)]
 	#[default(10)]
 	#[implementations(f64, DVec2)]
 	spacing: T,

node-graph/gcore/src/vector/vector_nodes.rs

@@ -429,14 +429,18 @@ where
 async fn round_corners(
 	_: impl Ctx,
 	source: VectorDataTable,
-	#[min(0.)]
+	#[hard_min(0.)]
 	#[default(10.)]
 	radius: PixelLength,
 	#[range((0., 1.))]
+	#[hard_min(0.)]
+	#[hard_max(1.)]
 	#[default(0.5)]
 	roundness: f64,
 	#[default(100.)] edge_length_limit: Percentage,
 	#[range((0., 180.))]
+	#[hard_min(0.)]
+	#[hard_max(180.)]
 	#[default(5.)]
 	min_angle_threshold: Angle,
 ) -> VectorDataTable {
@@ -538,7 +542,7 @@ async fn spatial_merge_by_distance(
 	_: impl Ctx,
 	vector_data: VectorDataTable,
 	#[default(0.1)]
-	#[min(0.0001)]
+	#[hard_min(0.0001)]
 	distance: f64,
 ) -> VectorDataTable {
 	let vector_data_transform = vector_data.transform();
@@ -748,7 +752,7 @@ async fn remove_handles(
 	_: impl Ctx,
 	vector_data: VectorDataTable,
 	#[default(10.)]
-	#[min(0.)]
+	#[soft_min(0.)]
 	max_handle_distance: f64,
 ) -> VectorDataTable {
 	let vector_data_transform = vector_data.transform();
@@ -879,8 +883,8 @@ async fn generate_handles(
 // 	_: impl Ctx,
 // 	source: VectorDataTable,
 // 	#[default(1.)]
-// 	#[min(1.)]
-// 	#[max(8.)]
+// 	#[hard_min(1.)]
+// 	#[soft_max(8.)]
 // 	subdivisions: f64,
 // ) -> VectorDataTable {
 // 	let source_transform = source.transform();
@@ -1367,7 +1371,7 @@ async fn poisson_disk_points(
 	_: impl Ctx,
 	vector_data: VectorDataTable,
 	#[default(10.)]
-	#[min(0.01)]
+	#[hard_min(0.01)]
 	separation_disk_diameter: f64,
 	seed: SeedValue,
 ) -> VectorDataTable {

node-graph/gstd/src/filter.rs

@@ -12,6 +12,7 @@ async fn blur(
 	image_frame: ImageFrameTable<Color>,
 	/// The radius of the blur kernel.
 	#[range((0., 100.))]
+	#[hard_min(0.)]
 	radius: PixelLength,
 	/// Use a lower-quality box kernel instead of a circular Gaussian kernel. This is faster but produces boxy artifacts.
 	box_blur: bool,

node-graph/gstd/src/image_color_palette.rs

@@ -5,8 +5,8 @@ use graphene_core::{Color, Ctx};
 async fn image_color_palette(
 	_: impl Ctx,
 	image: ImageFrameTable<Color>,
-	#[min(1.)]
-	#[max(28.)]
+	#[hard_min(1.)]
+	#[soft_max(28.)]
 	max_size: u32,
 ) -> Vec<Color> {
 	const GRID: f32 = 3.;

node-graph/node-macro/src/codegen.rs

@@ -2,7 +2,7 @@ use crate::parsing::*;
 use convert_case::{Case, Casing};
 use proc_macro_crate::FoundCrate;
 use proc_macro2::TokenStream as TokenStream2;
-use quote::{format_ident, quote};
+use quote::{format_ident, quote, quote_spanned};
 use std::sync::atomic::AtomicU64;
 use syn::punctuated::Punctuated;
 use syn::spanned::Spanned;
@@ -134,14 +134,22 @@ pub(crate) fn generate_node_code(parsed: &ParsedNodeFn) -> syn::Result<TokenStre
 	let number_min_values: Vec<_> = fields
 		.iter()
 		.map(|field| match field {
-			ParsedField::Regular { number_min: Some(number_min), .. } => quote!(Some(#number_min)),
+			ParsedField::Regular { number_soft_min, number_hard_min, .. } => match (number_soft_min, number_hard_min) {
+				(Some(soft_min), _) => quote!(Some(#soft_min)),
+				(None, Some(hard_min)) => quote!(Some(#hard_min)),
+				(None, None) => quote!(None),
+			},
 			_ => quote!(None),
 		})
 		.collect();
 	let number_max_values: Vec<_> = fields
 		.iter()
 		.map(|field| match field {
-			ParsedField::Regular { number_max: Some(number_max), .. } => quote!(Some(#number_max)),
+			ParsedField::Regular { number_soft_max, number_hard_max, .. } => match (number_soft_max, number_hard_max) {
+				(Some(soft_max), _) => quote!(Some(#soft_max)),
+				(None, Some(hard_max)) => quote!(Some(#hard_max)),
+				(None, None) => quote!(None),
+			},
 			_ => quote!(None),
 		})
 		.collect();
@@ -175,6 +183,33 @@ pub(crate) fn generate_node_code(parsed: &ParsedNodeFn) -> syn::Result<TokenStre
 		}
 	});
 
+	let min_max_args = fields.iter().map(|field| match field {
+		ParsedField::Regular {
+			pat_ident,
+			number_hard_min,
+			number_hard_max,
+			..
+		} => {
+			let name = &pat_ident.ident;
+			let mut tokens = quote!();
+			if let Some(min) = number_hard_min {
+				tokens.extend(quote_spanned! {min.span()=>
+					let #name = #graphene_core::misc::Clampable::clamp_hard_min(#name, #min);
+				});
+			}
+
+			if let Some(max) = number_hard_max {
+				tokens.extend(quote_spanned! {max.span()=>
+					let #name = #graphene_core::misc::Clampable::clamp_hard_max(#name, #max);
+				});
+			}
+			tokens
+		}
+		ParsedField::Node { .. } => {
+			quote!()
+		}
+	});
+
 	let all_implementation_types = fields.iter().flat_map(|field| match field {
 		ParsedField::Regular { implementations, .. } => implementations.into_iter().cloned().collect::<Vec<_>>(),
 		ParsedField::Node { implementations, .. } => implementations
@@ -186,13 +221,27 @@ pub(crate) fn generate_node_code(parsed: &ParsedNodeFn) -> syn::Result<TokenStre
 
 	let input_type = &parsed.input.ty;
 	let mut clauses = Vec::new();
+	let mut clampable_clauses = Vec::new();
+
 	for (field, name) in fields.iter().zip(struct_generics.iter()) {
 		clauses.push(match (field, *is_async) {
-			(ParsedField::Regular { ty, .. }, _) => {
+			(
+				ParsedField::Regular {
+					ty, number_hard_min, number_hard_max, ..
+				},
+				_,
+			) => {
 				let all_lifetime_ty = substitute_lifetimes(ty.clone(), "all");
 				let id = future_idents.len();
 				let fut_ident = format_ident!("F{}", id);
 				future_idents.push(fut_ident.clone());
+
+				// Add Clampable bound if this field uses hard_min or hard_max
+				if number_hard_min.is_some() || number_hard_max.is_some() {
+					// The bound applies to the Output type of the future, which is #ty
+					clampable_clauses.push(quote!(#ty: #graphene_core::misc::Clampable));
+				}
+
 				quote!(
 					#fut_ident: core::future::Future<Output = #ty> + #graphene_core::WasmNotSend + 'n,
 					for<'all> #all_lifetime_ty: #graphene_core::WasmNotSend,
@@ -220,6 +269,7 @@ pub(crate) fn generate_node_code(parsed: &ParsedNodeFn) -> syn::Result<TokenStre
 	let mut struct_where_clause = where_clause.clone();
 	let extra_where: Punctuated<WherePredicate, Comma> = parse_quote!(
 		#(#clauses,)*
+		#(#clampable_clauses,)*
 		#output_type: 'n,
 	);
 	struct_where_clause.predicates.extend(extra_where);
@@ -236,7 +286,10 @@ pub(crate) fn generate_node_code(parsed: &ParsedNodeFn) -> syn::Result<TokenStre
 		#[inline]
 		fn eval(&'n self, __input: #input_type) -> Self::Output {
 			Box::pin(async move {
+				use #graphene_core::misc::Clampable;
+
 				#(#eval_args)*
+				#(#min_max_args)*
 				self::#fn_name(__input #(, #field_names)*) #await_keyword
 			})
 		}