Show offset planes in the scene, let user select them #4481

franknoirot · 2024-11-13T18:18:42Z

Closes #4468.

Implements

Updates wasm-lib's implementation of offset_plane_inner to also call an engine-side effect to show a visible gray plane in the scene, and return the whole plane instead of just PlaneData
Updates start_sketch_on_inner's implementation to accept full Plane definitions in addition to PlaneData.
Hooks the returned plane into the ArtifactGraph on the client side
Hooks the new additional planes in the ArtifactGraph to be available for selection while attempting to sketch-on-face.
Adds a new ArtifactGraph test for the planes' addition, and a new Playwright test for selecting these planes for sketching.

Gotchas

Because we exclude plane type geometry from our default selection filter, these are not yet selectable on their own, so the user flow of "select plane -> start sketch" is not yet available.
These planes are always visible, and because of our limited notion of transparency, this will likely make models that use offset planes much more visibly noisy until we have a feature tree (Add a feature tree representation to the "code" pane #4274) that allows toggling planes' visibility. I would support a request to make these planes hidden except for during sketch plane selection (the "Sketch no face" state) until we have the feature tree implemented, or hiding offset planes that have sketches on them?

Future improvements

Selection behavior mentioned above
Visibility toggling mentioned above
Remove the engine-side scaling behavior of planes that these share with default planes

Demo

Screenshare.-.2024-11-13.1_24_12.PM-compressed.mp4

…click

qa-wolf · 2024-11-13T18:18:45Z

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!

vercel · 2024-11-13T18:18:46Z

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Nov 18, 2024 8:56pm

src/lang/modifyAst.ts

src/wasm-lib/kcl/src/std/sketch.rs

src/wasm-lib/kcl/src/std/planes.rs

…nues to work well for `startSketchOn`

jtran · 2024-11-13T19:11:04Z

src/lang/std/artifactGraph.ts

+  if (cmd.type === 'make_plane' && range[1] !== 0) {
+    // If we're calling `make_plane` and the code range doesn't end at `0`
+    // it's not a default plane, but a custom one from the offsetPlane standard library function


This seems pretty hacky. Can we add a flag when we create the default planes that indicate what they are?

I agree. I was going to say something that seems like a bit of a heuristic, which I'm not a fan of. The only reason why I didn't say anything is because I think this condition will be valid for a long time to come. Maybe add a comment on top that it's a heuristic?

Oh yeah I just remembered that we store the IDs of the default planes in the engine command manager! I can verify that the ID is none of those; would that be concrete enough?

Ah but that would bring a dependency into ArtifactGraph. It does kinda lead to the question: why aren't we storing the default planes in the ArtifactGraph?

Let's make an issue for this to remember to revisit later.

nadr0 · 2024-11-13T19:11:50Z

I ran the branch locally and tested out the new feature. It worked well locally. Left some comments in the code.

lf94

Looks good! Nothing glaring. All nits.

src/wasm-lib/kcl/src/std/sketch.rs

src/lang/modifyAst.ts

… last commit

@nadr0

…clear per @nadr0 and @lf94's feedback

src/wasm-lib/kcl/src/std/args.rs

This avoids the autocompletion issue I was having.

codecov · 2024-11-18T21:08:17Z

Codecov Report

Attention: Patch coverage is 76.03306% with 29 lines in your changes missing coverage. Please review.

Project coverage is 86.05%. Comparing base (fbb7b08) to head (3d3f7db).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/wasm-lib/kcl/src/std/args.rs	36.84%	24 Missing ⚠️
src/wasm-lib/kcl/src/lsp/tests.rs	62.50%	3 Missing ⚠️
src/wasm-lib/kcl/src/std/planes.rs	95.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4481      +/-   ##
==========================================
- Coverage   86.05%   86.05%   -0.01%     
==========================================
  Files          80       80              
  Lines       28762    28860      +98     
==========================================
+ Hits        24751    24835      +84     
- Misses       4011     4025      +14

Flag	Coverage Δ
wasm-lib	`86.05% <76.03%> (-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.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

franknoirot added 4 commits November 13, 2024 11:01

Update offset_plane to actually create and show the plane in-engine

ec2614f

Fix broken ability to use offsetPlanes in startSketchOn

e31cc71

Make the newly-visible offset planes usable for sketching via UI

3b71180

Add a playwright test for sketching on an offset plane via point-and-…

252dc86

…click

franknoirot requested review from Irev-Dev, jtran, lf94 and max-mrgrsk November 13, 2024 18:19

vercel bot deployed to Preview November 13, 2024 18:19 View deployment

franknoirot and others added 2 commits November 13, 2024 13:32

cargo clippy & cargo fmt

169c31c

Merge branch 'main' into franknoirot/4468/offset-planes-visible

1104683

nadr0 reviewed Nov 13, 2024

View reviewed changes

src/lang/modifyAst.ts Outdated Show resolved Hide resolved

nadr0 reviewed Nov 13, 2024

View reviewed changes

src/wasm-lib/kcl/src/std/sketch.rs Outdated Show resolved Hide resolved

vercel bot deployed to Preview November 13, 2024 18:57 View deployment

jtran reviewed Nov 13, 2024

View reviewed changes

src/wasm-lib/kcl/src/std/planes.rs Outdated Show resolved Hide resolved

jtran reviewed Nov 13, 2024

View reviewed changes

src/wasm-lib/kcl/src/std/planes.rs Outdated Show resolved Hide resolved

Make PlaneData the first item of SketchData so autocomplete conti…

746df3f

…nues to work well for `startSketchOn`

jtran reviewed Nov 13, 2024

View reviewed changes

lf94 approved these changes Nov 13, 2024

View reviewed changes

src/wasm-lib/kcl/src/std/sketch.rs Outdated Show resolved Hide resolved

src/lang/modifyAst.ts Outdated Show resolved Hide resolved

franknoirot added 2 commits November 13, 2024 14:17

@nadr0 feedback re: offsetIndex

d40806c

From @jtran: "Need to call the ID generator so that IDs are stable."

72478a8

vercel bot had a problem deploying to Preview November 13, 2024 19:34 Failure

More feedback from @jtran and fix incomplete use of id_generator in…

54fab4e

… last commit

vercel bot had a problem deploying to Preview November 13, 2024 19:47 Failure

Oops I missed saving isPathToNodeNumber earlier 🤦🏻

43abe39

vercel bot deployed to Preview November 13, 2024 21:19 View deployment

Make the distinction between Plane and PlaneOrientationData more …

aad4ff7

…clear per @nadr0 and @lf94's feedback

cargo fmt

bdfe9a5

vercel bot deployed to Preview November 14, 2024 21:22 View deployment

franknoirot added 2 commits November 15, 2024 10:50

Merge branch 'main' into franknoirot/4468/offset-planes-visible

7c77549

Remove log

6932490

vercel bot deployed to Preview November 15, 2024 16:19 View deployment

Print the unexpected diagnostics

99f09fd

vercel bot deployed to Preview November 15, 2024 16:38 View deployment

franknoirot and others added 5 commits November 15, 2024 15:41

Undo renaming of PlaneData

f9a8609

Remove generated PlaneRientationData docs page

809b69c

Redo doc generation after undoing PlaneData rename

b22dd8e

Impl FromKclValue for the new plane datatypes

7e1f854

Clippy lint

f59e191

vercel bot deployed to Preview November 15, 2024 21:19 View deployment

When starting a sketch, only hide the plane if it's a custom plane

4e8ee6e

vercel bot deployed to Preview November 15, 2024 21:40 View deployment

jtran added 2 commits November 18, 2024 11:06

Merge branch 'main' into franknoirot/4468/offset-planes-visible

9a75ff6

Fix FromKclValue and macro use since merge

30ab48e

vercel bot deployed to Preview November 18, 2024 16:24 View deployment

jtran mentioned this pull request Nov 18, 2024

Default planes should be distinguishable at runtime #4504

Open

jtran reviewed Nov 18, 2024

View reviewed changes

src/wasm-lib/kcl/src/std/args.rs Show resolved Hide resolved

Fix to not convert Plane to PlaneData

8cd0160

vercel bot deployed to Preview November 18, 2024 19:25 View deployment

Make sure offset planes are Custom type

05e56aa

jtran approved these changes Nov 18, 2024

View reviewed changes

vercel bot deployed to Preview November 18, 2024 20:15 View deployment

SketchData actually doesn't need to be in a certain order

3d3f7db

This avoids the autocompletion issue I was having.

vercel bot deployed to Preview November 18, 2024 20:56 View deployment

franknoirot merged commit 24bc4fc into main Nov 18, 2024
32 of 33 checks passed

franknoirot deleted the franknoirot/4468/offset-planes-visible branch November 18, 2024 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show offset planes in the scene, let user select them #4481

Show offset planes in the scene, let user select them #4481

franknoirot commented Nov 13, 2024 •

edited

Loading

qa-wolf bot commented Nov 13, 2024

vercel bot commented Nov 13, 2024 •

edited

Loading

jtran Nov 13, 2024

lf94 Nov 13, 2024

franknoirot Nov 13, 2024

franknoirot Nov 13, 2024

jtran Nov 18, 2024 •

edited

Loading

jtran Nov 18, 2024

nadr0 commented Nov 13, 2024

lf94 left a comment

codecov bot commented Nov 18, 2024 •

edited

Loading

Show offset planes in the scene, let user select them #4481

Show offset planes in the scene, let user select them #4481

Conversation

franknoirot commented Nov 13, 2024 • edited Loading

Implements

Gotchas

Future improvements

Demo

qa-wolf bot commented Nov 13, 2024

vercel bot commented Nov 13, 2024 • edited Loading

jtran Nov 13, 2024

Choose a reason for hiding this comment

lf94 Nov 13, 2024

Choose a reason for hiding this comment

franknoirot Nov 13, 2024

Choose a reason for hiding this comment

franknoirot Nov 13, 2024

Choose a reason for hiding this comment

jtran Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

jtran Nov 18, 2024

Choose a reason for hiding this comment

nadr0 commented Nov 13, 2024

lf94 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

franknoirot commented Nov 13, 2024 •

edited

Loading

vercel bot commented Nov 13, 2024 •

edited

Loading

jtran Nov 18, 2024 •

edited

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading