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

Add the runtime value of arguments to operations #5206

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

jtran
Copy link
Collaborator

@jtran jtran commented Jan 31, 2025

Resolves #5187. Stacked on #5205.

Using the full values made the size of Operations a lot bigger. We only include UUIDs, bools, numbers, ints, and strings. Other geometry like sketches only contain their artifact ID so that the UI can look it up in the artifact graph.

Copy link

qa-wolf bot commented Jan 31, 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 31, 2025

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

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Feb 12, 2025 0:03am

@franknoirot
Copy link
Collaborator

Are you concerned about the operations becoming a lot bigger?

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 90.29126% with 10 lines in your changes missing coverage. Please review.

Project coverage is 85.77%. Comparing base (c68e5d7) to head (00aae25).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/wasm-lib/kcl/src/execution/cad_op.rs 86.30% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5206      +/-   ##
==========================================
+ Coverage   85.76%   85.77%   +0.01%     
==========================================
  Files          94       94              
  Lines       34281    34369      +88     
==========================================
+ Hits        29400    29480      +80     
- Misses       4881     4889       +8     
Flag Coverage Δ
wasm-lib 85.77% <90.29%> (+0.01%) ⬆️

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.

This is great! I'm excited to check it out more thoroughly on Monday, looks really promising

@franknoirot
Copy link
Collaborator

This is a good addition, I think the artifactId's will help a lot for getting these other edit flows to work. One issue I see is with shell, which admittedly is probably one of the trickiest cases.

With the following KCL:

sketch001 = startSketchOn('XZ')
  |> circle({ center = [0, 0], radius = 174.66 }, %)
extrude001 = extrude(100, sketch001)
shell001 = shell({ faces = ['end'], thickness = 40 }, extrude001)

I get the following JS object for the operations:

[
    {
        "type": "StdLibCall",
        "name": "startSketchOn",
        "unlabeledArg": null,
        "labeledArgs": {
            "data": {
                "value": {
                    "type": "String",
                    "value": "XZ",
                    "__meta": [
                        {
                            "sourceRange": [
                                26,
                                30,
                                0
                            ]
                        }
                    ]
                },
                "sourceRange": [
                    26,
                    30,
                    0
                ]
            }
        },
        "sourceRange": [
            12,
            31,
            0
        ]
    },
    {
        "type": "StdLibCall",
        "name": "extrude",
        "unlabeledArg": null,
        "labeledArgs": {
            "length": {
                "value": {
                    "type": "Number",
                    "value": 100,
                    "__meta": [
                        {
                            "sourceRange": [
                                106,
                                109,
                                0
                            ]
                        }
                    ]
                },
                "sourceRange": [
                    106,
                    109,
                    0
                ]
            },
            "sketch_set": {
                "artifactIds": [
                    "487cfafc-927a-423d-9e6b-63ed3ac2dba0"
                ],
                "sourceRange": [
                    111,
                    120,
                    0
                ]
            }
        },
        "sourceRange": [
            98,
            121,
            0
        ]
    },
    {
        "type": "StdLibCall",
        "name": "shell",
        "unlabeledArg": null,
        "labeledArgs": {
            "data": {
                "sourceRange": [
                    139,
                    174,
                    0
                ]
            },
            "solid_set": {
                "artifactIds": [
                    "dac9e1e6-0852-475e-9464-f25146fb423d"
                ],
                "sourceRange": [
                    176,
                    186,
                    0
                ]
            }
        },
        "sourceRange": [
            133,
            187,
            0
        ]
    }
]

I think I can get to the proper selection of the face artifact using the artifactId for the solid_set value plus the faces value pulled using the sourceRange for the data argument, but it is still pretty intense. This is another +1 for your proposal for flatter stdlib functions. But I'm wondering if the data function could have an artifactId field like the solid_set field has? Then based on context clues I could know (at least in this case) that any artifactIds on the data argument would be the face artifacts, which are what I want.

@jtran jtran force-pushed the jtran/sketch-artifact-id branch from 07201da to 2bd5f8d Compare February 4, 2025 18:01
@jtran jtran force-pushed the jtran/operation-runtime-value branch from 5639dc6 to e284472 Compare February 4, 2025 18:02
@jtran jtran force-pushed the jtran/sketch-artifact-id branch 3 times, most recently from 45053f1 to 18d5980 Compare February 6, 2025 01:01
@jtran jtran force-pushed the jtran/operation-runtime-value branch from e284472 to fbfaaa7 Compare February 6, 2025 01:02
@jtran
Copy link
Collaborator Author

jtran commented Feb 6, 2025

It seems like you want the full nested structure, which I was trying to avoid, but doing that should provide all the info you could ever want. I duplicated the entire structure of KclValue, replacing the parts that the UI doesn't need with only artifactId. I call it OpKclValue.

This is what a shell operation looks like.

  {
    "labeledArgs": {
      "faces": {
        "value": {
          "type": "Array",
          "value": [
            {
              "type": "String",
              "value": "end"
            }
          ]
        },
        "sourceRange": [
          97,
          104,
          0
        ]
      },
      "thickness": {
        "value": {
          "type": "Number",
          "value": 0.25
        },
        "sourceRange": [
          118,
          122,
          0
        ]
      }
    },
    "name": "shell",
    "sourceRange": [
      83,
      123,
      0
    ],
    "type": "StdLibCall",
    "unlabeledArg": {
      "value": {
        "type": "Solid",
        "value": {
          "artifactId": "[uuid]"
        }
      },
      "sourceRange": [
        0,
        0,
        0
      ]
    }
  }

@jtran jtran force-pushed the jtran/operation-runtime-value branch from 58455b2 to 27a9405 Compare February 7, 2025 16:41
@jtran
Copy link
Collaborator Author

jtran commented Feb 7, 2025

I updated the example shell above since the move to keyword args. Note the source range of all zeros. It's because the solid was implicit in the pipeline. No % is required anymore, so it doesn't actually appear in the source.

@franknoirot
Copy link
Collaborator

I updated the example shell above since the move to keyword args. Note the source range of all zeros. It's because the solid was implicit in the pipeline. No % is required anymore, so it doesn't actually appear in the source.

This is super interesting. Another example, I think, of just how much geometry will have no code reference. Funny how kwargs is making that even more the case.

@jtran jtran force-pushed the jtran/operation-runtime-value branch from e90662f to 82c8a86 Compare February 7, 2025 22:03
@jtran jtran force-pushed the jtran/operation-runtime-value branch 2 times, most recently from 4af1eb9 to 6a0e9d1 Compare February 11, 2025 06:23
@jtran jtran marked this pull request as ready for review February 11, 2025 16:55
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.

Return KclValue for operations' arguments, and artifactId or KclValue for operation itself
2 participants