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

Tags for entities #3

Open
Irev-Dev opened this issue Jul 14, 2023 · 23 comments
Open

Tags for entities #3

Irev-Dev opened this issue Jul 14, 2023 · 23 comments

Comments

@Irev-Dev
Copy link

There's a need to tag entities in the language to refer to them later, currently, that's done with strings so

const part001 = startSketchAt([-1.2, 4.83])
|> line([2.8, 0], %)
|> angledLine([45, 3.09], %)

might become

const part001 = startSketchAt([-1.2, 4.83])
|> line({ to: [2.8, 0], tag: 'seg01' }, %)
|> angledLine([45, segLen('seg01', %)], %)

There are a number of problems with this

  • It's just an arbitrary sequence of characters, it can't really be part of static analysis, miss-spelling would cause segLen('sEg01', %) to fail.
  • Someone could put the string in a variable, but it would be outside the pipe and so very ugly with multiple tags and still nothing stopping two tags from conflicting (same string without the user realising)
  • Maybe we make some easy way of accessing the tag off the pipe object segLen(%$seg01, %). $ is arbitrary but also a bit weird, maybe .. might be a better convention? %..seg01, maybe also weird that seg01 starts its life as what looks like a string and then changes to something that behaves more like a key.
  • Also with the transformation from line([2.8, 0], %) -> line({ to: [2.8, 0], tag: 'seg01' }, %) you can see that I'm trying to keep the line function to always taking two params, and adding in more optional params by changing the first param to a key-value pair, which in itself is kind of ugly.

With all of that said maybe attributes could work as a way of adding tags that's still compatible with pipeExpressions or not

const part001 = startSketchAt([-1.2, 4.83])
#[tag=seg01]
|> line([2.8, 0], %)
|> angledLine([45, segLen(%..seg01, %)], %)

And maybe there's a possibility of this having this part of the typing system i.e.

const part001 = startSketchAt([-1.2, 4.83])
|> line([2.8, 0], %)
|> angledLine([45, segLen(%..seg01, %)], %) // <- error trying to access a non-existant tag

It would know that the tag seg01 doesn't exist ahead of execution/calls to the engine?

This goes against something I said in #2, that attributes can be used for metadata not associated with modelling, but I think I can live with that.

@adamchalmers
Copy link
Collaborator

adamchalmers commented Jul 14, 2023

Yeah, I really like this idea!

One way to make it more ergonomic would be this convention: any constant implicitly has a default tag, which is the name of that constant. So if you left off the #[tag=seg01] from your example, it would use part001 as the tag.

BTW in all the API code I call the tag the "ID", so we should standardize so we don't go back and forth between tag and ID.

@adamchalmers
Copy link
Collaborator

Based on the discussion in #2 I think we're going to avoid attributes for now. But you can easily specify a part's tag via a keyword (or required positional) argument. I still like the idea that named constants would have default tags from their binding label.

@Irev-Dev
Copy link
Author

The intention I had with the example the tag was for the line was for the line call, so part001 wouldn't make sense in this case. Taging the line would be used to fillet that edge after it was extruded or similar.

The default tag could still work but would be for the (sketch in this case) entity as a whole instead of something within.
But yeah if we're not going with attributes that's cool.

I'll close this one.

@adamchalmers
Copy link
Collaborator

adamchalmers commented Jul 18, 2023

Let's leave this open and discuss tags more generally. Thanks for explaining, and also for your blog post sketching out these ideas a few years ago.

I think tag should be an optional keyword argument in many kinds of function. At least, as you suggest, the lineTo functions.

Type

Current KCL docs don't have an arbitrary String type, rather, a Text type which is specifically for putting text into your designs. I suggest we have a new type which is Tag, which works kinda like Ruby symbols. Symbols are:

  • No difference between name and contents. With strings, you can have different names and content e.g. user = "me"), but a symbol is just its name, there is no extra content. It's just user or just me, there's no difference between the name of the binding and the value being bound to the name.
  • Unique (every symbol can be defined only once)
  • Immutable (because there's no value being bound, there's no value to change)

Seems like a good fit for tags. When the AST runs semantic analysis, it can track every tag declaration and make sure every tag reference refers to a valid tag. This means we'll need a consistent way to recognize when tags are being set.

What can be tagged?

We want lines to be tagged, as in your sketch example. What about particular faces? E.g. if you want to sketch on top of a cylinder, do you tag the top face of the cylinder, and then reference that tag in your sketch? Josh explained to me yesterday that generally CAD either sketches on a plane or on a face of a solid. Maybe our sketches will start by taking a "what to sketch on" parameter, which is something like

enum WhatToSketchOn = X | Y | Z | Face(Tag)

i.e. you can sketch on any plane, or on some tagged face of a Solid3D.

Syntax proposal

WIP idea, so please criticize it.

To tag an entity, you pass a tag keyword argument into its constructor, e.g. |> lineTo(point(28, 0), %, tag = seg01). All keyword arguments are optional, so it doesn't change the type signature or force you to tag everything.

Functions which can refer to tags explicitly have a parameter of type tag somewhere, e.g.

// Function is declared like this
const angledLine = (angle: Angle, length: Literal(Distance) | LengthOf(Tag), start: Point) =>
    todo!()

Note that here I've declared an anonymous enum for the length parameter. You could change this to a properly declared enum type, but it seems convenient to have inline enum declaration for cases like this. Typescript has them, they seem useful, and if you ever need to reuse them elsewhere you can always factor them out into a proper, named type.

Here's how you'd actually invoke the function:

// Used like this
myLine1 = angledLine(Angle::d(45), Literal(Distance::cm(4)), (0,0))
// Or like this
myLine2 = angledLine(Angle::d(45), LengthOf(seg01), (0,0))

@adamchalmers adamchalmers changed the title Could attributes be used as a way to add tags to entities Tags for entities Jul 18, 2023
@franknoirot
Copy link

length: Literal(Distance) | LengthOf(Tag)

This might be nice for generated code, but I imagine it would be hard to remember as a learning designer/programmer that you have to include this kind of inline enum in the function declaration if you want to be able to refer to tagged values in your custom functions (sidenote: I do think we will likely see end-users refer to things like that as "custom" even though it's all kinda custom, just arbitrary code).

Might be a silly question, but does that inline enum need to be there? LengthOf(seg01) as it's being passed into angledLine() should resolve to a valid Literal(Distance) anyway if the compiler resolves that inner function call first, right?

@adamchalmers
Copy link
Collaborator

adamchalmers commented Jul 18, 2023 via email

@franknoirot
Copy link

franknoirot commented Jul 18, 2023

Regarding the question of what can be tagged, I think any geometry can be:

  • Point
  • Line
  • Plane / Face

To that end, I think it's worth thinking about the signature for functions like lengthOf(). My initial thought is this:

/// Find the length of Line given its Tag
const lengthOf = (Tag::Line -> Literal(Distance)) => 
    todo!()

So you couldn't pass in the Tag for a Point, because getting the length of a Point is nonsensical as far as I know. But I don't know if that Tag::Line syntax is right.

However I'm not sure how to enumerate which kinds of entity tags can be passed into a function; like are there functions that make sense for Points, Lines, Planes, and any other geometric entities I'm missing?

@adamchalmers
Copy link
Collaborator

adamchalmers commented Jul 18, 2023

The current language design has a few built-in traits e.g. number, comparable. We could add a new trait dimensional which is implemented by Solid2D, Solid3D, Path, PathSegment. Then you'd have functions like

const lengthOf = (shape: dimensional -> Distance) => todo!()
const heightOf = (shape: dimensional -> Distance) => todo!()

etc.

I don't know if "height" and "length" are meaningful concepts here because they kinda presuppose a view or frame of reference. How do users say that the 2x4 rectangle has length 2 depth 4 vs. depth 2, length 4?

@franknoirot
Copy link

I like the dimensional trait idea, but yeah that's a good point: I agree that length as it relates to height isn't meaningful for this, as they're both "lengths" just along different named axes.

I don't think there would be a heightOf() function, and lengthOf() would only refer to Lines, where there is no ambiguity about which dimension.

I'm beginning to think that we won't need a way to represent a function type signature that accepts more than one type of Tag.

@franknoirot
Copy link

franknoirot commented Jul 20, 2023

Been giving this a think today. I don't love the idea of having to pass in an argument for tags for two reasons:

  1. it's unclear to me if it's a universally-available named parameter or only available on certain kinds of functions
  2. it provides no way for us to tag geometry that isn't directly created by the operation, but implicitly as a side-effect

I've written up my thoughts on tagging these implicitly-created pieces of geometry here:

Summary

We will need an affordance to tag pieces of geometry (Points, Lines, and Planes/Faces) that were implicitly created by an operation for reference to use later in KCL programs.

Problem Statement

In all CAD programs, it is possible to create a new sketch on a face created from an extrusion command. Here is a very basic example recorded in Onshape:
https://github.com/KittyCAD/kcl/assets/23481541/5d1b0fd0-f8c1-4312-9327-b37619b29d35

Extrusions implicitly create a number of new Faces and Lines within the model, as do other operations such as a Boolean Subtraction, Intersection, or Exclusion. Currently we have no way in KCL to represent these implicitly-created pieces of geometry.

Screenshot 2023-07-20 at 2 29 47 PM

Proposed Solution

Since there could be any number of new faces or lines created in such operations, I propose that we create a deterministic rule set for how they are indexed, something like:

Objects created implicitly by operations are indexed starting at 0 with the first index representing the element with the high maximum Z value, increasing to n. For any sets of implicitly-created elements with equal maximum Z bounds, they are indexed in ascending order according to the angle their first point makes with the positive X-axis about the Z-axis. For any further ties, they are ordered in ascending order by the distance of their first point from the origin...

...and so on until it is impossible to create a non-deterministic arrangement of implicit geometry. I believe my example above has some problems because if the object is rotated or translated, so I'd like people smarter than me to help find a deterministic way to index.

We index implicitly-created Faces and Lines using a rubric like the one above into separate lists. And we allow all geometry-creating functions to tag any of its sub-elements using a tagFace and tagEdge function that takes an index.

Code example

width = 10.cm
length = 5.cm
height = 12.cm

// Also showing a JS feature of matched named parameters  not needing "width = width"
box = startSketch(0, 0)
  |> rectangle(center = [0, 0], width, height = length) 
  |> extrude(height)

sketchingFace = box.tagFace(2)
roundedEdge = box.tagEdge(3)

knob = let (
    center = [length / 2, height / 2]
    radius = 3.mm
  ) in startSketch(sketchingFace)
  |> circle(center, radius)

fullPart = union(box, knob)

Conclusion

We should give every returned value from a geometry-creating function access to .tagFace(index) and .tagLine(index) methods, so that geometry both implicit and explicit can be represented and tagged for later use.

If we have more ergonomic or robust ways of indexing into the faces and lines I would love that, but as a product of codegen I don't see a huge usability issue with having it just be an index integer. Our compiler could give index out-of-bound error messages and so on if things change further up the dependency graph.

@Irev-Dev
Copy link
Author

Irev-Dev commented Jul 20, 2023

This is kind of why I think the base primitive should always be a segment,
Sorry I really don't like indexes sketchingFace = box.tagFace(2) strikes me as something we should avoid if we can.
It's going to make things harder to read, more arbitrary conventions that you will have to painfully learn slowly.

and not anything more abstract like a rectangle or a box because of this problem. By keeping segments, as the base, it means we can always come back to tagging them when a user wants to build on a face.

And btw, I don't think this then limits our UI/UX, as there's no reason why clicking a rectangle tool and dragging it on a sketch plane can't produce,

startLoop()
|> xLine(5, tag:'seg01', %)
|> yLine(4, tag:'seg01', %)
|> xLine(-segLen(%..seg01), %)
|> yLine(-segLen(%..seg02), %)

@franknoirot
Copy link

Sorry I really don't like indexes sketchingFace = box.tagFace(2) strikes me as something we should avoid if we can.
It's going to make things harder to read, more arbitrary conventions that you will have to painfully learn slowly.

Yeah I hear you @Irev-Dev. I guess since this is a domain-specific language accompanied by a GUI I was hoping we could create UX that aids in understanding the indices for the edges and faces, possibly like so:
Screenshot 2023-07-20 at 8 29 30 PM

By keeping segments, as the base, it means we can always come back to tagging them when a user wants to build on a face.

I'm a little confused, how does your example work when a user wants to reference a bit of geometry that was created implicitly as part of another operation, like creating a second sketch on a face created as a result of an extrude?

@Irev-Dev
Copy link
Author

Nice, your UI for highlighting all the indexes available is dope, though I still disagree, I think it's good UI as a work-around that we don't need to do if we use tags.

So if you have segments as the base, once it gets extruded each segment becomes a surface so we can tie it back, lines will become planar faces, arcs will become cylindrical faces, splines or other more exotic segments still become their own surfaces. What's left out is the capping faces of the extrusion, but these are easy to identify still as the base/root/bottom and top/leaf of the extrusion. Here's how I imagine it working.

The user has already created 4 line segments, but using a rectangle tool. They hover over one of the lines that doesn't already have a tag, but that's not a concern. When they hover over it it shows them the line of code
image

Then they click, they are clicking an individual segment, but it also counts as selecting the sketch and so the extrude action is available and they do so.
image

The blue here is the code that was added from clicking extrude in the last frame, now the user is hovering over the face that came from the same segment they were hovering over before, so we highlight that same segment in the code (orange)
image

They click the face so we put the cursor on this line
image

Now they click that they want to sketch on that face, in order to reference it code, we don't only have to code-gen creating a new sketch, but we also add a tag to that segment so that we can reference it
here transformToWallExtrusion knows to look for the face that comes from the segment tagged seg03 inside of part01, and puts the sketch plan on top of it
image

From here it's back to normal sketch flow, user adds a circle and exits the sketch
image

Then extrudes it
image

Fin
image

I just realised that I had already built this flow already, so the drawings was not really worth the effort.

Screen.Recording.2023-07-21.at.1.50.09.pm.mp4

🤦‍♂️

@Irev-Dev
Copy link
Author

What do you think @franknoirot?

@Irev-Dev
Copy link
Author

Irev-Dev commented Jul 21, 2023

Oh and because there is still more working in regards to code-gen workflow with the old fake engine, I put up a PR with an old commit so that we can still play with the old thing with a vercel build.
KittyCAD/modeling-app#185

@franknoirot
Copy link

This makes sense @Irev-Dev! I think I could see a couple niceties for the codegen of the rectangle tool if it's generating segment-based code too.

I get this now, that if we have the originating segments to tag before an extrude we can have a reference to the face created by that segment during the extrude (a child in the dependency graph, I think).

How would this work in instances where the face being extruded from is not so cleanly mapped from the segment it was created by, or which was created by an operation that never involved segments directly?

For example, take a boolean subtraction of two cubes partially overlapping in space. Now the user wants to sketch on one of the faces created by the subtraction. How do they reference it?
Screenshot 2023-07-21 at 12 54 13 AM

By segments alone it seems convoluted. At least with a heuristic, winding order sort of indexing you can easily say "face n of the resulting shape".

@Irev-Dev
Copy link
Author

Irev-Dev commented Jul 21, 2023

What I really don't like about indexes is that it's now moving the source of truth away from being declarative in language to being something that's being stored in the engine, the user will only be able to tell which index is which with help from the engine displaying the index in the view that you had.

Sure it has a heuristic/winding order, but how likely are users to learn and understand it, and what happens when you instancing much more complex shapes, even like a bolt, figuring out which faces have which index without help from the engine sounds really difficult.
Am I wrong, you've given this more thought than me, how would you figure that out? or is it like the winding order of individual extrudes, so it would be the sketch name plus the winding order index?

If instead only using tags then you could go a long way without feedback from the view because it's declarative.

And I have not found a circumstance where faces will not have tags if you insist on segments being the base abstraction. here's an excerpt from CadHub
image

I'm sure you can see how that applies to your cube subtraction example
image

so long as cubes aren't just instanced out of thin air and are instead created by extruding a 4-segment loop then those faces can still be tagged, and will be transferred as part of the subtraction.

@Irev-Dev
Copy link
Author

In the case where a part has instances created in a loop then the faces can be a combination of the face's tag from the original shape, and then it's index from the loop part01[5]. I think this index is fine because it comes from the lang itself.

@franknoirot
Copy link

No you're right, this is more learnable and intuitive than a winding order.

I think the idea that you have to go all the way back to tag the line segment that created the face on the cutting object in order to reference a face like my example will be a surprising revelation for users, but you've convinced me that all created geometries can be represented this way.

@franknoirot
Copy link

franknoirot commented Jul 21, 2023

How does this function parameter implementation of tagging work when you want to create a function by abstracting, but still want entities to be "taggable"? You have to expose the tag parameters from each of the segment-creating function calls within it, don't you?

// particular declaration
part001 = startLoop()
  |> xLine(5, tag: 'seg01', %)
  |> yLine(4, tag: 'seg02', %)
  |> xLine(-segLen(%..seg01), tag: 'seg03', %)
  |> close(%)
  |> extrude(4,%)

// turning part001 into a general box-like function.
// I don't know how to use this in conjunction with tags
box = (width: Distance, length: Distance,  height: Distance) => startLoop()
  |> xLine(width, tag: 'seg01', %)   // <--- I don't know what to call these tag parameters in my new function
  |> yLine(length, tag: 'seg02', %)
  |> xLine(-segLen(%..seg01), tag: 'seg03', %)
  |> close(%)
  |> extrude(height, %)

@adamchalmers
Copy link
Collaborator

I'd name the tags tag1, ..., tag4, and describe in the docstring which edge they belong to. I can't imagine it'll make a lot of sense if it's just textual but with the visualisation it'll work.

@Irev-Dev
Copy link
Author

Irev-Dev commented Aug 7, 2023

I was supposed to come back to this.

One issue I'm still not sure about with tags is the case where you're importing geometry from a third party library, and they haven't tagged all of their segments, and let's say you want to extrude on one of the faces without a tag, in that case:

  • We use a winding order or similar
  • We have rules for publishing, and one of those is tags for each face (seems burdensome)
  • We have a really streamline way of patching 3rd party libs in a way that the patch stays local to your repo, but we also give a way to push that change back to the original lib, I wouldn't have considered this if I wasn't incredibly impressed by the DX of https://www.npmjs.com/package/patch-package, would be cool to make something like that a first-class citizen in the app

You're last comment @franknoirot, it's worth remembering that the % is just the sketch entity that is being built up with each expression in the pipeBody and so %..seg01 is no different to myVar..seg01 in a different context

box = (width: Distance, length: Distance,  height: Distance) => startLoop()
  |> xLine(width, tag: 'seg01', %)   // <--- I don't know what to call these tag parameters in my new function
  |> yLine(length, tag: 'seg02', %)
  |> xLine(-segLen(%..seg01), tag: 'seg03', %)
  |> close(%)
  |> extrude(height, %)
  
 const boxInstance = box(/* . . . */)
 const boxInstanceFilleted = FilletByTag(boxInstance..seg01, 3, boxInstance)
 
 const ExactSameButDoneInPipe =  box(/* . . . */)
   |> FilletByTag(%..seg01, 3, %)

That's maybe a bad example because maybe FilletByTag should be FilletExtrusionStartEdgeByTag or something similar since it might be talking about the top or bottom of the extrusion.

Does that answer the question, I might have missed the point 😬

@franknoirot
Copy link

No @Irev-Dev I think that does answer my question. That's pretty cool, although as you mentioned above it does rely on the function author to provide tags for each of the entities in a function, and for the function user to understand both a) that the tags exist and b) what their names are.

My point with the winding order is that it guarantees that every entity is "tagged" according to a well-defined ruleset, no matter how or when they are referenced. While I agree that it is harder to learn this ordering and keep its rules in your head, the UI is there to help you with that and, more importantly, it does not place the burden of creating well-named tags for every entity in a part that could be referenced by a future user on error-prone manual function authors. Like @adamchalmers said above:

I'd name the tags tag1, ..., tag4, and describe in the docstring which edge they belong to. I can't imagine it'll make a lot of sense if it's just textual but with the visualisation it'll work.

That sounds like winding order to me, just a function-author-specific one. I don't see how with this named tag approach we aren't asking function users to learn the internal tag naming conventions of every external function they call, and asking them to carefully design the tag naming conventions of their internally-authored functions to be both well-defined and flexible.

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

No branches or pull requests

3 participants