ByoungSooPark/modeling-app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

lint default planes and add a suggestion (#6587)

Browse Source 
lint default planes and other plane cleanup

Signed-off-by: Jess Frazelle <github@jessfraz.com>
This commit is contained in:
Jess Frazelle
2025-04-29 19:11:02 -07:00
committed by GitHub
parent bf63b21d74
commit 9c29756a38
123 changed files with 14097 additions and 13997 deletions
Download Patch File Download Diff File Expand all files Collapse all files

164
rust/kcl-lib/src/engine/mod.rs
View File

@ -33,6 +33,7 @@ use kcmc::{
ModelingCmd,
};
use kittycad_modeling_cmds as kcmc;
use parse_display::{Display, FromStr};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use tokio::sync::RwLock;
@ -42,7 +43,7 @@ use uuid::Uuid;
use crate::execution::ArtifactCommand;
use crate::{
errors::{KclError, KclErrorDetails},
execution::{types::UnitLen, DefaultPlanes, IdGenerator, Point3d},
execution::{types::UnitLen, DefaultPlanes, IdGenerator, PlaneInfo, Point3d},
SourceRange,
};
@ -50,6 +51,40 @@ lazy_static::lazy_static! {
pub static ref GRID_OBJECT_ID: uuid::Uuid = uuid::Uuid::parse_str("cfa78409-653d-4c26-96f1-7c45fb784840").unwrap();
pub static ref GRID_SCALE_TEXT_OBJECT_ID: uuid::Uuid = uuid::Uuid::parse_str("10782f33-f588-4668-8bcd-040502d26590").unwrap();
pub static ref DEFAULT_PLANE_INFO: IndexMap<PlaneName, PlaneInfo> = IndexMap::from([
(PlaneName::Xy,PlaneInfo{
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 1.0, 0.0, UnitLen::Unknown),
}),
(PlaneName::NegXy,
PlaneInfo{
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(-1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 1.0, 0.0, UnitLen::Unknown),
}),
(PlaneName::Xz, PlaneInfo{
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
}),
(PlaneName::NegXz, PlaneInfo{
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(-1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
}),
(PlaneName::Yz, PlaneInfo{
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(0.0, 1.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
}),
(PlaneName::NegYz, PlaneInfo{
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(0.0, -1.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
}),
]);
}
#[derive(Default, Debug)]
@ -608,31 +643,23 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
async fn make_default_plane(
&self,
plane_id: uuid::Uuid,
x_axis: Point3d,
y_axis: Point3d,
info: &PlaneInfo,
color: Option<Color>,
source_range: SourceRange,
id_generator: &mut IdGenerator,
) -> Result<uuid::Uuid, KclError> {
// Create new default planes.
let default_size = 100.0;
let default_origin = Point3d {
x: 0.0,
y: 0.0,
z: 0.0,
units: UnitLen::Mm,
}
.into();
self.batch_modeling_cmd(
plane_id,
source_range,
&ModelingCmd::from(mcmd::MakePlane {
clobber: false,
origin: default_origin,
origin: info.origin.into(),
size: LengthUnit(default_size),
x_axis: x_axis.into(),
y_axis: y_axis.into(),
x_axis: info.x_axis.into(),
y_axis: info.y_axis.into(),
hide: Some(true),
}),
)
@ -656,22 +683,10 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
id_generator: &mut IdGenerator,
source_range: SourceRange,
) -> Result<DefaultPlanes, KclError> {
let plane_settings: Vec<(PlaneName, Uuid, Point3d, Point3d, Option<Color>)> = vec![
let plane_settings: Vec<(PlaneName, Uuid, Option<Color>)> = vec![
(
PlaneName::Xy,
id_generator.next_uuid(),
Point3d {
x: 1.0,
y: 0.0,
z: 0.0,
units: UnitLen::Mm,
},
Point3d {
x: 0.0,
y: 1.0,
z: 0.0,
units: UnitLen::Mm,
},
Some(Color {
r: 0.7,
g: 0.28,
@ -682,18 +697,6 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
(
PlaneName::Yz,
id_generator.next_uuid(),
Point3d {
x: 0.0,
y: 1.0,
z: 0.,
units: UnitLen::Mm,
},
Point3d {
x: 0.0,
y: 0.0,
z: 1.0,
units: UnitLen::Mm,
},
Some(Color {
r: 0.28,
g: 0.7,
@ -704,18 +707,6 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
(
PlaneName::Xz,
id_generator.next_uuid(),
Point3d {
x: 1.0,
y: 0.0,
z: 0.0,
units: UnitLen::Mm,
},
Point3d {
x: 0.0,
y: 0.0,
z: 1.0,
units: UnitLen::Mm,
},
Some(Color {
r: 0.28,
g: 0.28,
@ -723,64 +714,23 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
a: 0.4,
}),
),
(
PlaneName::NegXy,
id_generator.next_uuid(),
Point3d {
x: -1.0,
y: 0.0,
z: 0.0,
units: UnitLen::Mm,
},
Point3d {
x: 0.0,
y: 1.0,
z: 0.0,
units: UnitLen::Mm,
},
None,
),
(
PlaneName::NegYz,
id_generator.next_uuid(),
Point3d {
x: 0.0,
y: -1.0,
z: 0.0,
units: UnitLen::Mm,
},
Point3d {
x: 0.0,
y: 0.0,
z: 1.0,
units: UnitLen::Mm,
},
None,
),
(
PlaneName::NegXz,
id_generator.next_uuid(),
Point3d {
x: -1.0,
y: 0.0,
z: 0.0,
units: UnitLen::Mm,
},
Point3d {
x: 0.0,
y: 0.0,
z: 1.0,
units: UnitLen::Mm,
},
None,
),
(PlaneName::NegXy, id_generator.next_uuid(), None),
(PlaneName::NegYz, id_generator.next_uuid(), None),
(PlaneName::NegXz, id_generator.next_uuid(), None),
];
let mut planes = HashMap::new();
for (name, plane_id, x_axis, y_axis, color) in plane_settings {
for (name, plane_id, color) in plane_settings {
let info = DEFAULT_PLANE_INFO.get(&name).ok_or_else(|| {
// We should never get here.
KclError::Engine(KclErrorDetails {
message: format!("Failed to get default plane info for: {:?}", name),
source_ranges: vec![source_range],
})
})?;
planes.insert(
name,
self.make_default_plane(plane_id, x_axis, y_axis, color, source_range, id_generator)
self.make_default_plane(plane_id, info, color, source_range, id_generator)
.await?,
);
}
@ -907,21 +857,27 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
async fn close(&self);
}
#[derive(Debug, Hash, Eq, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema)]
#[derive(Debug, Hash, Eq, Copy, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema, Display, FromStr)]
#[ts(export)]
#[serde(rename_all = "camelCase")]
pub enum PlaneName {
/// The XY plane.
#[display("XY")]
Xy,
/// The opposite side of the XY plane.
#[display("-XY")]
NegXy,
/// The XZ plane.
#[display("XZ")]
Xz,
/// The opposite side of the XZ plane.
#[display("-XZ")]
NegXz,
/// The YZ plane.
#[display("YZ")]
Yz,
/// The opposite side of the YZ plane.
#[display("-YZ")]
NegYz,
}

18
rust/kcl-lib/src/execution/exec_ast.rs
View File

@ -1168,14 +1168,14 @@ impl Node<UnaryExpression> {
}
KclValue::Plane { value } => {
let mut plane = value.clone();
if plane.x_axis.x != 0.0 {
plane.x_axis.x *= -1.0;
if plane.info.x_axis.x != 0.0 {
plane.info.x_axis.x *= -1.0;
}
if plane.x_axis.y != 0.0 {
plane.x_axis.y *= -1.0;
if plane.info.x_axis.y != 0.0 {
plane.info.x_axis.y *= -1.0;
}
if plane.x_axis.z != 0.0 {
plane.x_axis.z *= -1.0;
if plane.info.x_axis.z != 0.0 {
plane.info.x_axis.z *= -1.0;
}
plane.value = PlaneType::Uninit;
@ -2694,9 +2694,9 @@ p2 = -p
.unwrap()
{
KclValue::Plane { value } => {
assert_eq!(value.x_axis.x, -1.0);
assert_eq!(value.x_axis.y, 0.0);
assert_eq!(value.x_axis.z, 0.0);
assert_eq!(value.info.x_axis.x, -1.0);
assert_eq!(value.info.x_axis.y, 0.0);
assert_eq!(value.info.x_axis.z, 0.0);
}
_ => unreachable!(),
}

166
rust/kcl-lib/src/execution/geometry.rs
View File

@ -8,6 +8,8 @@ use parse_display::{Display, FromStr};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use crate::engine::{PlaneName, DEFAULT_PLANE_INFO};
use crate::errors::KclErrorDetails;
#[cfg(feature = "artifact-graph")]
use crate::execution::ArtifactId;
use crate::{
@ -281,17 +283,26 @@ pub struct Plane {
pub artifact_id: ArtifactId,
// The code for the plane either a string or custom.
pub value: PlaneType,
/// The information for the plane.
#[serde(flatten)]
pub info: PlaneInfo,
#[serde(skip)]
pub meta: Vec<Metadata>,
}
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, ts_rs::TS, JsonSchema)]
#[ts(export)]
#[serde(rename_all = "camelCase")]
pub struct PlaneInfo {
/// Origin of the plane.
pub origin: Point3d,
/// What should the plane's X axis be?
pub x_axis: Point3d,
/// What should the plane's Y axis be?
pub y_axis: Point3d,
#[serde(skip)]
pub meta: Vec<Metadata>,
}
impl Plane {
impl PlaneInfo {
pub(crate) fn into_plane_data(self) -> PlaneData {
if self.origin.is_zero() {
match self {
@ -317,7 +328,6 @@ impl Plane {
z: 0.0,
units: _,
},
..
} => return PlaneData::XY,
Self {
origin:
@ -341,7 +351,6 @@ impl Plane {
z: 0.0,
units: _,
},
..
} => return PlaneData::NegXY,
Self {
origin:
@ -365,7 +374,6 @@ impl Plane {
z: 1.0,
units: _,
},
..
} => return PlaneData::XZ,
Self {
origin:
@ -389,7 +397,6 @@ impl Plane {
z: 1.0,
units: _,
},
..
} => return PlaneData::NegXZ,
Self {
origin:
@ -413,7 +420,6 @@ impl Plane {
z: 1.0,
units: _,
},
..
} => return PlaneData::YZ,
Self {
origin:
@ -437,96 +443,78 @@ impl Plane {
z: 1.0,
units: _,
},
..
} => return PlaneData::NegYZ,
_ => {}
}
}
PlaneData::Plane {
PlaneData::Plane(Self {
origin: self.origin,
x_axis: self.x_axis,
y_axis: self.y_axis,
})
}
}
impl TryFrom<PlaneData> for PlaneInfo {
type Error = KclError;
fn try_from(value: PlaneData) -> Result<Self, Self::Error> {
if let PlaneData::Plane(info) = value {
return Ok(info);
}
let name = match value {
PlaneData::XY => PlaneName::Xy,
PlaneData::NegXY => PlaneName::NegXy,
PlaneData::XZ => PlaneName::Xz,
PlaneData::NegXZ => PlaneName::NegXz,
PlaneData::YZ => PlaneName::Yz,
PlaneData::NegYZ => PlaneName::NegYz,
PlaneData::Plane(_) => {
// We will never get here since we already checked for PlaneData::Plane.
return Err(KclError::Internal(KclErrorDetails {
message: format!("PlaneData {:?} not found", value),
source_ranges: Default::default(),
}));
}
};
let info = DEFAULT_PLANE_INFO.get(&name).ok_or_else(|| {
KclError::Internal(KclErrorDetails {
message: format!("Plane {} not found", name),
source_ranges: Default::default(),
})
})?;
Ok(info.clone())
}
}
impl From<PlaneData> for PlaneType {
fn from(value: PlaneData) -> Self {
match value {
PlaneData::XY => PlaneType::XY,
PlaneData::NegXY => PlaneType::XY,
PlaneData::XZ => PlaneType::XZ,
PlaneData::NegXZ => PlaneType::XZ,
PlaneData::YZ => PlaneType::YZ,
PlaneData::NegYZ => PlaneType::YZ,
PlaneData::Plane(_) => PlaneType::Custom,
}
}
}
pub(crate) fn from_plane_data(value: PlaneData, exec_state: &mut ExecState) -> Self {
impl Plane {
pub(crate) fn from_plane_data(value: PlaneData, exec_state: &mut ExecState) -> Result<Self, KclError> {
let id = exec_state.next_uuid();
match value {
PlaneData::XY => Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 1.0, 0.0, UnitLen::Unknown),
value: PlaneType::XY,
meta: vec![],
},
PlaneData::NegXY => Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(-1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 1.0, 0.0, UnitLen::Unknown),
value: PlaneType::XY,
meta: vec![],
},
PlaneData::XZ => Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
value: PlaneType::XZ,
meta: vec![],
},
PlaneData::NegXZ => Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(-1.0, 0.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
value: PlaneType::XZ,
meta: vec![],
},
PlaneData::YZ => Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(0.0, 1.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
value: PlaneType::YZ,
meta: vec![],
},
PlaneData::NegYZ => Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin: Point3d::new(0.0, 0.0, 0.0, UnitLen::Mm),
x_axis: Point3d::new(0.0, -1.0, 0.0, UnitLen::Unknown),
y_axis: Point3d::new(0.0, 0.0, 1.0, UnitLen::Unknown),
value: PlaneType::YZ,
meta: vec![],
},
PlaneData::Plane { origin, x_axis, y_axis } => {
let id = exec_state.next_uuid();
Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin,
x_axis,
y_axis,
value: PlaneType::Custom,
meta: vec![],
}
}
}
Ok(Plane {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
info: PlaneInfo::try_from(value.clone())?,
value: value.into(),
meta: vec![],
})
}
/// The standard planes are XY, YZ and XZ (in both positive and negative)
@ -629,7 +617,7 @@ impl Sketch {
adjust_camera: false,
planar_normal: if let SketchSurface::Plane(plane) = &self.on {
// We pass in the normal for the plane here.
let normal = plane.x_axis.axes_cross_product(&plane.y_axis);
let normal = plane.info.x_axis.axes_cross_product(&plane.info.y_axis);
Some(normal.into())
} else {
None
@ -664,13 +652,13 @@ impl SketchSurface {
}
pub(crate) fn x_axis(&self) -> Point3d {
match self {
SketchSurface::Plane(plane) => plane.x_axis,
SketchSurface::Plane(plane) => plane.info.x_axis,
SketchSurface::Face(face) => face.x_axis,
}
}
pub(crate) fn y_axis(&self) -> Point3d {
match self {
SketchSurface::Plane(plane) => plane.y_axis,
SketchSurface::Plane(plane) => plane.info.y_axis,
SketchSurface::Face(face) => face.y_axis,
}
}

3
rust/kcl-lib/src/execution/kcl_value.rs
View File

@ -4,6 +4,7 @@ use anyhow::Result;
use schemars::JsonSchema;
use serde::Serialize;
use super::types::UnitType;
use crate::{
errors::KclErrorDetails,
execution::{
@ -19,8 +20,6 @@ use crate::{
CompilationError, KclError, ModuleId, SourceRange,
};
use super::types::UnitType;
pub type KclObjectFields = HashMap<String, KclValue>;
/// Any KCL value.

12
rust/kcl-lib/src/execution/types.rs
View File

@ -8,7 +8,7 @@ use crate::{
execution::{
kcl_value::{KclValue, TypeDef},
memory::{self},
ExecState, Plane, Point3d,
ExecState, Plane, PlaneInfo, Point3d,
},
parsing::{
ast::types::{PrimitiveType as AstPrimitiveType, Type},
@ -1076,9 +1076,11 @@ impl KclValue {
id,
#[cfg(feature = "artifact-graph")]
artifact_id: id.into(),
origin,
x_axis: x_axis.normalize(),
y_axis: y_axis.normalize(),
info: PlaneInfo {
origin,
x_axis: x_axis.normalize(),
y_axis: y_axis.normalize(),
},
value: super::PlaneType::Uninit,
meta: meta.clone(),
};
@ -1364,7 +1366,7 @@ mod test {
KclValue::TagIdentifier(Box::new("foo".parse().unwrap())),
KclValue::TagDeclarator(Box::new(crate::parsing::ast::types::TagDeclarator::new("foo"))),
KclValue::Plane {
value: Box::new(Plane::from_plane_data(crate::std::sketch::PlaneData::XY, exec_state)),
value: Box::new(Plane::from_plane_data(crate::std::sketch::PlaneData::XY, exec_state).unwrap()),
},
// No easy way to make a Face, Sketch, Solid, or Helix
KclValue::ImportedGeometry(crate::execution::ImportedGeometry::new(

93
rust/kcl-lib/src/lint/checks/default_plane.rs Normal file
View File

@ -0,0 +1,93 @@
use anyhow::Result;
use crate::{
errors::Suggestion,
lint::rule::{def_finding, Discovered, Finding},
walk::Node,
};
use super::offset_plane::start_sketch_on_check_specific_plane;
def_finding!(
Z0002,
"default plane should be called versus explicitly defined",
"\
startSketchOn should be a default plane in this case ✏️
The startSketchOn stdlib function has the ability to define a default Plane
to begin the sketch on.
These are the default planes: XY, -XY, XZ, -XZ, YZ, -YZ.
"
);
pub fn lint_should_be_default_plane(node: Node) -> Result<Vec<Discovered>> {
let Some((call_source_range, plane_name, offset)) = start_sketch_on_check_specific_plane(node)? else {
return Ok(vec![]);
};
// We only care about the default planes.
if offset != 0.0 {
return Ok(vec![]);
}
let suggestion = Suggestion {
title: "use defaultPlane instead".to_owned(),
insert: format!("{}", plane_name),
source_range: call_source_range,
};
Ok(vec![Z0002.at(
format!(
"custom plane in startSketchOn; defaultPlane {} would work here",
plane_name
),
call_source_range,
Some(suggestion),
)])
}
#[cfg(test)]
mod tests {
use super::{lint_should_be_default_plane, Z0002};
use crate::lint::rule::{test_finding, test_no_finding};
test_finding!(
z0002_bad_sketch_on,
lint_should_be_default_plane,
Z0002,
"\
startSketchOn({
origin = { x = 0, y = 0, z = 0 },
xAxis = { x = 1, y = 0, z = 0 },
yAxis = { x = 0, y = 0, z = 1 },
})
|> startProfile(at = [0, 0])
",
"custom plane in startSketchOn; defaultPlane XZ would work here",
Some("XZ".to_string())
);
test_no_finding!(
z0002_good_sketch_on,
lint_should_be_default_plane,
Z0002,
"\
startSketchOn({
origin = { x = 10, y = -14.3, z = 0 },
xAxis = { x = 1, y = 0, z = 0 },
yAxis = { x = 0, y = 0, z = 1 },
})
"
);
test_no_finding!(
z0002_offset_plane,
lint_should_be_default_plane,
Z0002,
"\
startSketchOn({
origin = { x = 0, y = -14.3, z = 0 },
xAxis = { x = 1, y = 0, z = 0 },
yAxis = { x = 0, y = 0, z = 1 },
})
"
);
}

2
rust/kcl-lib/src/lint/checks/mod.rs
View File

@ -1,5 +1,7 @@
mod camel_case;
mod default_plane;
mod offset_plane;
pub use camel_case::{lint_object_properties, lint_variables, Z0001};
pub use default_plane::{lint_should_be_default_plane, Z0002};
pub use offset_plane::{lint_should_be_offset_plane, Z0003};

315
rust/kcl-lib/src/lint/checks/offset_plane.rs
View File

@ -1,9 +1,9 @@
use std::collections::HashMap;
use anyhow::Result;
use crate::{
engine::{PlaneName, DEFAULT_PLANE_INFO},
errors::Suggestion,
execution::{types::UnitLen, PlaneInfo, Point3d},
lint::rule::{def_finding, Discovered, Finding},
parsing::ast::types::{BinaryPart, Expr, LiteralValue, ObjectExpression, UnaryOperator},
walk::Node,
@ -28,136 +28,25 @@ use offsetPlane where possible.
);
pub fn lint_should_be_offset_plane(node: Node) -> Result<Vec<Discovered>> {
let Node::CallExpression(call) = node else {
let Some((call_source_range, plane_name, offset)) = start_sketch_on_check_specific_plane(node)? else {
return Ok(vec![]);
};
if call.inner.callee.inner.name.name != "startSketchOn" {
// We don't care about the default planes.
if offset == 0.0 {
return Ok(vec![]);
}
if call.arguments.len() != 1 {
// we only look for single-argument object patterns, if there's more
// than that we don't have a plane decl
return Ok(vec![]);
}
let Expr::ObjectExpression(arg) = &call.arguments[0] else {
return Ok(vec![]);
};
let mut origin: Option<(f64, f64, f64)> = None;
let mut x_vec: Option<(f64, f64, f64)> = None;
let mut y_vec: Option<(f64, f64, f64)> = None;
for property in &arg.inner.properties {
let Expr::ObjectExpression(ref point) = property.inner.value else {
return Ok(vec![]);
};
let Some((x, y, z)) = get_xyz(&point.inner) else {
return Ok(vec![]);
};
let property_name = &property.inner.key.inner.name;
match property_name.as_str() {
"origin" => origin = Some((x, y, z)),
"xAxis" => x_vec = Some((x, y, z)),
"yAxis" => y_vec = Some((x, y, z)),
_ => {
continue;
}
};
}
let Some(origin) = origin else { return Ok(vec![]) };
let Some(x_vec) = x_vec else { return Ok(vec![]) };
let Some(y_vec) = y_vec else { return Ok(vec![]) };
if [origin.0, origin.1, origin.2].iter().filter(|v| **v == 0.0).count() < 2 {
return Ok(vec![]);
}
// two of the origin values are 0, 0; let's work it out and check
// what's **up**
/// This will attempt to very poorly translate orientation to a letter
/// if it's possible to do so. The engine will norm these vectors, so
/// we'll just use logic off 0 for now, but this sucks, generally speaking.
fn vector_to_letter<'a>(x: f64, y: f64, z: f64) -> Option<&'a str> {
if x > 0.0 && y == 0.0 && z == 0.0 {
return Some("X");
}
if x < 0.0 && y == 0.0 && z == 0.0 {
return Some("-X");
}
if x == 0.0 && y > 0.0 && z == 0.0 {
return Some("Y");
}
if x == 0.0 && y < 0.0 && z == 0.0 {
return Some("-Y");
}
if x == 0.0 && y == 0.0 && z > 0.0 {
return Some("Z");
}
if x == 0.0 && y == 0.0 && z < 0.0 {
return Some("-Z");
}
None
}
let allowed_planes = HashMap::from([
// allowed built-in planes
("XY".to_owned(), true),
("-XY".to_owned(), true),
("XZ".to_owned(), true),
("-XZ".to_owned(), true),
("YZ".to_owned(), true),
("-YZ".to_owned(), true),
]);
// Currently, the engine **ONLY** accepts[1] the following:
//
// XY
// -XY
// XZ
// -XZ
// YZ
// -YZ
//
// [1]: https://zoo.dev/docs/kcl/types/PlaneData
let plane_name = format!(
"{}{}",
vector_to_letter(x_vec.0, x_vec.1, x_vec.2).unwrap_or(""),
vector_to_letter(y_vec.0, y_vec.1, y_vec.2).unwrap_or(""),
);
if !allowed_planes.contains_key(&plane_name) {
return Ok(vec![]);
};
let call_source_range = SourceRange::new(
call.arguments[0].start(),
call.arguments[0].end(),
call.arguments[0].module_id(),
);
let offset = get_offset(origin, x_vec, y_vec);
let suggestion = offset.map(|offset| Suggestion {
let suggestion = Suggestion {
title: "use offsetPlane instead".to_owned(),
insert: format!("offsetPlane({}, offset = {})", plane_name, offset),
source_range: call_source_range,
});
};
Ok(vec![Z0003.at(
format!(
"custom plane in startSketchOn; offsetPlane from {} would work here",
plane_name
),
call_source_range,
suggestion,
Some(suggestion),
)])
}
@ -205,35 +94,172 @@ fn get_xyz(point: &ObjectExpression) -> Option<(f64, f64, f64)> {
Some((x?, y?, z?))
}
fn get_offset(origin: (f64, f64, f64), x_axis: (f64, f64, f64), y_axis: (f64, f64, f64)) -> Option<f64> {
fn get_offset(info: &PlaneInfo) -> Option<f64> {
// Check which number is not a 1 or -1, or zero.
// Return that back out since that is the offset.
// This is a bit of a hack, but it works for now.
// We can do better later.
if origin.0 != 1.0 && origin.0 != -1.0 && origin.0 != 0.0 {
return Some(origin.0);
} else if origin.1 != 1.0 && origin.1 != -1.0 && origin.1 != 0.0 {
return Some(origin.1);
} else if origin.2 != 1.0 && origin.2 != -1.0 && origin.2 != 0.0 {
return Some(origin.2);
} else if x_axis.0 != 1.0 && x_axis.0 != -1.0 && x_axis.0 != 0.0 {
return Some(x_axis.0);
} else if x_axis.1 != 1.0 && x_axis.1 != -1.0 && x_axis.1 != 0.0 {
return Some(x_axis.1);
} else if x_axis.2 != 1.0 && x_axis.2 != -1.0 && x_axis.2 != 0.0 {
return Some(x_axis.2);
} else if y_axis.0 != 1.0 && y_axis.0 != -1.0 && y_axis.0 != 0.0 {
return Some(y_axis.0);
} else if y_axis.1 != 1.0 && y_axis.1 != -1.0 && y_axis.1 != 0.0 {
return Some(y_axis.1);
} else if y_axis.2 != 1.0 && y_axis.2 != -1.0 && y_axis.2 != 0.0 {
return Some(y_axis.2);
if info.origin.x != 1.0 && info.origin.x != -1.0 && info.origin.x != 0.0 {
return Some(info.origin.x);
} else if info.origin.y != 1.0 && info.origin.y != -1.0 && info.origin.y != 0.0 {
return Some(info.origin.y);
} else if info.origin.z != 1.0 && info.origin.z != -1.0 && info.origin.z != 0.0 {
return Some(info.origin.z);
} else if info.x_axis.x != 1.0 && info.x_axis.x != -1.0 && info.x_axis.x != 0.0 {
return Some(info.x_axis.x);
} else if info.x_axis.y != 1.0 && info.x_axis.y != -1.0 && info.x_axis.y != 0.0 {
return Some(info.x_axis.y);
} else if info.x_axis.z != 1.0 && info.x_axis.z != -1.0 && info.x_axis.z != 0.0 {
return Some(info.x_axis.z);
} else if info.y_axis.x != 1.0 && info.y_axis.x != -1.0 && info.y_axis.x != 0.0 {
return Some(info.y_axis.x);
} else if info.y_axis.y != 1.0 && info.y_axis.y != -1.0 && info.y_axis.y != 0.0 {
return Some(info.y_axis.y);
} else if info.y_axis.z != 1.0 && info.y_axis.z != -1.0 && info.y_axis.z != 0.0 {
return Some(info.y_axis.z);
}
None
}
pub fn start_sketch_on_check_specific_plane(node: Node) -> Result<Option<(SourceRange, PlaneName, f64)>> {
let Node::CallExpression(call) = node else {
return Ok(None);
};
if call.inner.callee.inner.name.name != "startSketchOn" {
return Ok(None);
}
if call.arguments.len() != 1 {
// we only look for single-argument object patterns, if there's more
// than that we don't have a plane decl
return Ok(None);
}
let call_source_range = SourceRange::new(
call.arguments[0].start(),
call.arguments[0].end(),
call.arguments[0].module_id(),
);
let Expr::ObjectExpression(arg) = &call.arguments[0] else {
return Ok(None);
};
let mut origin: Option<Point3d> = None;
let mut x_vec: Option<Point3d> = None;
let mut y_vec: Option<Point3d> = None;
for property in &arg.inner.properties {
let Expr::ObjectExpression(ref point) = property.inner.value else {
return Ok(None);
};
let Some((x, y, z)) = get_xyz(&point.inner) else {
return Ok(None);
};
let property_name = &property.inner.key.inner.name;
match property_name.as_str() {
"origin" => {
origin = Some(Point3d {
x,
y,
z,
units: UnitLen::Mm,
})
}
"xAxis" => {
x_vec = Some(Point3d {
x,
y,
z,
units: UnitLen::Unknown,
})
}
"yAxis" => {
y_vec = Some(Point3d {
x,
y,
z,
units: UnitLen::Unknown,
})
}
_ => {
continue;
}
};
}
let (Some(origin), Some(x_vec), Some(y_vec)) = (origin, x_vec, y_vec) else {
return Ok(None);
};
if [origin.x, origin.y, origin.z].iter().filter(|v| **v == 0.0).count() < 2 {
return Ok(None);
}
let plane_info = PlaneInfo {
origin,
x_axis: x_vec,
y_axis: y_vec,
};
// Return early if we have a default plane.
if let Some((name, _)) = DEFAULT_PLANE_INFO.iter().find(|(_, plane)| **plane == plane_info) {
return Ok(Some((call_source_range, *name, 0.0)));
}
let normalized_plane_info = normalize_plane_info(&plane_info);
println!("normalized plane info: {:?}", normalized_plane_info);
// Check our default planes.
let Some((matched_plane_name, _)) = DEFAULT_PLANE_INFO
.iter()
.find(|(_, plane)| **plane == normalized_plane_info)
else {
return Ok(None);
};
let Some(offset) = get_offset(&plane_info) else {
return Ok(None);
};
Ok(Some((call_source_range, *matched_plane_name, offset)))
}
// Clone the plane info and normalize any number that is not zero to 1.0 or -1.0 (if negative)
// so we can compare it to the built-in planes.
fn normalize_plane_info(plane_info: &PlaneInfo) -> PlaneInfo {
let mut normalized_plane_info = plane_info.clone();
normalized_plane_info.origin = Point3d {
x: 0.0,
y: 0.0,
z: 0.0,
units: normalized_plane_info.origin.units,
};
normalized_plane_info.y_axis.x = if normalized_plane_info.y_axis.x != 0.0 {
normalized_plane_info.y_axis.x.signum()
} else {
0.0
};
normalized_plane_info.y_axis.y = if normalized_plane_info.y_axis.y != 0.0 {
normalized_plane_info.y_axis.y.signum()
} else {
0.0
};
normalized_plane_info.y_axis.z = if normalized_plane_info.y_axis.z != 0.0 {
normalized_plane_info.y_axis.z.signum()
} else {
0.0
};
normalized_plane_info
}
#[cfg(test)]
mod tests {
use super::{lint_should_be_offset_plane, Z0003};
@ -265,6 +291,19 @@ startSketchOn({
xAxis = { x = 1, y = 0, z = 0 },
yAxis = { x = 0, y = 0, z = 1 },
})
"
);
test_no_finding!(
z0003_default_plane,
lint_should_be_offset_plane,
Z0003,
"\
startSketchOn({
origin = { x = 0, y = 0, z = 0 },
xAxis = { x = 1, y = 0, z = 0 },
yAxis = { x = 0, y = 0, z = 1 },
})
"
);
}

1
rust/kcl-lib/src/parsing/ast/types/mod.rs
View File

@ -323,6 +323,7 @@ impl Node<Program> {
let rules = vec![
crate::lint::checks::lint_variables,
crate::lint::checks::lint_object_properties,
crate::lint::checks::lint_should_be_default_plane,
crate::lint::checks::lint_should_be_offset_plane,
];

16
rust/kcl-lib/src/std/args.rs
View File

@ -15,8 +15,8 @@ use crate::{
execution::{
kcl_value::FunctionSource,
types::{NumericType, PrimitiveType, RuntimeType, UnitAngle, UnitLen, UnitType},
ExecState, ExecutorContext, ExtrudeSurface, Helix, KclObjectFields, KclValue, Metadata, Sketch, SketchSurface,
Solid, TagIdentifier,
ExecState, ExecutorContext, ExtrudeSurface, Helix, KclObjectFields, KclValue, Metadata, PlaneInfo, Sketch,
SketchSurface, Solid, TagIdentifier,
},
parsing::ast::types::TagNode,
source_range::SourceRange,
@ -984,11 +984,11 @@ impl<'a> FromKclValue<'a> for super::sketch::PlaneData {
fn from_kcl_val(arg: &'a KclValue) -> Option<Self> {
// Case 0: actual plane
if let KclValue::Plane { value } = arg {
return Some(Self::Plane {
origin: value.origin,
x_axis: value.x_axis,
y_axis: value.y_axis,
});
return Some(Self::Plane(PlaneInfo {
origin: value.info.origin,
x_axis: value.info.x_axis,
y_axis: value.info.y_axis,
}));
}
// Case 1: predefined plane
if let Some(s) = arg.as_str() {
@ -1008,7 +1008,7 @@ impl<'a> FromKclValue<'a> for super::sketch::PlaneData {
let origin = plane.get("origin").and_then(FromKclValue::from_kcl_val)?;
let x_axis = plane.get("xAxis").and_then(FromKclValue::from_kcl_val)?;
let y_axis = plane.get("yAxis").and_then(FromKclValue::from_kcl_val)?;
Some(Self::Plane { origin, x_axis, y_axis })
Some(Self::Plane(PlaneInfo { origin, x_axis, y_axis }))
}
}

12
rust/kcl-lib/src/std/planes.rs
View File

@ -24,13 +24,13 @@ async fn inner_offset_plane(
exec_state: &mut ExecState,
args: &Args,
) -> Result<Plane, KclError> {
let mut plane = Plane::from_plane_data(plane, exec_state);
let mut plane = Plane::from_plane_data(plane, exec_state)?;
// Though offset planes might be derived from standard planes, they are not
// standard planes themselves.
plane.value = PlaneType::Custom;
let normal = plane.x_axis.axes_cross_product(&plane.y_axis);
plane.origin += normal * offset.to_length_units(plane.origin.units);
let normal = plane.info.x_axis.axes_cross_product(&plane.info.y_axis);
plane.info.origin += normal * offset.to_length_units(plane.info.origin.units);
make_offset_plane_in_engine(&plane, exec_state, args).await?;
Ok(plane)
@ -53,10 +53,10 @@ async fn make_offset_plane_in_engine(plane: &Plane, exec_state: &mut ExecState,
plane.id,
ModelingCmd::from(mcmd::MakePlane {
clobber: false,
origin: plane.origin.into(),
origin: plane.info.origin.into(),
size: LengthUnit(default_size),
x_axis: plane.x_axis.into(),
y_axis: plane.y_axis.into(),
x_axis: plane.info.x_axis.into(),
y_axis: plane.info.y_axis.into(),
hide: Some(false),
}),
)

27
rust/kcl-lib/src/std/sketch.rs
View File

@ -18,7 +18,7 @@ use crate::{
errors::{KclError, KclErrorDetails},
execution::{
types::{ArrayLen, NumericType, PrimitiveType, RuntimeType, UnitLen},
BasePath, ExecState, Face, GeoMeta, KclValue, Path, Plane, Point2d, Point3d, Sketch, SketchSurface, Solid,
BasePath, ExecState, Face, GeoMeta, KclValue, Path, Plane, PlaneInfo, Point2d, Sketch, SketchSurface, Solid,
TagEngineInfo, TagIdentifier,
},
parsing::ast::types::TagNode,
@ -953,16 +953,7 @@ pub enum PlaneData {
#[serde(rename = "-YZ", alias = "-yz")]
NegYZ,
/// A defined plane.
Plane {
/// Origin of the plane.
origin: Point3d,
/// What should the plane’s X axis be?
#[serde(rename = "xAxis")]
x_axis: Point3d,
/// What should the plane’s Y axis be?
#[serde(rename = "yAxis")]
y_axis: Point3d,
},
Plane(PlaneInfo),
}
/// Start a sketch on a specific plane or face.
@ -1177,13 +1168,13 @@ async fn inner_start_sketch_on(
}
SketchData::Plane(plane) => {
if plane.value == crate::exec::PlaneType::Uninit {
if plane.origin.units == UnitLen::Unknown {
if plane.info.origin.units == UnitLen::Unknown {
return Err(KclError::Semantic(KclErrorDetails {
message: "Origin of plane has unknown units".to_string(),
source_ranges: vec![args.source_range],
}));
}
let plane = make_sketch_plane_from_orientation(plane.into_plane_data(), exec_state, args).await?;
let plane = make_sketch_plane_from_orientation(plane.info.into_plane_data(), exec_state, args).await?;
Ok(SketchSurface::Plane(plane))
} else {
// Create artifact used only by the UI, not the engine.
@ -1252,7 +1243,7 @@ async fn make_sketch_plane_from_orientation(
exec_state: &mut ExecState,
args: &Args,
) -> Result<Box<Plane>, KclError> {
let plane = Plane::from_plane_data(data.clone(), exec_state);
let plane = Plane::from_plane_data(data.clone(), exec_state)?;
// Create the plane on the fly.
let clobber = false;
@ -1262,10 +1253,10 @@ async fn make_sketch_plane_from_orientation(
plane.id,
ModelingCmd::from(mcmd::MakePlane {
clobber,
origin: plane.origin.into(),
origin: plane.info.origin.into(),
size,
x_axis: plane.x_axis.into(),
y_axis: plane.y_axis.into(),
x_axis: plane.info.x_axis.into(),
y_axis: plane.info.y_axis.into(),
hide,
}),
)
@ -1374,7 +1365,7 @@ pub(crate) async fn inner_start_profile(
adjust_camera: false,
planar_normal: if let SketchSurface::Plane(plane) = &sketch_surface {
// We pass in the normal for the plane here.
let normal = plane.x_axis.axes_cross_product(&plane.y_axis);
let normal = plane.info.x_axis.axes_cross_product(&plane.info.y_axis);
Some(normal.into())
} else {
None