Skip to content

Fix min and max macro not enforcing limits when data flows #2464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion node-graph/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
1 change: 1 addition & 0 deletions node-graph/gcore/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion node-graph/gcore/src/raster/adjustments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
9 changes: 5 additions & 4 deletions node-graph/gcore/src/vector/generator_nodes.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::misc::{ArcType, AsU64, GridType};
use super::{PointId, SegmentId, StrokeId};
use crate::Ctx;
use crate::graphene_core::num_traits::FromPrimitive;
use crate::registry::types::Angle;
use crate::vector::{HandleId, VectorData, VectorDataTable};
use bezier_rs::Subpath;
Expand Down Expand Up @@ -97,11 +98,11 @@ fn rectangle<T: CornerRadius>(
}

#[node_macro::node(category("Vector: Shape"))]
fn regular_polygon<T: AsU64>(
fn regular_polygon<T: AsU64 + std::cmp::PartialOrd + FromPrimitive>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TrueDoctor can we avoid needing to include these trait bounds for T here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I described the solution when this was discussed originally, I still might have the partial code for that somewhere. These bounds should basically automatically be added by the macro

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can also add that as a todo and implement that later if there are enough users that not having it becomes annoying

_: impl Ctx,
_primary: (),
#[default(6)]
#[min(3.)]
#[hard_min(3.)]
#[implementations(u32, u64, f64)]
sides: T,
#[default(50)] radius: f64,
Expand All @@ -112,11 +113,11 @@ fn regular_polygon<T: AsU64>(
}

#[node_macro::node(category("Vector: Shape"))]
fn star<T: AsU64>(
fn star<T: AsU64 + std::cmp::PartialOrd + FromPrimitive>(
_: impl Ctx,
_primary: (),
#[default(5)]
#[min(2.)]
#[hard_min(2.)]
#[implementations(u32, u64, f64)]
sides: T,
#[default(50)] radius: f64,
Expand Down
8 changes: 4 additions & 4 deletions node-graph/gcore/src/vector/vector_nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ where
async fn round_corners(
_: impl Ctx,
source: VectorDataTable,
#[min(0.)]
#[hard_min(0.)]
#[default(10.)]
radius: PixelLength,
#[range((0., 1.))]
Expand Down Expand Up @@ -513,7 +513,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();
Expand Down Expand Up @@ -723,7 +723,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();
Expand Down Expand Up @@ -1331,7 +1331,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 {
Expand Down
4 changes: 2 additions & 2 deletions node-graph/gstd/src/image_color_palette.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.)]
#[hard_max(28.)]
max_size: u32,
) -> Vec<Color> {
const GRID: f32 = 3.;
Expand Down
40 changes: 38 additions & 2 deletions node-graph/node-macro/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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! {
let #name = #graphene_core::num_traits::clamp_min(#name, #graphene_core::num_traits::FromPrimitive::from_f64(#min).unwrap());
});
}

if let Some(max) = number_hard_max {
tokens.extend(quote! {
let #name = #graphene_core::num_traits::clamp_max(#name, #graphene_core::num_traits::FromPrimitive::from_f64(#max).unwrap());
});
}
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
Expand Down Expand Up @@ -237,6 +272,7 @@ pub(crate) fn generate_node_code(parsed: &ParsedNodeFn) -> syn::Result<TokenStre
fn eval(&'n self, __input: #input_type) -> Self::Output {
Box::pin(async move {
#(#eval_args)*
#(#min_max_args)*
self::#fn_name(__input #(, #field_names)*) #await_keyword
})
}
Expand Down
75 changes: 52 additions & 23 deletions node-graph/node-macro/src/parsing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,10 @@ pub(crate) enum ParsedField {
ty: Type,
exposed: bool,
value_source: ParsedValueSource,
number_min: Option<LitFloat>,
number_max: Option<LitFloat>,
number_soft_min: Option<LitFloat>,
number_soft_max: Option<LitFloat>,
number_hard_min: Option<LitFloat>,
number_hard_max: Option<LitFloat>,
number_mode_range: Option<ExprTuple>,
implementations: Punctuated<Type, Comma>,
},
Expand Down Expand Up @@ -230,7 +232,7 @@ impl Parse for NodeFnAttributes {
r#"
Unsupported attribute in `node`.
Supported attributes are 'category', 'path' and 'name'.

Example usage:
#[node_macro::node(category("Value"), name("Test Node"))]
"#
Expand Down Expand Up @@ -419,16 +421,29 @@ fn parse_field(pat_ident: PatIdent, ty: Type, attrs: &[Attribute]) -> syn::Resul
_ => ParsedValueSource::None,
};

let number_min = extract_attribute(attrs, "min")
let number_soft_min = extract_attribute(attrs, "soft_min")
.map(|attr| {
attr.parse_args()
.map_err(|e| Error::new_spanned(attr, format!("Invalid numerical `soft_min` value for argument '{}': {}", ident, e)))
})
.transpose()?;
let number_soft_max = extract_attribute(attrs, "soft_max")
.map(|attr| {
attr.parse_args()
.map_err(|e| Error::new_spanned(attr, format!("Invalid numerical `soft_max` value for argument '{}': {}", ident, e)))
})
.transpose()?;

let number_hard_min = extract_attribute(attrs, "hard_min")
.map(|attr| {
attr.parse_args()
.map_err(|e| Error::new_spanned(attr, format!("Invalid numerical `min` value for argument '{}': {}", ident, e)))
.map_err(|e| Error::new_spanned(attr, format!("Invalid numerical `hard_min` value for argument '{}': {}", ident, e)))
})
.transpose()?;
let number_max = extract_attribute(attrs, "max")
let number_hard_max = extract_attribute(attrs, "hard_max")
.map(|attr| {
attr.parse_args()
.map_err(|e| Error::new_spanned(attr, format!("Invalid numerical `max` value for argument '{}': {}", ident, e)))
.map_err(|e| Error::new_spanned(attr, format!("Invalid numerical `hard_max` value for argument '{}': {}", ident, e)))
})
.transpose()?;

Expand Down Expand Up @@ -500,8 +515,10 @@ fn parse_field(pat_ident: PatIdent, ty: Type, attrs: &[Attribute]) -> syn::Resul
description,
widget_override,
exposed,
number_min,
number_max,
number_soft_min,
number_soft_max,
number_hard_min,
number_hard_max,
number_mode_range,
ty,
value_source,
Expand Down Expand Up @@ -716,8 +733,10 @@ mod tests {
ty: parse_quote!(f64),
exposed: false,
value_source: ParsedValueSource::None,
number_min: None,
number_max: None,
number_soft_min: None,
number_soft_max: None,
number_hard_min: None,
number_hard_max: None,
number_mode_range: None,
implementations: Punctuated::new(),
}],
Expand Down Expand Up @@ -781,8 +800,10 @@ mod tests {
ty: parse_quote!(DVec2),
exposed: false,
value_source: ParsedValueSource::None,
number_min: None,
number_max: None,
number_soft_min: None,
number_soft_max: None,
number_hard_min: None,
number_hard_max: None,
number_mode_range: None,
implementations: Punctuated::new(),
},
Expand Down Expand Up @@ -834,8 +855,10 @@ mod tests {
ty: parse_quote!(f64),
exposed: false,
value_source: ParsedValueSource::Default(quote!(50.)),
number_min: None,
number_max: None,
number_soft_min: None,
number_soft_max: None,
number_hard_min: None,
number_hard_max: None,
number_mode_range: None,
implementations: Punctuated::new(),
}],
Expand Down Expand Up @@ -885,8 +908,10 @@ mod tests {
ty: parse_quote!(f64),
exposed: false,
value_source: ParsedValueSource::None,
number_min: None,
number_max: None,
number_soft_min: None,
number_soft_max: None,
number_hard_min: None,
number_hard_max: None,
number_mode_range: None,
implementations: {
let mut p = Punctuated::new();
Expand All @@ -911,8 +936,8 @@ mod tests {
a: f64,
/// b
#[range((0., 100.))]
#[min(-500.)]
#[max(500.)]
#[soft_min(-500.)]
#[soft_max(500.)]
b: f64,
) -> f64 {
a + b
Expand Down Expand Up @@ -948,8 +973,10 @@ mod tests {
ty: parse_quote!(f64),
exposed: false,
value_source: ParsedValueSource::None,
number_min: Some(parse_quote!(-500.)),
number_max: Some(parse_quote!(500.)),
number_soft_min: Some(parse_quote!(-500.)),
number_soft_max: Some(parse_quote!(500.)),
number_hard_min: None,
number_hard_max: None,
number_mode_range: Some(parse_quote!((0., 100.))),
implementations: Punctuated::new(),
}],
Expand Down Expand Up @@ -999,8 +1026,10 @@ mod tests {
widget_override: ParsedWidgetOverride::None,
exposed: true,
value_source: ParsedValueSource::None,
number_min: None,
number_max: None,
number_soft_min: None,
number_soft_max: None,
number_hard_min: None,
number_hard_max: None,
number_mode_range: None,
implementations: Punctuated::new(),
}],
Expand Down
Loading
Loading