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

functions that operation on a tag should not require the % #2942

Closed
1 of 3 tasks
jessfraz opened this issue Jul 5, 2024 · 5 comments · Fixed by #3143
Closed
1 of 3 tasks

functions that operation on a tag should not require the % #2942

jessfraz opened this issue Jul 5, 2024 · 5 comments · Fixed by #3143
Assignees
Labels
enhancement New feature or request kcl Language and compiler features

Comments

@jessfraz
Copy link
Contributor

jessfraz commented Jul 5, 2024

One thing that annoys me about tags now that they are high level is they should map directly back to the uuid in engine so like getNextAdjacentEdge(bs.tags.edge7, %) should just be getNextAdjacentEdge(bs.tags.edge7) like you should not need the %

part of #2941 and making tags high level functions

@jessfraz jessfraz added enhancement New feature or request kcl Language and compiler features labels Jul 5, 2024
@jessfraz jessfraz self-assigned this Jul 5, 2024
@Irev-Dev
Copy link
Collaborator

Irev-Dev commented Jul 8, 2024

Yes, I agree! (and @max-mrgrsk will too)

It makes it much more cumbersome to use, I know you mentioned the %, but I would assert that having to include the variable and the tag is less of a pain within the same pipeExpression with the % when compared to matching up a tag and some other variable as can be the case for sketchOnFace, will also make the AstMods much easier since we don't need to track down the exact right variable to slot in.

Feels like a win on all fronts.

@max-mrgrsk
Copy link
Collaborator

totally agree! super nice idea and it looks so much cleaner

@jessfraz
Copy link
Contributor Author

jessfraz commented Jul 8, 2024

yeah I started on a PR for this, its going to be a bit because its more involved than i realized , it will get there but ill do it in small changes lol

@jtran
Copy link
Collaborator

jtran commented Jul 19, 2024

Can someone explain this to me? getNextAdjacdntEdge() uses the extrude group.

let tagged_path = extrude_group
.sketch_group
.get_path_by_tag(&tag)
.ok_or_else(|| {
KclError::Type(KclErrorDetails {
message: format!("No edge found with tag: `{}`", tag.value),
source_ranges: vec![args.source_range],
})
})?
.get_base();

Are you saying that we can skip this step using only the tag and prior information?

I also want to make sure that we're not breaking the rule that the pipe operator is syntax sugar to convert this:

a |> b(%) |> c(%) |> d(%)

to this:

d(c(b(a)))

@jessfraz
Copy link
Contributor Author

it will make this:

const width = 20
const length = 10
const thickness = 1
const chamferLength = 2

const mountingPlateSketch = startSketchOn("XY")
  |> startProfileAt([-width / 2, -length / 2], %)
  |> lineTo([width / 2, -length / 2], %, $edge1)
  |> lineTo([width / 2, length / 2], %, $edge2)
  |> lineTo([-width / 2, length / 2], %, $edge3)
  |> close(%, $edge4)

const mountingPlate = extrude(thickness, mountingPlateSketch)
  |> chamfer({
       length: chamferLength,
       tags: [
         getNextAdjacentEdge(edge1, %),
         getNextAdjacentEdge(edge2, %),
         getNextAdjacentEdge(edge3, %),
         getNextAdjacentEdge(edge4, %)
       ]
     }, %)

be this:

const width = 20
const length = 10
const thickness = 1
const chamferLength = 2

const mountingPlateSketch = startSketchOn("XY")
  |> startProfileAt([-width / 2, -length / 2], %)
  |> lineTo([width / 2, -length / 2], %, $edge1)
  |> lineTo([width / 2, length / 2], %, $edge2)
  |> lineTo([-width / 2, length / 2], %, $edge3)
  |> close(%, $edge4)

const mountingPlate = extrude(thickness, mountingPlateSketch)
  |> chamfer({
       length: chamferLength,
       tags: [
         getNextAdjacentEdge(edge1),
         getNextAdjacentEdge(edge2),
         getNextAdjacentEdge(edge3),
         getNextAdjacentEdge(edge4)
       ]
     }, %)

not changing the behvior of pipes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request kcl Language and compiler features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants