-
Notifications
You must be signed in to change notification settings - Fork 101
Sketch mode partial execution #9557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Merging this PR will improve performance by 91.7%
Performance Changes
Comparing Footnotes
|
1c3f2c3 to
8db6f45
Compare
8db6f45 to
50a16a3
Compare
4bfd370 to
775bc72
Compare
2639c74 to
52a7c21
Compare
52a7c21 to
af1ee95
Compare
af1ee95 to
61ab327
Compare
61ab327 to
22d2294
Compare
22d2294 to
d954f8d
Compare
f688970 to
50f2484
Compare
The plane Object ID in frontend tests is wrong.
Update test expectations
50f2484 to
12937c7
Compare
| match preserve_mem { | ||
| PreserveMem::Always => { | ||
| #[cfg(feature = "artifact-graph")] | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this change, what's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a statement is added to the end of a KCL file, we don't re-execute the whole file. We only execute that new statement.
When executing only the new statement(s) or mock executing for sketch mode, we need the scene objects that were created during the last execution, which are in the execution cache. The cache is read to create the initial module state. Depending on whether it's mock execution or engine execution, it's rehydrated differently. The preserve_mem parameter is another complication. Depending on it, we may use local_state or we may not.
I agree that this is confusing, even before this PR's changes. I researched it some to make #9511, but I still don't completely understand it. To be honest, I don't really know if I got this part all right, except the fact that the tests I made all pass. I can add comments explaining what I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
a4e769b to
b875fea
Compare
When editing a sketch, current
mainre-executes the KCL program from the beginning up to and including the sketch block. But this doesn't make sense because mock execution can't call the engine. So any code prior to the sketch block that depends on engine responses will always be wrong.This PR makes it so that engine execution properly caches needed scene objects in cached "memory" so that when we mock execute for sketch mode, we can mock execute only the sketch block and everything that's needed is available.
As part of this, we allow sketching on custom planes, i.e. from a KCL object or
offsetPlane(), and faces. But the user needs to assign it to a variable first. In particular, negative planes like-XYneed to be assigned to a variable first. Same with faces by using the newfaceOf()to sketch on the face. You need to assign it to a variable first.onparameter should be allowed to be any expression #9599Note: This works differently from TypeScript-implemented partial execution because that works by snipping the pipeline expression out of the AST and only executing that. On the other hand, this PR works by marking the sketch block AST node, and the executor skips nodes up to it and exits immediately afterwards.
Note
Enables targeted execution for sketch editing and updates geometry primitives to support it.
sketchblock; addMockConfig.sketch_block_id,ExecState.sketch_mode, and skip logic inexec_ast.rsSketchModeState(stack, module infos, andscene_objects) and wire into run paths and LSPscene_object_by_id(_mut)helpers; keep IDs stable when re-executingSketchCtor { on: String }, addonly_sketch_block, execute new sketch with real engine to seed cache, and support editing/deleting with sketch-modePlaneType→PlaneKind;Planenow tracks initialization via optionalobject_id; addensure_sketch_plane_in_engine; update std ops (planes,sketch,gdt,extrude,shapes) and type coercionssketch(on = <identifier>)(no arbitrary expressions)Written by Cursor Bugbot for commit ad4f8d3. This will update automatically on new commits. Configure here.