[SOLVE] #9273 Center and Corner rectangle tools #9657

andrewvarga · 2026-01-14T16:54:37Z

Implements the center and corner rectangle tools in sketch 2.0.

Still in draft:

There are a few open questions that I'm mentioning in the comments below
Need to add tests (we discussed to prefer unit tests over e2e @Irev-Dev let me know what you think)

… of the end points of lines

vercel · 2026-01-14T16:54:43Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
modeling-app	Ready	Preview, Comment	Jan 16, 2026 4:04pm

andrewvarga · 2026-01-14T18:24:07Z

src/machines/sketchSolve/tools/rectTool.ts

+        }
+      }
+    ),
+    askUserForDimensionValues: createMachine({


We're going to add the the option to set the dimensions explicitly in number inputs at the end of the process, but we probably need the dimension constraint, so for now it's not wired in yet.

andrewvarga · 2026-01-14T18:47:06Z

src/machines/sketchSolve/tools/rectUtils.ts

+    0,
+    sketchId,
+    ctor,
+    undefined, //'rectangle-segment',


Should the label be set? I just realized I set it to undefined during debugging an issue but this was not the reason.

Now it's set.

andrewvarga · 2026-01-14T18:47:14Z

src/machines/sketchSolve/tools/rectTool.ts

+          type: 'set draft entities',
+          data: {
+            // Only setting line ids instead of segmentIds
+            segmentIds: output.draft.lineIds,


@jtran @Irev-Dev I suspect this may be related to the rust side, if I send output.draft.segmentIds which contains line ids + point ids then when I call delete draft entities when pressing ESC before placing the second point, it fails with this error:

Failed to delete draft entities: Objectmessage: "Failed to delete objects in sketch: Error { msg: \"Source range not found: SourceRange([69, 150, 0])\" }"[[Prototype]]: Object deleteDraftEntities @ sketchSolveImpl.ts:831

The current way of sending only the line ids works but then the draft rectangle looks like this, ie. points are not rendered as draft/grey (because they are not added to the drafts).

We can either:

Make delete draft entities-> delete_objects() support point ids even if they are owned by lines: either by deleting the owner too or probably it's enough that it just skips owned points but still deletes the lines

Keep setting only the lines as draft, and add some special logic in rendering so that if the point's owner line is draft then the point is rendered as draft too.

Let me know what you think!

Is it always the case that when the frontend attempts to delete a point, it always deletes its owning line/arc/whatever? I think so. If that's the case, we should probably handle it in delete_objects() so that it can handle anything that's thrown at it. In there, before we start deleting:

Create an empty Vec of owner object IDs and another empty Vec of child object IDs

For each segment to delete:

If the segment is a point with an owner: add the segment to the child IDs; add the owner to the owner IDs

Remove all child IDs from the segments to delete

Add (i.e. union) all owner IDs to the segments to delete

We need to add to and remove from the segments to delete in a separate step so that we don't modify the set while iterating over it.

This sounds like maybe a bug in src/machines/sketchSolve/sketchSolveDiagram.ts with set draft entities, as in it probably shouldn't be sending the points. I get the same error when hitting esc before the second point for line, so maybe I never noticed the error.

Hmm I recorded this video to make a separate bug issue for it

Screenshare.-.2026-01-15.12_45_58.PM.mp4

But now I'm thinking it's related?

Can you make an issue for this?

@Irev-Dev Yes that's also caused by the same issue I believe.
We could also fix this by not sending the lines (TS only fix), but the side effect of that is that points would not be rendered in gray/draft colors unless we treat them in a special way by checking if their owner lines are draft.

I've added a fix in ec8cfaf and now deleting points of lines works, @jtran let me know if this is what you had in mind?

andrewvarga · 2026-01-14T19:37:44Z

src/machines/sketchSolve/tools/rectUtils.ts

+  const edits = [
+    {
+      id: line1Id,
+      ctor: makeLineSegmentCtor(start1, start2, units),
+    },
+    {
+      id: line2Id,
+      ctor: makeLineSegmentCtor(start2, start3, units),
+    },
+    {
+      id: line3Id,
+      ctor: makeLineSegmentCtor(start3, start4, units),
+    },
+    {
+      id: line4Id,
+      ctor: makeLineSegmentCtor(start4, start1, units),
+    },
+  ]


@jtran I could get this to work by editing the lines, the first version I did failed:
KittyCAD:fa22aea...KittyCAD:ac70c98#diff-98e1577bbbfe65a6586527d8fc7e55aa9b1669c1d3bb44756a534b615ad627b5R334-R351

This above branch doesn't add constraints to make the issue more obvious: the problem is that even though I think the edits array is correct the resulting KCL is not correct, the start points of lines are not set, only the end points:

I think this may be caused by a bug in edit_segments.

I'm not too familiar with this code but here is a theory (that could be wrong):

For a rectangle I send 8 points to edit_segments: 4 lines, each have a start + end point), edit_segments handles each point independently in a loop:

First point edit, eg. line1's start point

edit_point sees owner = line1

It clones line1's ctor from self.scene_graph

It updates start -> new mouse position

Calls edit_line -> this updates the AST with start = new start, end = existing end

Later in the same batch, we edit line1’s end point

edit_point runs again

It clones line1's ctor from self.scene_graph again, but this is still the old ctor because self.scene_graph is only updated after the loop finishes, so it updates the end correctly but the start value is overwritten with the original value and then it calls edit_line, overriding the first point edit (line1 start).

Yes, I think you're right. I'm not sure the best way to fix this that doesn't add a bunch of code for the different cases. The most straightforward way off the top of my head would be to add new variants to AstMutateCommand for EditLineStart and EditLineEnd. Then change edit_line() or refactor it somehow so that the code accepts either a LineCtor or an AstMutateCommand. Whatever we do, we'll need to do the same for arcs.

I was thinking one way to fix this would be to gather all points edits by their owner lines in edit_segment and then call edit_line instead of edit_point.
That may not be a general enough fix since edit_point could still be called from new code once with the start point and once with the end point, causing similar issues.

Also note that this fix here is not needed for the current code in the PR since I'm only editing the lines, it's only a problem if I switched to editing points - but I suspect this bug could cause problems down the line so maybe worth fixing?
Is there any disadvantage to editing the lines instead of the points?

From the Rust code's perspective, it would definitely be simpler if only the line were edited. But it seems that this is complexity that's necessary. It has to be dealt with somewhere.

To future proof it, in edit_segments(), we could try to have a pre-processing step where we collect all the points that have the same owner, and merge them into editing the owning line or arc. Then, when we loop through and do the actual edits, there will only be one call to edit_line(), editing both the start and end at the same time.

Would that satisfy everything?

@jtran Yes I think so. I've updated edit_segments with that preprocessing step and things seem to be working there (that branch is editing points directly without constraints so the issue is more apparent).
cd27803

If you like this I will add it to this PR.

andrewvarga · 2026-01-14T19:51:35Z

src/lib/toolbar.ts

+                    type: 'equip tool',
+                    data: { tool: 'cornerRectTool' },
+                  }),
+            icon: 'rectangle',


@franknoirot I'm thinking we could have separate icons for corner and center rectangles, like a simple dot in the center for the center rectangle and keep corner rectangle the same? It's a small thing I can do even but it would make it more obvious which one is currently selected.

I added a simple one in 92467e2 but let me know if you have something else in mind

Irev-Dev · 2026-01-15T00:51:40Z

src/machines/sketchSolve/sketchSolveImpl.ts

+      rectOriginMode:
+        nameOfToolToSpawn === 'centerRectTool' ? 'center' : 'corner',


[Change]:
I don't love this, just because we want this input to be generic for all tools, so rectOriginMode is not going to be used by most tools.

I think this just a naming issue, if we call it toolVarientName or subType I would be okay, that way we can use this for threePointArc as a varient of the arc tool etc.

Good call, I didn't like that too much either:
f88027b

Irev-Dev · 2026-01-15T01:08:33Z

src/machines/sketchSolve/tools/rectTool.ts

+          type: 'set draft entities',
+          data: {
+            // Only setting line ids instead of segmentIds
+            segmentIds: output.draft.lineIds,


This sounds like maybe a bug in src/machines/sketchSolve/sketchSolveDiagram.ts with set draft entities, as in it probably shouldn't be sending the points. I get the same error when hitting esc before the second point for line, so maybe I never noticed the error.

Irev-Dev · 2026-01-15T01:11:17Z

src/machines/sketchSolve/tools/rectUtils.ts

The pattern in so far for file names has been Diagram.ts for the Xstate code and Impl.ts for any utils, maybe worth changing to that?

Yes, so initially I had a cornerRectTool and a centerRectTool in mind that both share some common functions that's why I added the rectUtils, but as you also mentioned they are so similar there was no need for 2 tools and rectUtils got stuck.

I think the rectTool is still small enough so we can get away with just having a single file for it, ie. I can combine rectUtils into rectTool, similar to how pointTool is also a single file?

I kind of like the idea behind having a Diagram and Impl, but in the case of Line I found it a bit counter-productive to jump between the two files to understand how it works and sometimes I wasn't sure why a function is supposed to be in Diagram or Impl, eg modAndSolveFirstClick.

Irev-Dev · 2026-01-15T01:12:20Z

src/machines/sketchSolve/tools/rectUtils.ts

+  /**
+   *
+   * This is how the rectangle looks like when drawn from bottom-left to top-right corner direction:
+   *
+   *  start4, end3      start3, end2
+   *        o----line3----o
+   *        |             |
+   *      line4         line2
+   *        |             |
+   *        o----line1----o
+   *  start1, end4      start2, end1
+   *
+   */


Irev-Dev · 2026-01-15T01:19:39Z

src/machines/sketchSolve/tools/rectUtils.ts

+  return constraintId
+}
+
+export async function createDraftRectangle({


So this function makes 4 seperate rustContext function calls to create the 4 segments, than another 4 for the coincident constraints, and then more for the other constraints, each of these goes over the WASM bridge and each is a seperate call to the partical execution.

Maybe this is fast enough to not worry about, but thought I'd mention it and CC @jtran.

As for the line tool, because it chains the segments it has the same problem, but on a smaller scale, (create segement and create coincident constraint), we created rustContext.chainSegment(...), so it only crossed the wasm bridge once.

I see, are you thinking that if this becomes a performance problem we introduce a single wasm function that does all these steps in one go?
I suspect for this call site it's probably not an issue because we only create the rectangle once on the first click but it could become a problem if we used it on mouse move / something recurring.

Right. We generally try to minimize the number of times we cross the wasm boundary for performance. There's overhead in serializing and deserializing arguments and return values compared with a regular function call.

There's also a correctness concern that whenever an async call is done, state could be changed by something else running concurrently, interfering with our assumptions. (E.g. the KCL file changes on disk and we react to it, or maybe a previous execution finally resolves.) But the truth is, even code internal to Rust is async. We made it async in order to read imported KCL files by calling async JS, send engine messages, etc. So just making one Rust function doesn't completely solve the correctness concern.

I'm honestly a bit torn on this, as to whether it should be an expectation to implement serial api use as single wasm exposed wasm function or not.

Irev-Dev · 2026-01-15T01:24:31Z

Need to add tests (we discussed to prefer unit tests over e2e @Irev-Dev let me know what you think)

Yeah so I've added e2e/playwright/sketch-solve-edit.spec.ts as a sanity e2e test. The idea being we do have at least one e2e test for the new sketch mode to make sure it doesn't break entirely, but I haven't being doing e2e tests for the other tools for sketch mode so yeah I think unit/integration tests is good.

(e2e/playwright/sketch-solve-edit.spec.ts probably needs to be expanded, to cover a bit more than it does now (was created when we only had straight line segments for example) but think it should stay fairly minimal.)

codspeed-hq · 2026-01-15T21:48:34Z

Merging this PR will improve performance by 13.83%

⚡ 1 improved benchmark
✅ 151 untouched benchmarks
⏩ 93 skipped benchmarks¹

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	Simulation	`mock_execute_mike_stress_test_program`	366.8 ms	322.2 ms	+13.83%

_{Comparing andrewvarga/9273/center-and-corner-rectangles (98a5817) with main (f5883a0)²}

93 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
No successful run was found on main (4fcbf8c) during the generation of this report, so f5883a0 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

andrewvarga added 14 commits January 12, 2026 15:43

first steps of new tools: center and corner rectangles

3af0437

fix overlay icon for rectangle tools

5d28283

Center rectangle tool adding first point (not working yet)

f0f5098

center rectangle tool now draws a line successfully

b4666e3

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

028e7ca

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

5d2d7f5

center rect tool somewhat works now by editing lines directly instead…

67ff12a

… of the end points of lines

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

6bc7c72

center rectangle tool fixes

add2739

cleanups

cfc8323

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

cb42f95

clean up point editing

89d81e3

rectTool fixes

718b836

fmt

5dbaec3

andrewvarga requested a review from a team as a code owner January 14, 2026 16:54

vercel bot deployed to Preview January 14, 2026 17:10 View deployment

andrewvarga marked this pull request as draft January 14, 2026 18:03

andrewvarga commented Jan 14, 2026

View reviewed changes

andrewvarga added 2 commits January 14, 2026 19:38

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

a0365ec

small cleanups

8755727

andrewvarga requested review from Irev-Dev and jtran January 14, 2026 19:48

andrewvarga commented Jan 14, 2026

View reviewed changes

vercel bot deployed to Preview January 14, 2026 20:04 View deployment

pierremtb linked an issue Jan 14, 2026 that may be closed by this pull request

Centre and corner rectangles #9273

Open

Irev-Dev mentioned this pull request Jan 15, 2026

Toolbar rejig #9671

Open

Irev-Dev reviewed Jan 15, 2026

View reviewed changes

andrewvarga added 3 commits January 15, 2026 09:19

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

b6126b3

rename rectOriginMode -> toolVariant, introduce ToolInput type

f88027b

lint

2e20bb0

vercel bot deployed to Preview January 15, 2026 10:17 View deployment

andrewvarga added 2 commits January 15, 2026 16:29

Merge branch 'main' into andrewvarga/9273/center-and-corner-rectangles

646bcc0

fix delete_segments if both line and its points are deleted

ec8cfaf

vercel bot deployed to Preview January 15, 2026 21:46 View deployment

center rectangle icon added for center rectangle tool

92467e2

vercel bot deployed to Preview January 16, 2026 10:38 View deployment

andrewvarga added 3 commits January 16, 2026 15:35

add tests in rectTool.spec.ts

688b42e

renamings in rectTool

f15c486

collapse nested ifs into &&

98a5817

vercel bot deployed to Preview January 16, 2026 16:04 View deployment

		rectOriginMode:
		nameOfToolToSpawn === 'centerRectTool' ? 'center' : 'corner',

[SOLVE] #9273 Center and Corner rectangle tools #9657

Are you sure you want to change the base?

[SOLVE] #9273 Center and Corner rectangle tools #9657

Conversation

andrewvarga commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtran Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewvarga Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Irev-Dev commented Jan 15, 2026

Uh oh!

codspeed-hq bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will improve performance by 13.83%

Performance Changes

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

andrewvarga commented Jan 14, 2026 •

edited

Loading

vercel bot commented Jan 14, 2026 •

edited

Loading

jtran Jan 14, 2026 •

edited

Loading

andrewvarga Jan 16, 2026 •

edited

Loading

codspeed-hq bot commented Jan 15, 2026 •

edited

Loading