forked from 1b-agent/opencadstudio-upstream
fix: review fixes for the Aligned 2Lines placement and DCCONVERT
The 2Lines placement (#1459) wrote the second line's new ends back with `set_resolved_point`, which was not the inverse of the `resolve_point` it paired with. `resolve_point` reads polyline vertices in WCS through the polyline's OCS; the writer put that world point straight into the OCS vertex, so a mirrored polyline (normal -Z, common in real drawings) had its vertex flipped, and the closing segment's end marker — which names vertex 0 on a closed polyline — found no vertex and the placement was silently dropped. The writer now converts back into the OCS, keeps the polyline's own elevation, and wraps the closing marker the same way. The same arm pushed `test_pline_line_then_arc` past the default test thread's stack: its locals, a cloned entity among them, sat in the frame of the command-result dispatcher, which is on the stack of every click. The placement is built out of line and handed over boxed. Merging #1447 had left nothing setting DIMCONSTRAINT's default to Diameter; the Dispatch arm #1461 turned into a match now covers it with Radius and Convert. DCCONVERT's result lines go through the translation macros. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
1ec2f554a9
commit
f2d534bf71
3 files changed
+84
-49
No files matched your search
+13
-38
@@ -4794,7 +4794,7 @@ impl OpenCADStudio {
|
||||
second_ends,
|
||||
} => {
|
||||
use crate::scene::parametric_constraints::{
|
||||
resolve_point, set_resolved_point, two_lines_placement, ConstraintKind,
|
||||
resolve_point, two_lines_placed_entity, ConstraintKind,
|
||||
};
|
||||
let scope = self.tabs[i].current_parametric_scope();
|
||||
let refs = [first_line, second_line];
|
||||
@@ -4806,38 +4806,16 @@ impl OpenCADStudio {
|
||||
return self.apply_cmd_result(CmdResult::ReportError(message.to_string()));
|
||||
}
|
||||
// The reference's placement of the second line, written
|
||||
// before the Parallel constraint holds it there.
|
||||
let placed = {
|
||||
let document = &self.tabs[i].scene.document;
|
||||
let world = |reference: crate::scene::parametric_constraints::ParametricRef| {
|
||||
let entity = document.get_entity(reference.entity)?;
|
||||
let point = resolve_point(entity, reference.marker?)?;
|
||||
Some(glam::DVec3::new(point.x, point.y, point.z))
|
||||
};
|
||||
let ends = |pair: [crate::scene::parametric_constraints::ParametricRef; 2]| {
|
||||
Some([world(pair[0])?, world(pair[1])?])
|
||||
};
|
||||
ends(first_ends)
|
||||
.zip(ends(second_ends))
|
||||
.and_then(|(first, second)| two_lines_placement(first, first_pick, second))
|
||||
};
|
||||
let placed_entity = placed.and_then(|points| {
|
||||
let mut entity = self.tabs[i]
|
||||
.scene
|
||||
.document
|
||||
.get_entity(second_line.entity)
|
||||
.cloned()?;
|
||||
let written = second_ends.iter().zip(points).all(|(reference, point)| {
|
||||
reference.marker.is_some_and(|marker| {
|
||||
set_resolved_point(
|
||||
&mut entity,
|
||||
marker,
|
||||
acadrust::types::Vector3::new(point.x, point.y, point.z),
|
||||
)
|
||||
})
|
||||
});
|
||||
written.then_some(entity)
|
||||
});
|
||||
// before the Parallel constraint holds it there. Built out of
|
||||
// line and boxed: this arm's locals would otherwise sit in
|
||||
// `apply_cmd_result`'s frame, which is on every click's stack.
|
||||
let placed_entity = two_lines_placed_entity(
|
||||
&self.tabs[i].scene.document,
|
||||
first_ends,
|
||||
first_pick,
|
||||
second_line,
|
||||
second_ends,
|
||||
);
|
||||
let already = self.tabs[i]
|
||||
.scene
|
||||
.parametric_constraint_set(scope)
|
||||
@@ -4861,11 +4839,7 @@ impl OpenCADStudio {
|
||||
});
|
||||
let pending = self.begin_undo(i, "Parallel constraint", 1, true);
|
||||
if let Some(entity) = placed_entity {
|
||||
let before = self.tabs[i].scene.document.get_entity_arc(second_line.entity);
|
||||
self.tabs[i]
|
||||
.scene
|
||||
.record_undo_before(second_line.entity, before);
|
||||
self.tabs[i].scene.update_entity(entity);
|
||||
self.tabs[i].scene.replace_entity_recorded(entity);
|
||||
}
|
||||
self.tabs[i]
|
||||
.scene
|
||||
@@ -6286,6 +6260,7 @@ impl OpenCADStudio {
|
||||
if from_menu {
|
||||
match cmd.as_str() {
|
||||
"DCRADIUS" => self.dim_constraint_last = "Radius",
|
||||
"DCDIAMETER" => self.dim_constraint_last = "Diameter",
|
||||
"DCCONVERT" => self.dim_constraint_last = "Convert",
|
||||
_ => {}
|
||||
}
|
||||
|
||||
@@ -1398,10 +1398,10 @@ impl OpenCADStudio {
|
||||
}
|
||||
if conversions.is_empty() {
|
||||
self.command_line
|
||||
.push_output("0 associative dimensions converted");
|
||||
.push_output(crate::t!("0 associative dimensions converted").as_ref());
|
||||
self.command_line.push_output(
|
||||
format!("{refused} associative dimension(s) could not be converted")
|
||||
.as_str(),
|
||||
crate::tf!("{refused} associative dimension(s) could not be converted")
|
||||
.as_ref(),
|
||||
);
|
||||
self.tabs[i].scene.deselect_all();
|
||||
self.refresh_properties();
|
||||
@@ -1502,9 +1502,9 @@ impl OpenCADStudio {
|
||||
self.tabs[i].dirty = true;
|
||||
self.refresh_properties();
|
||||
self.command_line
|
||||
.push_output(format!("{converted} associative dimensions converted").as_str());
|
||||
.push_output(crate::tf!("{converted} associative dimensions converted").as_ref());
|
||||
self.command_line.push_output(
|
||||
format!("{refused} associative dimension(s) could not be converted").as_str(),
|
||||
crate::tf!("{refused} associative dimension(s) could not be converted").as_ref(),
|
||||
);
|
||||
if let Some(pd) = pending {
|
||||
self.commit_undo_delta(i, pd);
|
||||
|
||||
@@ -525,28 +525,47 @@ pub(crate) fn measured_expression(value: f64) -> String {
|
||||
|
||||
/// Writes a constraint point's position: a line's start (0) or end (1), a
|
||||
/// polyline's vertex by index. Other entities and markers are left alone.
|
||||
///
|
||||
/// The inverse of [`resolve_point`], which reads polyline vertices in WCS
|
||||
/// through the polyline's OCS: the world point goes back into the OCS the
|
||||
/// vertex is stored in (a mirrored polyline has a -Z normal, and writing the
|
||||
/// world X into its OCS X would flip the vertex), the elevation stays the
|
||||
/// polyline's own, and the closing segment's end marker names vertex 0.
|
||||
pub(crate) fn set_resolved_point(
|
||||
entity: &mut acadrust::EntityType,
|
||||
marker: i32,
|
||||
point: Vector3,
|
||||
) -> bool {
|
||||
if marker < 0 {
|
||||
return false;
|
||||
}
|
||||
let index = polyline_vertex_index(entity, marker as usize).unwrap_or(marker as usize);
|
||||
let to_ocs = |normal: Vector3| {
|
||||
crate::scene::view::transform::wcs_point_to_ocs(
|
||||
(point.x, point.y, point.z),
|
||||
(normal.x, normal.y, normal.z),
|
||||
)
|
||||
};
|
||||
match entity {
|
||||
acadrust::EntityType::Line(line) => match marker {
|
||||
0 => line.start = point,
|
||||
1 => line.end = point,
|
||||
_ => return false,
|
||||
},
|
||||
acadrust::EntityType::LwPolyline(polyline) if marker >= 0 => {
|
||||
let Some(vertex) = polyline.vertices.get_mut(marker as usize) else {
|
||||
acadrust::EntityType::LwPolyline(polyline) => {
|
||||
let (x, y, _) = to_ocs(polyline.normal);
|
||||
let Some(vertex) = polyline.vertices.get_mut(index) else {
|
||||
return false;
|
||||
};
|
||||
vertex.location = acadrust::types::Vector2::new(point.x, point.y);
|
||||
vertex.location = acadrust::types::Vector2::new(x, y);
|
||||
}
|
||||
acadrust::EntityType::Polyline2D(polyline) if marker >= 0 => {
|
||||
let Some(vertex) = polyline.vertices.get_mut(marker as usize) else {
|
||||
acadrust::EntityType::Polyline2D(polyline) => {
|
||||
let (x, y, _) = to_ocs(polyline.normal);
|
||||
let Some(vertex) = polyline.vertices.get_mut(index) else {
|
||||
return false;
|
||||
};
|
||||
vertex.location = point;
|
||||
vertex.location.x = x;
|
||||
vertex.location.y = y;
|
||||
}
|
||||
_ => return false,
|
||||
}
|
||||
@@ -585,6 +604,36 @@ pub(crate) fn two_lines_placement(
|
||||
Some([s0 + shift, s0 + heading * length + shift])
|
||||
}
|
||||
|
||||
/// The second line of an Aligned 2Lines pick moved to where
|
||||
/// [`two_lines_placement`] puts it, or `None` when either line cannot be
|
||||
/// resolved or written back.
|
||||
///
|
||||
/// Out of line and boxed on purpose: the caller is an arm of the command
|
||||
/// driver's result dispatcher, whose debug-build frame is on the stack of
|
||||
/// every click, and an `EntityType` held there is paid for on each of them.
|
||||
#[inline(never)]
|
||||
pub(crate) fn two_lines_placed_entity(
|
||||
document: &acadrust::CadDocument,
|
||||
first_ends: [ParametricRef; 2],
|
||||
first_pick: glam::DVec3,
|
||||
second_line: ParametricRef,
|
||||
second_ends: [ParametricRef; 2],
|
||||
) -> Option<Box<acadrust::EntityType>> {
|
||||
let world = |reference: ParametricRef| {
|
||||
let point = resolve_point(document.get_entity(reference.entity)?, reference.marker?)?;
|
||||
Some(glam::DVec3::new(point.x, point.y, point.z))
|
||||
};
|
||||
let ends = |pair: [ParametricRef; 2]| Some([world(pair[0])?, world(pair[1])?]);
|
||||
let points = two_lines_placement(ends(first_ends)?, first_pick, ends(second_ends)?)?;
|
||||
let mut entity = Box::new(document.get_entity(second_line.entity)?.clone());
|
||||
let written = second_ends.iter().zip(points).all(|(reference, point)| {
|
||||
reference.marker.is_some_and(|marker| {
|
||||
set_resolved_point(&mut entity, marker, Vector3::new(point.x, point.y, point.z))
|
||||
})
|
||||
});
|
||||
written.then_some(entity)
|
||||
}
|
||||
|
||||
/// Moves a dynamic dimension's extension origins to `first`/`second`,
|
||||
/// keeping its dimension line where it was; true when anything moved.
|
||||
fn dynamic_dimension_follow_points(
|
||||
@@ -2400,6 +2449,17 @@ impl super::Scene {
|
||||
let _ = self.document.layers.add(layer);
|
||||
}
|
||||
|
||||
/// Replace an entity in place inside an open undo recording, keeping the
|
||||
/// previous version for undo. Out of line so a boxed entity handed over by
|
||||
/// the command driver never lands in its frame.
|
||||
#[inline(never)]
|
||||
pub(crate) fn replace_entity_recorded(&mut self, entity: Box<acadrust::EntityType>) {
|
||||
let handle = entity.common().handle;
|
||||
let before = self.document.get_entity_arc(handle);
|
||||
self.record_undo_before(handle, before);
|
||||
self.update_entity(*entity);
|
||||
}
|
||||
|
||||
/// True when an angular constraint drives with the parameter `name`.
|
||||
pub(crate) fn parameter_is_angular(&self, name: &str) -> bool {
|
||||
self.parametric_constraints
|
||||
|
||||
Reference in new issue
Block a user