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

[BUG]: Something happens with SketchGroups when returned via a map #3338

Closed
paultag opened this issue Aug 9, 2024 · 4 comments · Fixed by #3439
Closed

[BUG]: Something happens with SketchGroups when returned via a map #3338

paultag opened this issue Aug 9, 2024 · 4 comments · Fixed by #3439
Assignees
Labels
ast Issues / features relevant to ast and parser. bug Something isn't working kcl Language and compiler features

Comments

@paultag
Copy link
Collaborator

paultag commented Aug 9, 2024

Describe the bug

I'm able to return a SketchGroup from a function, but if it's inside an object, it doesn't behave the same as returning it directly.

Steps to Reproduce


fn test = () => {
  return startSketchOn('XY')
    |> startProfileAt([0, 0], %)
    |> line([0, 1], %)
    |> line([1, 0], %)
    |> line([0, -1], %)
    |> close(%)
}


fn test2 = () => {
  return {
    thing: startSketchOn('XY')
      |> startProfileAt([0, 0], %)
      |> line([0, 1], %)
      |> line([1, 0], %)
      |> line([0, -1], %)
      |> close(%)
  }
}

const x = test()

x
  |> extrude(-10, %)

const x2 = test2()

x2.thing
  |> extrude(10, %)

Argument at index 1 was supposed to be type kcl_lib::executor::SketchGroupSet but wasn't

Expected Behavior

I should see two rectanges, one going down, one going up. I only see one going down.

Screenshots and Recordings

Desktop OS

Linux

Browser

Chrome

Version

Version 127.0.6533.88 (Official Build) (64-bit)

Additional Context

No response

@paultag paultag added the bug Something isn't working label Aug 9, 2024
@paultag
Copy link
Collaborator Author

paultag commented Aug 9, 2024

image

@jtran
Copy link
Contributor

jtran commented Aug 9, 2024

I think I know what’s going on here. We move values around (in variables in ProgramMemory, Pipelines, etc.) using the MemoryItem type. When it’s just a SketchGroup, MemoryItem has an enum variant for that. But when it’s an Object, MemoryItem stuffs it in UserVal, which is a JSON value. So the SketchGroup gets serialized to JSON. The data is technically there, and we could jump through hoops to get it out. But I think we really need to stop storing values in JSON #1130 on the Rust side.

@jessfraz
Copy link
Contributor

jessfraz commented Aug 9, 2024

yeah i agree #1130 is the real fix, i have previously done the hoops and it would be nice to just fix this globally

@jessfraz
Copy link
Contributor

jessfraz commented Aug 9, 2024

cc @adamchalmers

@jessfraz jessfraz added kcl Language and compiler features ast Issues / features relevant to ast and parser. labels Aug 13, 2024
@jtran jtran self-assigned this Aug 14, 2024
adamchalmers added a commit that referenced this issue Aug 15, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 15, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 15, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 15, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 16, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 16, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 21, 2024
We can store Rust types like `SketchGroup` as their own variant of
`KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and
try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using
the other. If we stopped using ::UserVal, we'd need a new variant for
every Rust type we wanted to build, including user-defined types. So I
don't think that's practical.

Instead, we should store every KCL value by de/serializing it into
UserVal. This is a first step along that path, removing just the
SketchGroup variants. If it goes well, we can remove the other
specialized variants too.

My only concern is there might be performance implications from how
frequently we convert between serde_json::Value and Rust types via Serde.
But I'm not too worried -- there's no parsing JSON strings, just
traversing serde_json::Value trees. This isn't great for performance but
I think it'll probably be miniscule in comparison to doing all the API
calls. I'll benchmark it and see.
adamchalmers added a commit that referenced this issue Aug 21, 2024
We can store Rust types like `SketchGroup` as their own variant of `KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using the other. If we stopped using ::UserVal, we'd need a new variant for every Rust type we wanted to build, including user-defined types. So I don't think that's practical.

Instead, we should store every KCL value by de/serializing it into UserVal. This is a first step along that path, removing just the SketchGroup variants. If it goes well, we can remove the other specialized variants too.

My only concern is there might be performance implications from how frequently we convert between serde_json::Value and Rust types via Serde. But I'm not too worried -- there's no parsing JSON strings, just traversing serde_json::Value trees. This isn't great for performance but I think it'll probably be miniscule in comparison to doing all the API calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast Issues / features relevant to ast and parser. bug Something isn't working kcl Language and compiler features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants