fix: preserve constraint endpoints when converting shapes to polygons (#27)#76
fix: preserve constraint endpoints when converting shapes to polygons (#27)#761egoman wants to merge 1 commit into
Conversation
…#27) When converting a rectangle or ellipse to a polygon, constraints that referenced the old shape were being silently dropped because deleteByIdDirect cascades to delete attached constraints. This commit: - Adds _relinkEndpointForShapeConversion() to map locked-rectangle and locked-ellipse endpoints to locked-polygon (or free point for center) - Adds _relinkAndRecordConstraint() to update constraints and record the appropriate undo entries for all constraint types - Restructures convertRectangleToPolygon and convertEllipseToPolygon to relink constraints before deleting the old shape, preventing cascade deletion - Uses a transaction so undo/redo properly restores original endpoints - Adds 14 comprehensive tests covering perimeter points, center points, resolvability, undo/redo, multiple constraints, horizontal/vertical constraints, and non-matching endpoints
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| this.historyManager.applyTransaction('polygon-to-rectangle', () => { | ||
| this.historyManager.apply(UndoEntry.rectangleToPolygon(geometry, polygon)); | ||
| // 1. Add the new polygon (record undo entry so it's removed on undo) | ||
| this.addDirect(polygon); | ||
| this.historyManager.push(UndoEntry.insert(polygon)); | ||
|
|
||
| // 2. Relink any existing constraints from rectangle to polygon BEFORE the rectangle is deleted | ||
| for (const constraint of existingConstraints) { | ||
| this._relinkAndRecordConstraint(constraint, rectangleId, polygon.id, 'rectangle', center); | ||
| } | ||
|
|
||
| // 3. Delete the old rectangle (no cascade since constraints are already relinked) | ||
| this.deleteByIdDirect(rectangleId); | ||
| this.historyManager.push(UndoEntry.deleteGeometry(geometry)); | ||
|
|
||
| // 4. Add H/V constraints for the new polygon edges | ||
| for (const template of constraintTemplates) { | ||
| this.addConstraint(template); | ||
| } |
There was a problem hiding this comment.
🚩 Old rectangleToPolygon and ellipseToPolygon UndoEntry types are now dead code
The old UndoEntry.rectangleToPolygon() and UndoEntry.ellipseToPolygon() factory functions (src/lib/history/types.ts:529-542) and their corresponding entry types (RectangleToPolygonEntry, EllipseToPolygonEntry at src/lib/history/types.ts:168-183) are no longer produced by any code path. The HistoryManager still has applyForward/applyReverse cases for 'rectangle-to-polygon' and 'ellipse-to-polygon' entry types (src/lib/history/HistoryManager.ts:343-350, 587-594). These are dead code now that the conversion uses insert + delete + move-endpoints entries instead. They may need to remain for backward compatibility with serialized undo stacks from older saves, but if not, they should be cleaned up.
(Refers to lines 1074-1093)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
When converting a rectangle or ellipse to a polygon (via
ConvertToPolygonActionorTrimSplitTool), any attached constraints were being silently dropped. The old shape was deleted viadeleteByIdDirect, which cascades to delete all constraints referencing it.Approach
The fix relinks constraint endpoints before deleting the old shape, mapping
locked-rectangle/locked-ellipseendpoints tolocked-polygonendpoints:upperLeft→ pointIndex 0,upperRight→ 1,lowerRight→ 2,lowerLeft→ 3top→ 0,right→ 1,bottom→ 2,left→ 3pointendpoints since there is no corresponding polygon vertexEach constraint update is recorded via the appropriate
*MoveEndpointsundo entry (linear, perpendicular, parallel, horizontal, vertical, colinear), ensuring undo/redo properly restores original endpoints.What changed from the previous attempt (#38)
The previous PR (#38) restructured the entire
GeometryStorefrom ECS components to separate arrays — this was too invasive and affected the entire codebase.This fix takes a targeted approach: only the conversion methods are modified, no architecture changes needed. The key insight is that by relinking constraints before
deleteByIdDirect, the cascade deletion no longer applies.Assumptions
rectangleToPolygonandellipseToPolygonoutputslocked-rectangleandlocked-ellipseendpoints are remapped;locked-polygonandlocked-datumpass through unchangedFixes #27