Skip to content
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

Rust artifact graph #5068

Merged

Conversation

jtran
Copy link
Collaborator

@jtran jtran commented Jan 15, 2025

Resolves #4860.

This moves the creation of the artifact graph from TS to Rust, which is returned as an output of KCL execution. Besides a minor rename (solid2D to solid2d), the actual graph should have zero changes. I did a direct port in order to preserve the current output and maintain the team's understanding.

Visualizations

Check out the new visualizations using Mermaid.

src/wasm-lib/kcl/tests/artifact_graph_example_code1/artifact_graph_flowchart.snap.md

flowchart LR
  subgraph path2 [Path]
    2["Path<br>[37, 64, 0]"]
    3["Segment<br>[70, 86, 0]"]
    4["Segment<br>[92, 119, 0]"]
    5["Segment<br>[125, 150, 0]"]
    6["Segment<br>[156, 203, 0]"]
    7["Segment<br>[209, 217, 0]"]
    8[Solid2d]
  end
  subgraph path25 [Path]
    25["Path<br>[352, 379, 0]"]
    26["Segment<br>[385, 400, 0]"]
    27["Segment<br>[406, 422, 0]"]
    28["Segment<br>[428, 475, 0]"]
    29["Segment<br>[481, 489, 0]"]
    30[Solid2d]
  end
  1["Plane<br>[12, 31, 0]"]
  9["Sweep Extrusion<br>[231, 254, 0]"]
  10[Wall]
  11[Wall]
  12[Wall]
  13[Wall]
  14["Cap Start"]
  15["Cap End"]
  16["SweepEdge Opposite"]
  17["SweepEdge Adjacent"]
  18["SweepEdge Opposite"]
  19["SweepEdge Adjacent"]
  20["SweepEdge Opposite"]
  21["SweepEdge Adjacent"]
  22["SweepEdge Opposite"]
  23["SweepEdge Adjacent"]
  24["EdgeCut Fillet<br>[260, 301, 0]"]
  31["Sweep Extrusion<br>[503, 524, 0]"]
  32[Wall]
  33[Wall]
  34[Wall]
  35["Cap End"]
  36["SweepEdge Opposite"]
  37["SweepEdge Adjacent"]
  38["SweepEdge Opposite"]
  39["SweepEdge Adjacent"]
  40["SweepEdge Opposite"]
  41["SweepEdge Adjacent"]
  1 --- 2
  2 --- 3
  2 --- 4
  2 --- 5
  2 --- 6
  2 --- 7
  2 ---- 9
  2 --- 8
  3 --- 13
  3 --- 22
  3 --- 23
  4 --- 12
  4 --- 20
  4 --- 21
  4 --- 24
  5 --- 11
  5 --- 18
  5 --- 19
  6 --- 10
  6 --- 16
  6 --- 17
  9 --- 10
  9 --- 11
  9 --- 12
  9 --- 13
  9 --- 14
  9 --- 15
  9 --- 16
  9 --- 17
  9 --- 18
  9 --- 19
  9 --- 20
  9 --- 21
  9 --- 22
  9 --- 23
  11 --- 25
  25 --- 26
  25 --- 27
  25 --- 28
  25 --- 29
  25 ---- 31
  25 --- 30
  26 --- 34
  26 --- 40
  26 --- 41
  27 --- 33
  27 --- 38
  27 --- 39
  28 --- 32
  28 --- 36
  28 --- 37
  31 --- 32
  31 --- 33
  31 --- 34
  31 --- 35
  31 --- 36
  31 --- 37
  31 --- 38
  31 --- 39
  31 --- 40
  31 --- 41
Loading

src/wasm-lib/kcl/tests/fillet-and-shell/artifact_graph_mind_map.snap.md

mindmap
  root
    Plane
      Path
        Segment
        Segment
        Segment
        Segment
        Solid2d
    Plane
      Path
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Sweep Extrusion
          Wall
          Wall
          Wall
          Wall
          Cap Start
          Cap End
          SweepEdge Opposite
          SweepEdge Adjacent
          SweepEdge Opposite
          SweepEdge Adjacent
          SweepEdge Opposite
          SweepEdge Adjacent
          SweepEdge Opposite
          SweepEdge Adjacent
        Solid2d
    Plane
      Path
      Path
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Sweep Extrusion
          Wall
          Cap Start
          Cap End
          SweepEdge Opposite
          SweepEdge Adjacent
        Solid2d
      Path
        Segment
        Solid2d
    Plane
      Path
      Path
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Sweep Extrusion
          Wall
          Cap Start
          Cap End
          SweepEdge Opposite
          SweepEdge Adjacent
        Solid2d
      Path
        Segment
        Solid2d
    Plane
      Path
      Path
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Sweep Extrusion
          Wall
          Cap Start
          Cap End
          SweepEdge Opposite
          SweepEdge Adjacent
        Solid2d
      Path
        Segment
        Solid2d
    Plane
      Path
      Path
        Segment
          Wall
          SweepEdge Opposite
          SweepEdge Adjacent
        Sweep Extrusion
          Wall
          Cap Start
          Cap End
          SweepEdge Opposite
          SweepEdge Adjacent
        Solid2d
      Path
        Segment
        Solid2d
Loading

Related issues:

The Mermaid representations of the artifact graph serve a few purposes:

  1. We now commit and regression test the artifact graph of all simulation tests, not just a few hand-created ones.
  2. Generating the graph image no longer requires a force simulation and a browser to run playwright. This means it doesn't trigger in CI, commit to your branch, and halt your PR until a manual push.
  3. The graph is no longer a PNG image. Now it's a textual Mermaid diagram whose diff can be easily examined in git.
  4. I hope that most of the team finds the new flowcharts easier to read. I put in extra effort to group paths into subgraphs and preserve the flow of information. For example, the flow in a KCL program always goes plane → path → sweep → edge cut. The flowchart should follow a similar flow, reading left-to-right. On the other hand, the mind map is a simplified overview. For simple designs, it can sometimes be easier to see, but it omits a lot.

KCL Program Output

The artifact graph is (part of) the output of a KCL program, so it's important that we both understand it and make sure we don't break it.

Data Flow

Instead of returning from WASM and building the artifact graph in TS, it's built in Rust on the WASM side before returning from KCL execution. This means that the artifact graph is part of ExecState which is cached on the Rust side.

Before:

flowchart LR
  ts[TS]
  rs[Rust in WASM]
  ws[WebSocket API Wrapper in TS]
  engine[Engine API]
  ag[Artifact Graph]@{ shape: doc }
  ac[Artifact Commands]@{ shape: doc }
  resp[Engine Responses]@{ shape: doc }
  ts -->|KCL| rs --> ws --> engine
  engine --> resp --> artifactGraph.ts
  rs --> ac --> artifactGraph.ts --> ag
Loading

After:

flowchart LR
  ts[TS]
  rs[Rust in WASM]
  ws[WebSocket API Wrapper in TS]
  engine[Engine API]
  artifact.rs[artifact.rs in WASM]
  ag[Artifact Graph]@{ shape: doc }
  ac[Artifact Commands]@{ shape: doc }
  resp[Engine Responses]@{ shape: doc }
  ts -->|KCL| rs --> ws --> engine
  engine --> resp --> artifact.rs
  rs --> ac --> artifact.rs --> ag
Loading

The only exception to this is that PathToNodes are all empty. The wrapper in wasm.ts immediately fills them in on the TS side.

Copy link

qa-wolf bot commented Jan 15, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Jan 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Jan 17, 2025 5:31pm

@jtran jtran force-pushed the jtran/rust-artifact-graph-episode-ii-ferris-strikes-back branch from e5a9326 to 177c078 Compare January 15, 2025 18:11
@jtran jtran force-pushed the jtran/rust-artifact-graph-episode-ii-ferris-strikes-back branch from 177c078 to 5d9ce98 Compare January 16, 2025 03:34
@jtran jtran force-pushed the jtran/rust-artifact-graph-episode-ii-ferris-strikes-back branch from 8095105 to 87283e0 Compare January 16, 2025 06:25
@jtran jtran marked this pull request as ready for review January 16, 2025 07:01
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 87.25389% with 123 lines in your changes missing coverage. Please review.

Project coverage is 86.09%. Comparing base (412e956) to head (cdb2141).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/wasm-lib/kcl/src/execution/artifact.rs 84.64% 78 Missing ⚠️
...sm-lib/kcl/src/execution/artifact/mermaid_tests.rs 87.65% 41 Missing ⚠️
src/wasm-lib/kcl/src/execution/mod.rs 87.50% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5068      +/-   ##
==========================================
+ Coverage   86.04%   86.09%   +0.04%     
==========================================
  Files          88       89       +1     
  Lines       31450    32389     +939     
==========================================
+ Hits        27062    27884     +822     
- Misses       4388     4505     +117     
Flag Coverage Δ
wasm-lib 86.09% <87.25%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@franknoirot franknoirot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking through the crux of the changes in wasm-lib, but during user testing I am seeing a similar behavior to the issue that arose last week, where sweeps were not being populated in the artifactGraph from code mods, but they appear in there after a refresh. Has this PR received the fixes you applied to main for this?

Screenshare.-.2025-01-16.12_48_42.PM-compressed.mp4

src/clientSideScene/sceneEntities.ts Outdated Show resolved Hide resolved
src/lang/KclSingleton.ts Show resolved Hide resolved
src/lang/std/engineConnection.ts Outdated Show resolved Hide resolved
src/lang/wasm.ts Outdated Show resolved Hide resolved

impl EdgeDirection {
#[must_use]
fn merge(&self, other: EdgeDirection) -> EdgeDirection {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[c] Neat! this is fun to read

@jtran jtran force-pushed the jtran/rust-artifact-graph-episode-ii-ferris-strikes-back branch from 12b5f2c to b3c87f5 Compare January 16, 2025 21:26
@jtran jtran force-pushed the jtran/rust-artifact-graph-episode-ii-ferris-strikes-back branch from 641b348 to 2cc2c91 Compare January 16, 2025 22:32
@jtran
Copy link
Collaborator Author

jtran commented Jan 16, 2025

@franknoirot, see the follow-up:

@jtran
Copy link
Collaborator Author

jtran commented Jan 17, 2025

I think I know what the problem is. We need to also save the engine responses in the execution cache.

@jtran
Copy link
Collaborator Author

jtran commented Jan 17, 2025

Cargo bench fix is here:

Copy link
Collaborator

@franknoirot franknoirot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior I raised an issue about is fixed, and the app is behaving as expected now. Excellent work on this wow!

@franknoirot franknoirot merged commit 0698432 into main Jan 17, 2025
33 of 34 checks passed
@franknoirot franknoirot deleted the jtran/rust-artifact-graph-episode-ii-ferris-strikes-back branch January 17, 2025 19:34
franknoirot added a commit that referenced this pull request Jan 17, 2025
pierremtb added a commit that referenced this pull request Feb 6, 2025
* Start implementing a "prepareToEdit" callback for extrude

* Start of generic edit flow for operations

* Actually invoking command bar send generically on double-click

* Refactor: break out non-React hook helper to calculate Kcl expression value

* Add unit tests, fmt

* Integrate helper to get calculated KclExpression

* Clean up unused imports, simplify use of `programMemoryFromVariables`

* Implement basic extrude editing

* Refactor: move DefaultPlanesStr to its own lib file

* Add support for editing offset planes

* Add Edit right-click menu option

* Turn off edit flow for sketch for now

* Add e2e tests for sketch and offset plane editing, fix bug found with offset plane editing

* Add failing e2e extrude edit test

* Remove action version of extrude AST mod

* Fix behavior when adding a constant while editing operation, fixing e2e test

* Patch in changes from 61b02b5

* Remove shell's prepareToEdit

* Add other Surface types to `artifactIsPlaneWithPaths`

* refactor: rename `item` to `operation`

* Allow `prepareToEdit` to fail with a toast, signal sketch-on-offset is unimplemented

* Rework sketch e2e test to test several working and failing cases

* Fix tsc errors related to making `codeRef` optional

* Make basic error messages more friendly

* fmt

* Reset modifyAst.ts to main

* Fix broken artifactGraph unit test

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* Remove unused import

* Look at this (photo)Graph *in the voice of Nickelback*

* Make the offset plane insert at the end, not one before

* Fix bug caught by e2e test failure with "Command needs review" logic

* Update src/machines/modelingMachine.ts

Co-authored-by: Pierre Jacquier <[email protected]>

* Remove console logs per @pierremtb

* Update src/components/CommandBar/CommandBarHeader.tsx

Co-authored-by: Jonathan Tran <[email protected]>

* Use better programMemory init thanks @jtran

* Fix tsc post merge of #5068

* Fix logic for `artifactIsPlaneWithPaths` post-merge

* Need to disable the sketch-on-face case now that artifactGraph is in Rust. Will active in a future PR (cc @jtran)

* Re-run CI after snapshots

* Update FeatureTreePane to not use `useCommandsContext`, missed during merge

* Fix merge issue, import location change on edited file

* fix click test step, which I believe is waiting for context scripts to load

* Convert toolbarFixture.exeIndicator to getter

We need to convert all these selectors on fixtures to getters, because
they can go stale if called on the fixture constructor.

* Missed a dumb little thing in toolbarFixture.ts

* Fix goof with merge

* fmt

* Another dumb missed thing during merge

I gotta get used to the LazyGit merge tool I'm not good at it yet

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* Conver sceneFixture's exeIndicator to a getter

Locators on fixtures will be frozen from the time of the fixture's
initialization, I'm increasingly convinced

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* Post-kwargs E2E test cleanup

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubuntu-8-cores)

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Pierre Jacquier <[email protected]>
Co-authored-by: Jonathan Tran <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move artifact graph creation into Rust
3 participants