Sketch 2.0 WIP: add_segment stub #8594
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| type: 'update sketch outcome', | ||
| data: result, | ||
| }) | ||
|
|
There was a problem hiding this comment.
Bug: Improper Use of sendParent in Async Context
The modAndSolve actor directly calls sendParent within its fromPromise implementation. sendParent is an XState action creator, intended for machine actions, not direct calls inside async functions. This could prevent events from being sent to the parent or cause runtime issues.
| segment: SegmentCtor, | ||
| label: Option<String>, | ||
| ) -> Result<(SourceDelta, SceneGraphDelta)>; | ||
| ) -> Result<(KclSource, SketchExecOutcome)>; |
There was a problem hiding this comment.
This might be a temp change, if we want to stick with SceneGraphDelta?
I can see that NRC had made Sketch which is similar SketchExecOutcome
But is all ObjectIds, so guess I would still need a map of all of the objects?
There was a problem hiding this comment.
I think we should discuss this in our next call. I have a clearer understanding of it all now that I've implemented some of it, but I'm still on the fence about which way we should go.
Objects are analogous to runtime artifacts, i.e. those in the artifact graph. So it makes sense that if it's referencing segments, they would be ObjectIds pointing to them, not the Objects themselves. And yes, execution will build up a mapping from ObjectId to Object and provide a way to look things up.
CodSpeed Performance ReportMerging #8594 will not alter performanceComparing Summary
Footnotes
|
| #[ts(export)] | ||
| // TODO not sure if this needs to be file name and content? | ||
| pub struct KclSource { | ||
| pub text: String, |
There was a problem hiding this comment.
For the TODO, this is probably fine for now. We can't code-mod or refactor in imported files yet.
But the way I see it, this type and SourceDelta are redundant. They're the same and should be merged into one. I don't really care what we name it. Since this is the kcl-api crate, it seems a little redundant to call it "KCL source". But I get that the prefix probably makes it easier in TypeScript so that you don't need to alias it.
|
|
||
| #[derive(Debug, Clone, Deserialize, Serialize, ts_rs::TS)] | ||
| #[ts(export)] | ||
| pub enum ConstrainedStatus { |
There was a problem hiding this comment.
This type is redundant/should be merged with Freedom in this file above.
| wasm-pack build kcl-wasm-lib --release --target web --out-dir pkg | ||
| export RUSTFLAGS='' | ||
| cargo test -p kcl-lib --features artifact-graph export_bindings | ||
| cargo test -p kcl-api export_bindings |
There was a problem hiding this comment.
Do we need to do this for kcl-error also? I guess I'm confused how things are working at all now on main if we do need it.
2 participants
WIP, PR aims to use the pointTool to call the
add_segmentrust function, and get back solve data as the return.The function is stubbed out so does not do a code-mod or anything, but by returning fake data, we can start to draw the sketch client side.
Related to #8143