Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
32 changes: 31 additions & 1 deletion rust/kcl-lib/src/std/shapes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,10 +539,40 @@ pub async fn ellipse(exec_state: &mut ExecState, args: Args) -> Result<KclValue,
args.get_unlabeled_kw_arg("sketchOrSurface", &RuntimeType::sketch_or_surface(), exec_state)?;
let center = args.get_kw_arg("center", &RuntimeType::point2d(), exec_state)?;
let major_radius = args.get_kw_arg_opt("majorRadius", &RuntimeType::length(), exec_state)?;
let major_diameter: Option<TyF64> = args.get_kw_arg_opt("majorDiameter", &RuntimeType::length(), exec_state)?;
let major_axis = args.get_kw_arg_opt("majorAxis", &RuntimeType::point2d(), exec_state)?;
let minor_radius = args.get_kw_arg("minorRadius", &RuntimeType::length(), exec_state)?;
let minor_radius = args.get_kw_arg_opt("minorRadius", &RuntimeType::length(), exec_state)?;
let minor_diameter: Option<TyF64> = args.get_kw_arg_opt("minorDiameter", &RuntimeType::length(), exec_state)?;
let tag = args.get_kw_arg_opt("tag", &RuntimeType::tag_decl(), exec_state)?;

let major_radius = match (major_radius, major_diameter) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `majorDiameter` or `majorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(major_radius), _) => Some(major_radius),
(_, Some(major_diameter)) => Some(TyF64 {
n: 0.5 * major_diameter.n,
ty: major_diameter.ty,
}),
};
Comment on lines +537 to +549
Copy link
Contributor

Choose a reason for hiding this comment

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

Validation logic issue with optional parameters

There's a validation error in the major_radius matching logic. When both majorRadius and majorDiameter are None, the code returns an error:

(Some(_), Some(_)) | (None, None) => {
    return Err(KclError::new_type(KclErrorDetails::new(
        "Provide either `majorDiameter` or `majorRadius`, not both.".to_string(),
        vec![args.source_range],
    )));
}

However, according to the function signature, majorRadius is optional, and users can provide majorAxis instead. This validation breaks backward compatibility for code that uses majorAxis without specifying either radius parameter.

The validation should only reject cases where both diameter and radius are provided simultaneously, not when both are absent.

Suggested change
let major_radius = match (major_radius, major_diameter) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `majorDiameter` or `majorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(major_radius), _) => Some(major_radius),
(_, Some(major_diameter)) => Some(TyF64 {
n: 0.5 * major_diameter.n,
ty: major_diameter.ty,
}),
};
let major_radius = match (major_radius, major_diameter) {
(Some(_), Some(_)) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `majorDiameter` or `majorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(major_radius), _) => Some(major_radius),
(_, Some(major_diameter)) => Some(TyF64 {
n: 0.5 * major_diameter.n,
ty: major_diameter.ty,
}),
(None, None) => None,
};

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.


let minor_radius = match (minor_radius, minor_diameter) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `minorDiameter` or `minorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(minor_radius), _) => minor_radius,
(_, Some(minor_diameter)) => TyF64 {
n: 0.5 * minor_diameter.n,
ty: minor_diameter.ty,
},
};

let sketch = inner_ellipse(
sketch_or_surface,
center,
Expand Down
68 changes: 64 additions & 4 deletions rust/kcl-lib/src/std/sketch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1921,9 +1921,38 @@ async fn inner_subtract_2d(
pub async fn elliptic_point(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
let x = args.get_kw_arg_opt("x", &RuntimeType::length(), exec_state)?;
let y = args.get_kw_arg_opt("y", &RuntimeType::length(), exec_state)?;
let major_radius = args.get_kw_arg("majorRadius", &RuntimeType::num_any(), exec_state)?;
let minor_radius = args.get_kw_arg("minorRadius", &RuntimeType::num_any(), exec_state)?;
let major_radius = args.get_kw_arg_opt("majorRadius", &RuntimeType::num_any(), exec_state)?;
let major_diameter: Option<TyF64> = args.get_kw_arg_opt("majorDiameter", &RuntimeType::num_any(), exec_state)?;
let minor_radius = args.get_kw_arg_opt("minorRadius", &RuntimeType::num_any(), exec_state)?;
let minor_diameter: Option<TyF64> = args.get_kw_arg_opt("minorDiameter", &RuntimeType::num_any(), exec_state)?;

let major_radius = match (major_radius, major_diameter) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `majorDiameter` or `majorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(major_radius), _) => major_radius,
(_, Some(major_diameter)) => TyF64 {
n: 0.5 * major_diameter.n,
ty: major_diameter.ty,
},
};

let minor_radius = match (minor_radius, minor_diameter) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `minorDiameter` or `minorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(minor_radius), _) => minor_radius,
(_, Some(minor_diameter)) => TyF64 {
n: 0.5 * minor_diameter.n,
ty: minor_diameter.ty,
},
};
let elliptic_point = inner_elliptic_point(x, y, major_radius, minor_radius, &args).await?;

args.make_kcl_val_from_point(elliptic_point, exec_state.length_unit().into())
Expand Down Expand Up @@ -2004,10 +2033,41 @@ pub async fn elliptic(exec_state: &mut ExecState, args: Args) -> Result<KclValue
let angle_start = args.get_kw_arg("angleStart", &RuntimeType::degrees(), exec_state)?;
let angle_end = args.get_kw_arg("angleEnd", &RuntimeType::degrees(), exec_state)?;
let major_radius = args.get_kw_arg_opt("majorRadius", &RuntimeType::length(), exec_state)?;
let major_diameter: Option<TyF64> = args.get_kw_arg_opt("majorDiameter", &RuntimeType::length(), exec_state)?;
let major_axis = args.get_kw_arg_opt("majorAxis", &RuntimeType::point2d(), exec_state)?;
let minor_radius = args.get_kw_arg("minorRadius", &RuntimeType::length(), exec_state)?;
let minor_radius = args.get_kw_arg_opt("minorRadius", &RuntimeType::length(), exec_state)?;
let minor_diameter: Option<TyF64> = args.get_kw_arg_opt("minorDiameter", &RuntimeType::length(), exec_state)?;
let tag = args.get_kw_arg_opt("tag", &RuntimeType::tag_decl(), exec_state)?;

let major_radius = match (major_radius, major_diameter) {
(Some(_), Some(_)) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `majorDiameter` or `majorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(None, None) => None,
(Some(major_radius), _) => Some(major_radius),
(_, Some(major_diameter)) => Some(TyF64 {
n: 0.5 * major_diameter.n,
ty: major_diameter.ty,
}),
};

let minor_radius = match (minor_radius, minor_diameter) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `minorDiameter` or `minorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
(Some(minor_radius), _) => minor_radius,
(_, Some(minor_diameter)) => TyF64 {
n: 0.5 * minor_diameter.n,
ty: minor_diameter.ty,
},
};

let new_sketch = inner_elliptic(
sketch,
center,
Expand Down Expand Up @@ -2047,7 +2107,7 @@ pub(crate) async fn inner_elliptic(
let major_axis = match (major_axis, major_radius) {
(Some(_), Some(_)) | (None, None) => {
return Err(KclError::new_type(KclErrorDetails::new(
"Provide either `majorAxis` or `majorRadius`.".to_string(),
"Provide either `majorAxis` or `majorRadius`, not both.".to_string(),
vec![args.source_range],
)));
}
Expand Down
37 changes: 27 additions & 10 deletions rust/kcl-lib/std/sketch.kcl
Original file line number Diff line number Diff line change
Expand Up @@ -355,17 +355,26 @@ export fn circle(
/// exampleSketch = startSketchOn(XY)
/// |> ellipse(center = [0, 0], majorRadius = 50, minorRadius = 20)
/// ```
///
/// ```
/// exampleSketch = startSketchOn(YZ)
/// |> ellipse(center = [10, 10], majorDiameter = 40, minorRadius = 10)
/// ```
@(impl = std_rust)
export fn ellipse(
/// Sketch to extend, or plane or surface to sketch on.
@sketchOrSurface: Sketch | Plane | Face,
/// The center of the ellipse.
@(snippetArray = ["0", "0"])
center: Point2d,
/// The minor radius of the ellipse.
minorRadius: number(Length),
/// The major radius of the ellipse. Equivalent to majorAxis = [majorRadius, 0].
/// The minor radius of the ellipse. Incompatible with `minorDiameter`.
minorRadius?: number(Length),
/// The minor diameter of the ellipse. Incompatible with `minorRadius`.
minorDiameter?: number(Length),
/// The major radius of the ellipse. Equivalent to majorAxis = [majorRadius, 0]. Incompatible with `majorDiameter`.
majorRadius?: number(Length),
/// The major diameter of the ellipse. Equivalent to 2.0 * majorRadius. Incompatible with `majorRadius`.
majorDiameter?: number(Length),
/// The major axis of the ellipse.
majorAxis?: Point2d,
/// Create a new tag which refers to this ellipse.
Expand Down Expand Up @@ -2230,11 +2239,15 @@ export fn elliptic(
/// Where along the ellptic should this segment end?
@(includeInSnippet = true)
angleEnd: number(Angle),
/// The minor radius, b, of the elliptic equation x^2 / a^2 + y^2 / b^2 = 1.
/// The minor radius, b, of the elliptic equation x^2 / a^2 + y^2 / b^2 = 1. Incompatible with `minorDiameter`.
@(includeInSnippet = true)
minorRadius: number(Length),
/// The major radius, a, of the elliptic equation x^2 / a^2 + y^2 / b^2 = 1. Equivalent to majorAxis = [majorRadius, 0].
minorRadius?: number(Length),
/// The minor diameter of the elliptic equation. Incompatible with `minorRadius`.
minorDiameter?: number(Length),
/// The major radius, a, of the elliptic equation x^2 / a^2 + y^2 / b^2 = 1. Equivalent to majorAxis = [majorRadius, 0]. Incompatible with `majorDiameter`.
majorRadius?: number(Length),
/// The major diameter of the elliptic equation. Incompatible with `majorRadius`.
majorDiameter?: number(Length),
/// The major axis of the elliptic.
@(includeInSnippet = true)
majorAxis?: Point2d,
Expand All @@ -2252,10 +2265,14 @@ export fn elliptic(
///```
@(impl = std_rust)
export fn ellipticPoint(
/// The major radius, a, of the elliptic equation x^2 / a ^ 2 + y^2 / b^2 = 1.
majorRadius: number,
/// The minor radius, b, of the hyperbolic equation x^2 / a ^ 2 + y^2 / b^2 = 1.
minorRadius: number,
/// The major radius, a, of the elliptic equation x^2 / a ^ 2 + y^2 / b^2 = 1. Incompatible with `majorDiameter`.
majorRadius?: number,
/// Them major diameter of the elliptic equation. Incompatible with `majorRadius`.
majorDiameter?: number,
/// The minor radius, b, of the elliptic equation x^2 / a ^ 2 + y^2 / b^2 = 1. Incompatible with `minorDiameter`.
minorRadius?: number,
/// The minor diameter of the elliptic equation. Incompatible with `minorRadius`.
minorDiameter?: number,
/// The x value. Calculates y and returns (x, y). Incompatible with `y`.
x?: number(Length),
/// The y value. Calculates x and returns (x, y). Incompatible with `x`.
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading