Selection dry-run validation for Shell #4775

pierremtb · 2024-12-12T21:27:48Z

Fixes #4711. Adds playwright test too for the failure case.

Will eventually fix #4711

qa-wolf · 2024-12-12T21:27:52Z

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-12-12T21:27:53Z

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Jan 16, 2025 6:55pm

…hell-selection-guards-for-circular-shapes

…guards-for-circular-shapes

…u-8-cores)

Fixes #4711

…u-8-cores)

lf94 · 2025-01-16T16:05:18Z

src/lib/commandBarConfigs/validators.ts

+
+  const shellCommand = async () => {
+    // TODO: figure out something better than an arbitrarily small value
+    const DEFAULT_THICKNESS: Models['LengthUnit_type'] = 1e-9


Ooof, yeah that's a tough one. I would have done this as a scale of the volume of the model's bounding box as a 1st attempt, but we don't have that information easily accessible.

lf94 · 2025-01-16T16:05:55Z

src/lib/commandBarConfigs/validators.ts

+    .find((v) => v.type === 'sweep')?.pathId
+
+  if (!object_id) {
+    return "Unable to shell, couldn't find the solid"


Don't want to return Promise.reject?

No, the arg configuration for validation takes a function that looks like

async function validator ({context, data}) : string | boolean {}

If you return true passes validation
If you return false fails validation but you don't get an error message
If you return string it fails validation but you get to see that message within the failure case.

This function is implemented correctly

modeling-app/src/machines/commandBarMachine.ts

Line 290 in 842ef5e

.then((result) => {

Here is the usage of the validator function

Now that I think of this I could have probably have thrown a new Error object with a string instead of saying "success" and returning a string with an error message...

It really isn't any error in the sense of the JS code.. I just need to know if an operation failed or passed then why it failed or passed.

The JS code is just proxying the engine result.

Yup, thanks for the added context @nadr0!

Right I was suggesting changing the type x) All good

src/lib/commandBarConfigs/validators.ts

nadr0 · 2025-01-16T18:04:35Z

Do we need this submodule commit?

Submodule kcl-samples added at 76d4a2

pierremtb · 2025-01-16T18:05:34Z

@nadr0 I was just looking at this on another branch. I have no idea what this is sorry, good catch!

…guards-for-circular-shapes

pierremtb · 2025-01-16T18:07:21Z

@nadr0 should be gone now

nadr0

Addressing some comments.

nadr0 · 2025-01-16T18:08:58Z

src/lib/commandBarConfigs/validators.ts

+    .find((v) => v.type === 'sweep')?.pathId
+
+  if (!object_id) {
+    return "Unable to shell, couldn't find the solid"


modeling-app/src/machines/commandBarMachine.ts

Line 290 in 842ef5e

.then((result) => {

Here is the usage of the validator function

src/lib/commandBarConfigs/validators.ts

nadr0 · 2025-01-16T18:10:54Z

src/lib/commandBarConfigs/validators.ts

+    .find((v) => v.type === 'sweep')?.pathId
+
+  if (!object_id) {
+    return "Unable to shell, couldn't find the solid"


Now that I think of this I could have probably have thrown a new Error object with a string instead of saying "success" and returning a string with an error message...

It really isn't any error in the sense of the JS code.. I just need to know if an operation failed or passed then why it failed or passed.

The JS code is just proxying the engine result.

src/lib/commandBarConfigs/validators.ts

nadr0 · 2025-01-16T18:35:46Z

I have been manually testing this to trigger the validation. I found an awesome shell result

# main.kcl
sketch001 = startSketchOn('XZ')
  |> startProfileAt([-0.12, -0.07], %)
  |> line([-0.04, 0.2], %, $seg02)
  |> line([0.32, 0.03], %, $seg01)
  |> yLine(-0.26, %)
  |> line([-0.14, -0.02], %)
  |> lineTo([profileStartX(%), profileStartY(%)], %)
  |> close(%)
extrude001 = extrude(5, sketch001)

Select this face and do the shell and you can see the result.

…guards-for-circular-shapes

nadr0

Thanks for addressing the comments, looks good.

pierremtb added 6 commits December 11, 2024 16:43

WIP: mess with shell selection validation

e4c5abe

Will eventually fix #4711

Update from main

05bb05d

WIP: not working yet

db9dac9

Update from main

f84eb3f

Working loft dry run validator

9410cec

Clean up shell (still not working)

826a01c

pierremtb requested a review from nadr0 December 12, 2024 21:29

vercel bot deployed to Preview December 12, 2024 21:32 View deployment

Bump kittycad-modeling-cmds

a32fbf9

pierremtb changed the title ~~WIP: shell and loft dry run validation~~ Add dry-run validation to Loft and Shell Dec 13, 2024

Clean up

bf4546e

vercel bot deployed to Preview December 13, 2024 14:36 View deployment

pierremtb added 2 commits December 13, 2024 11:23

Merge branch 'achalmers/bump-kcmc' into pierremtb/issue4711-Improve-s…

8246f4e

…hell-selection-guards-for-circular-shapes

Add logging

b28501b

vercel bot deployed to Preview December 13, 2024 16:41 View deployment

Add proper object_id and face_id mapping, still not working for shell

5c38faa

vercel bot deployed to Preview December 13, 2024 18:29 View deployment

pierremtb and others added 2 commits December 13, 2024 13:44

Fix faceId

c329bf9

A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

bc7995a

vercel bot deployed to Preview December 13, 2024 19:07 View deployment

pierremtb and others added 2 commits December 16, 2024 17:28

Merge branch 'main' into pierremtb/issue4711-Improve-shell-selection-…

3771c11

…guards-for-circular-shapes

A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

c5531d7

vercel bot deployed to Preview December 16, 2024 17:52 View deployment

Trigger CI

808f037

vercel bot deployed to Preview December 16, 2024 18:05 View deployment

A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

de70a67

vercel bot deployed to Preview December 16, 2024 18:17 View deployment

pierremtb mentioned this pull request Dec 16, 2024

Add dry-run validation for Loft #4820

Merged

pierremtb and others added 4 commits January 16, 2025 08:42

Trigger CI

902b33c

A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubunt…

57cc20c

…u-8-cores)

Trigger CI

9973c0a

Clean up

1ec571a

vercel bot deployed to Preview January 16, 2025 14:29 View deployment

pierremtb and others added 4 commits January 16, 2025 09:32

Fix tests

9c63a48

Add dry-run validation to Loft and Shell

5913dec

Fixes #4711

A snapshot a day keeps the bugs away! 📷🐛 (OS: namespace-profile-ubunt…

2e61adb

…u-8-cores)

Trigger CI

34aa753

vercel bot deployed to Preview January 16, 2025 15:07 View deployment

pierremtb marked this pull request as ready for review January 16, 2025 15:47

lf94 reviewed Jan 16, 2025

View reviewed changes

src/lib/commandBarConfigs/validators.ts Show resolved Hide resolved

lf94 reviewed Jan 16, 2025

View reviewed changes

src/lib/commandBarConfigs/validators.ts Show resolved Hide resolved

pierremtb and others added 2 commits January 16, 2025 13:06

Remove kcl-samples

503879d

Merge branch 'main' into pierremtb/issue4711-Improve-shell-selection-…

2a0ea07

…guards-for-circular-shapes

nadr0 reviewed Jan 16, 2025

View reviewed changes

vercel bot deployed to Preview January 16, 2025 18:30 View deployment

Merge branch 'main' into pierremtb/issue4711-Improve-shell-selection-…

9a05b0d

…guards-for-circular-shapes

vercel bot deployed to Preview January 16, 2025 18:55 View deployment

pierremtb mentioned this pull request Jan 16, 2025

Click UI - Shell doesn't work on a revolved surface #5087

Open

nadr0 approved these changes Jan 17, 2025

View reviewed changes

pierremtb merged commit 0592d3b into main Jan 17, 2025
30 of 31 checks passed

pierremtb deleted the pierremtb/issue4711-Improve-shell-selection-guards-for-circular-shapes branch January 17, 2025 15:16

pierremtb linked an issue Jan 17, 2025 that may be closed by this pull request

Click UI - Shell doesn't work on a revolved surface #5087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selection dry-run validation for Shell #4775

Selection dry-run validation for Shell #4775

pierremtb commented Dec 12, 2024 •

edited

Loading

qa-wolf bot commented Dec 12, 2024

vercel bot commented Dec 12, 2024 •

edited

Loading

lf94 Jan 16, 2025

lf94 Jan 16, 2025

nadr0 Jan 16, 2025

nadr0 Jan 16, 2025

nadr0 Jan 16, 2025

pierremtb Jan 16, 2025

lf94 Jan 17, 2025

nadr0 commented Jan 16, 2025

pierremtb commented Jan 16, 2025

pierremtb commented Jan 16, 2025

nadr0 left a comment

nadr0 Jan 16, 2025

nadr0 Jan 16, 2025

nadr0 commented Jan 16, 2025

nadr0 left a comment

Selection dry-run validation for Shell #4775

Selection dry-run validation for Shell #4775

Conversation

pierremtb commented Dec 12, 2024 • edited Loading

qa-wolf bot commented Dec 12, 2024

vercel bot commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadr0 commented Jan 16, 2025

pierremtb commented Jan 16, 2025

pierremtb commented Jan 16, 2025

nadr0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadr0 commented Jan 16, 2025

nadr0 left a comment

Choose a reason for hiding this comment

pierremtb commented Dec 12, 2024 •

edited

Loading

vercel bot commented Dec 12, 2024 •

edited

Loading