Skip to content

Allow point-and-click Offset Plane to work from sweep faces #7882

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

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

pierremtb
Copy link
Contributor

@pierremtb pierremtb commented Jul 22, 2025

Closes #7555 and https://github.com/KittyCAD/modeling-app/milestone/14

Refactors the codemod the same way we did sweeps, transforms, helix, shell as part of https://github.com/KittyCAD/modeling-app/milestone/4, with non-playwright test cases for default planes, offset plane on offset plane, and offset plane on cap.

Two things to resolve before merging:

  • the rust side could be cleaned up now that we have the plane IDs
  • the pre-selection gotcha for planes listed in the comments

Demo:

Screen.Recording.2025-07-23.at.2.53.37.PM.mov

Edit flows not working yet
Will eventually close #7555
Copy link

vercel bot commented Jul 22, 2025

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

Name Status Preview Comments Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2025 6:37pm

@@ -381,26 +381,23 @@ test.describe('Feature Tree pane', () => {
await cmdBar.expectState({
commandName: 'Offset plane',
stage: 'arguments',
currentArgKey: 'distance',
currentArgKey: 'offset',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this to be more in line with KCL

Comment on lines +1103 to +1104
const xz = await toolbar.getFeatureTreeOperation('Front plane', 0)
await xz.click()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And would like to keep these two tests as they aren't px dependent anymore

/**
* Append an offset plane to the AST
*/
export function addOffsetPlane({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to faces.ts with the addShell codemod since they will share a few utils

ast: modifiedAst,
call,
pathToEdit: nodeToEdit,
pathIfNewPipe: undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No pipe expr for offset planes for now

Comment on lines 62 to 64
const result = buildSolidsAndFacesExprs(
faces,
artifactGraph,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it a lil nicer for shell as well


console.log('Offset plane operation:', operation)

// TODO: replace with util function when available
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function from #7819

@pierremtb pierremtb changed the title WIP: Offset Plane point-and-click for sweep faces Offset Plane point-and-click for sweep faces Jul 23, 2025
@pierremtb pierremtb changed the title Offset Plane point-and-click for sweep faces Offset Plane point-and-click from sweep face Jul 23, 2025
@pierremtb pierremtb marked this pull request as ready for review July 23, 2025 18:56
@pierremtb pierremtb requested review from a team as code owners July 23, 2025 18:56
@pierremtb pierremtb changed the title Offset Plane point-and-click from sweep face Allow point-and-click Offset Plane to work from sweep face Jul 23, 2025
@pierremtb pierremtb changed the title Allow point-and-click Offset Plane to work from sweep face Allow point-and-click Offset Plane to work from sweep faces Jul 23, 2025
// 2. Prepare unlabeled and labeled arguments
let planeExpr: Expr | undefined
const hasFaceToOffset = plane.graphSelections.some(
(sel) => sel.artifact?.type === 'cap' || sel.artifact?.type === 'wall'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if we can or should generalize to other face types as well

@@ -686,12 +686,13 @@ export const modelingMachineCommandConfig: StateMachineCommandSetConfig<
},
plane: {
inputType: 'selection',
selectionTypes: ['plane'],
selectionTypes: ['plane', 'cap', 'wall'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here there's a bit of a gotcha where you need to pre-select the face, probably due to planes being made available for selection but haven't been able to investigate yet.

Copy link

codspeed-hq bot commented Jul 23, 2025

CodSpeed Instrumentation Performance Report

Merging #7882 will improve performances by 14.24%

Comparing pierremtb/issue7555-Add-ability-to-define-plane-offset-from-a-face-in-point-and-click (a58a1b5) with main (06362f9)

Summary

⚡ 1 improvements
✅ 88 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
mock_execute_mike_stress_test_program 302.6 ms 264.9 ms +14.24%

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

Successfully merging this pull request may close these issues.

Add planeOf artifacts to the graph Add ability to define plane offset from a face in point-and-click
2 participants