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

Sketches storing wrong path segment types #4297

Open
adamchalmers opened this issue Oct 24, 2024 · 0 comments
Open

Sketches storing wrong path segment types #4297

adamchalmers opened this issue Oct 24, 2024 · 0 comments

Comments

@adamchalmers
Copy link
Collaborator

adamchalmers commented Oct 24, 2024

Background

Sketches are composed of a "base path", then a sequence of 0 or more subsequent paths.

Paths have various types (e.g. arc, straight line, tangential arc).

Problem 1

Sketches do not properly store the type of path. Arcs are registered as Path::ToPoint, not Path::Arc. See this program:

startSketchOn('XZ')
|> startProfileAt([1, 2], %)
|> lineTo([5, 5], %)
|> arc({
     angleStart: -180.000000,
     angleEnd: -270.000000,
     radius: 4.000000
   }, %)

creates a Sketch like:

base:             [1.0, 2.0] to [1.0, 2.0]
path 0 (ToPoint): [1.0, 2.0] to [5.0, 5.0]
path 1 (ToPoint): [5.0, 5.0] to [9.0, 9.0]

Clearly the lineTo and arc should generate different kinds of path, but they're both ToPoint, which appears to be the "straight line" path type. There's no variant for Arc and there needs to be.

On the other hand, there's three different types for straight lines (ToPoint, AngledLineTo, Horizontal) where there probably only needs to be one. Kurt might know why these are different but my impression is this was all copied directly from the original JS-only untitled-app prototype.

Problem 2

When users tag a path, the tag stores a reference to "base path", i.e. just a (start, end) pair of 2D points. This should really be storing the type of path too, e.g. is it an arc, straight line, tangential arc, etc. Why? Because...

Problem 3

Functions like segLen() aka "segment length" cannot accurately calculate the length of a segment, because they take a tag, and as discussed in Problem 2, tagged paths just store a start/end, not a path type (arc, straight line, etc). Instead segLen assumes the path is a straight line, even if it's an arc. Clearly this is wrong and we have to solve problem 2 to solve this.

Problem 4

Why is "base path" even a type? It's used two ways:

  1. Every sketch has a base path called start, whose to and from fields are both just the start of the profile
  2. Every kind of path (arc, tangentialArc, straight line, etc) has a base field that stores where the start/end is

The first case can be eliminated, sketches can just store a start: Point2d field instead of start: BasePath.

The second case is weird. We're moving towards a model where paths don't always store their endpoint, because some of them will require (sigh) an API call or jumping out of WASM into JS then back into a different WASM (C++) then back to JS then back to KCL interpreter. So we're going to try to refactor paths so they don't always know their endpoints. And of course, the start of path n is just the end of path n-1 so it won't always make sense for paths to store their end either.

So we probably need to remove the base path type.

adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
See "Problem 2" in #4297

This is a pure refactor, it should not change any behaviour at all.
It adds more information into the tag system, but nothing reads that
extra information yet. It will be used to address problem 3 of the above
issue.
adamchalmers added a commit that referenced this issue Oct 25, 2024
See "Problem 2" in #4297

This is a pure refactor, it should not change any behaviour at all.
It adds more information into the tag system, but nothing reads that
extra information yet. It will be used to address problem 3 of the above
issue.
adamchalmers added a commit that referenced this issue Oct 25, 2024
See "Problem 2" in #4297

This is a pure refactor, it should not change any behaviour at all.
It adds more information into the tag system, but nothing reads that
extra information yet. It will be used to address problem 3 of the above
issue.
adamchalmers added a commit that referenced this issue Oct 25, 2024
See "Problem 2" in #4297

This is a pure refactor, it should not change any behaviour at all.
It adds more information into the tag system, but nothing reads that
extra information yet. It will be used to address problem 3 of the above
issue.
adamchalmers added a commit that referenced this issue Oct 25, 2024
See "Problem 2" in #4297

This is a pure refactor, it should not change any behaviour at all.
It adds more information into the tag system, but nothing reads that
extra information yet. It will be used to address problem 3 of the above
issue.
adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
adamchalmers added a commit that referenced this issue Oct 25, 2024
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

1 participant