fix sketch mode scale issue (#6107)

* fix sketch mode scale issue

* fmt

* Fix snapshot tests in kurt-5621-scale-issue (#6111)

* Change test function to share units with settings (#6114)

* Change test function to share units with settings

* Consolidate to the same module

---------

Co-authored-by: Pierre Jacquier <pierrejacquier39@gmail.com>
Co-authored-by: Jonathan Tran <jonnytran@gmail.com>
Co-authored-by: Jess Frazelle <jessfraz@users.noreply.github.com>

e2e/playwright/editor-tests.spec.ts

@@ -985,12 +985,13 @@ sketch001 = startSketchOn(XZ)
   test(
     'Can undo a sketch modification with ctrl+z',
     { tag: ['@skipWin'] },
-    async ({ page, homePage }) => {
+    async ({ page, homePage, editor }) => {
       const u = await getUtils(page)
       await page.addInitScript(async () => {
         localStorage.setItem(
           'persistCode',
-          `sketch001 = startSketchOn(XZ)
+          `@settings(defaultLengthUnit=in)
+sketch001 = startSketchOn(XZ)
   |> startProfileAt([4.61, -10.01], %)
   |> line(end = [12.73, -0.09])
   |> tangentialArcTo([24.95, -0.38], %)
@@ -1080,41 +1081,45 @@ sketch001 = startSketchOn(XZ)
       await expect(page.locator('.cm-content')).not.toHaveText(prevContent)
 
       // expect the code to have changed
-      await expect(page.locator('.cm-content'))
+      await editor.expectEditor.toContain(
-        .toHaveText(`sketch001 = startSketchOn(XZ)
+        `sketch001 = startSketchOn(XZ)
     |> startProfileAt([2.71, -2.71], %)
     |> line(end = [15.4, -2.78])
     |> tangentialArcTo([27.6, -3.05], %)
     |> close()
-    |> extrude(length = 5)
+    |> extrude(length = 5)`,
-  `)
+        { shouldNormalise: true }
+      )
 
       // Hit undo
       await page.keyboard.down('Control')
       await page.keyboard.press('KeyZ')
       await page.keyboard.up('Control')
 
-      await expect(page.locator('.cm-content'))
+      await editor.expectEditor.toContain(
-        .toHaveText(`sketch001 = startSketchOn(XZ)
+        `sketch001 = startSketchOn(XZ)
     |> startProfileAt([2.71, -2.71], %)
     |> line(end = [15.4, -2.78])
     |> tangentialArcTo([24.95, -0.38], %)
     |> close()
-    |> extrude(length = 5)`)
+    |> extrude(length = 5)`,
+        { shouldNormalise: true }
+      )
 
       // Hit undo again.
       await page.keyboard.down('Control')
       await page.keyboard.press('KeyZ')
       await page.keyboard.up('Control')
 
-      await expect(page.locator('.cm-content'))
+      await editor.expectEditor.toContain(
-        .toHaveText(`sketch001 = startSketchOn(XZ)
+        `sketch001 = startSketchOn(XZ)
     |> startProfileAt([2.71, -2.71], %)
     |> line(end = [12.73, -0.09])
     |> tangentialArcTo([24.95, -0.38], %)
     |> close()
-    |> extrude(length = 5)
+    |> extrude(length = 5)`,
-  `)
+        { shouldNormalise: true }
+      )
 
       // Hit undo again.
       await page.keyboard.down('Control')
@@ -1122,13 +1127,15 @@ sketch001 = startSketchOn(XZ)
       await page.keyboard.up('Control')
 
       await page.waitForTimeout(100)
-      await expect(page.locator('.cm-content'))
+      await editor.expectEditor.toContain(
-        .toHaveText(`sketch001 = startSketchOn(XZ)
+        `sketch001 = startSketchOn(XZ)
     |> startProfileAt([4.61, -10.01], %)
     |> line(end = [12.73, -0.09])
     |> tangentialArcTo([24.95, -0.38], %)
     |> close()
-  |> extrude(length = 5)`)
+    |> extrude(length = 5)`,
+        { shouldNormalise: true }
+      )
     }
   )
 

e2e/playwright/fixtures/toolbarFixture.ts

@@ -10,6 +10,9 @@ import {
   openPane,
 } from '@e2e/playwright/test-utils'
 import { expect } from '@e2e/playwright/zoo-test'
+import { type baseUnitLabels } from '@src/lib/settings/settingsTypes'
+
+type LengthUnitLabel = (typeof baseUnitLabels)[keyof typeof baseUnitLabels]
 
 export class ToolbarFixture {
   public page: Page
@@ -236,6 +239,12 @@ export class ToolbarFixture {
   async checkIfFeatureTreePaneIsOpen() {
     return this.checkIfPaneIsOpen(this.featureTreeId)
   }
+  async selectUnit(unit: LengthUnitLabel) {
+    await this.page.getByTestId('units-menu').click()
+    const optionLocator = this.page.getByRole('button', { name: unit })
+    await expect(optionLocator).toBeVisible()
+    await optionLocator.click()
+  }
 
   /**
    * Get a specific operation button from the Feature Tree pane.

e2e/playwright/regression-tests.spec.ts

@@ -798,6 +798,74 @@ plane002 = offsetPlane(XZ, offset = -2 * x)`
       await page.getByTestId('custom-cmd-send-button').click()
     }
   )
+
+  test('scale other than default works with sketch mode', async ({
+    page,
+    homePage,
+    toolbar,
+    editor,
+    scene,
+  }) => {
+    await test.step('Load the washer code', async () => {
+      await page.addInitScript(async () => {
+        localStorage.setItem(
+          'persistCode',
+          `@settings(defaultLengthUnit = in)
+
+innerDiameter = 0.203
+outerDiameter = 0.438
+thicknessMax = 0.038
+thicknessMin = 0.024
+washerSketch = startSketchOn(XY)
+  |> circle(center = [0, 0], radius = outerDiameter / 2)
+
+washer = extrude(washerSketch, length = thicknessMax)`
+        )
+      })
+      await page.setBodyDimensions({ width: 1200, height: 500 })
+      await homePage.goToModelingScene()
+    })
+    const [circleCenterClick] = scene.makeMouseHelpers(650, 300)
+    const [circleRadiusClick] = scene.makeMouseHelpers(800, 320)
+    const [washerFaceClick] = scene.makeMouseHelpers(657, 286)
+
+    await page.waitForTimeout(100)
+    await test.step('Start sketching on the washer face', async () => {
+      await toolbar.startSketchPlaneSelection()
+      await washerFaceClick()
+      await page.waitForTimeout(600) // engine animation
+      await toolbar.expectToolbarMode.toBe('sketching')
+    })
+
+    await test.step('Draw a circle and verify code', async () => {
+      // select circle tool
+      await expect
+        .poll(async () => {
+          await toolbar.circleBtn.click()
+          return toolbar.circleBtn.getAttribute('aria-pressed')
+        })
+        .toBe('true')
+      await page.waitForTimeout(100)
+      await circleCenterClick()
+      // this number will be different if the scale is not set correctly for inches
+      await editor.expectEditor.toContain(
+        'circle(sketch001, center = [0.06, -0.06]'
+      )
+      await circleRadiusClick()
+
+      await editor.expectEditor.toContain(
+        'circle(sketch001, center = [0.06, -0.06], radius = 0.18'
+      )
+    })
+
+    await test.step('Exit sketch mode', async () => {
+      await toolbar.exitSketch()
+      await toolbar.expectToolbarMode.toBe('modeling')
+
+      await toolbar.selectUnit('Yards')
+      await editor.expectEditor.toContain('@settings(defaultLengthUnit = yd)')
+    })
+  })
 })
 
 async function clickExportButton(page: Page) {

e2e/playwright/sketch-tests.spec.ts

@@ -479,7 +479,8 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
     await page.addInitScript(async () => {
       localStorage.setItem(
         'persistCode',
-        `sketch001 = startSketchOn(XZ)
+        `@settings(defaultLengthUnit=in)
+sketch001 = startSketchOn(XZ)
     |> circle(center = [4.61, -5.01], radius = 8)`
       )
     })
@@ -564,12 +565,14 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
   test('Can edit a sketch that has been extruded in the same pipe', async ({
     page,
     homePage,
+    editor,
   }) => {
     const u = await getUtils(page)
     await page.addInitScript(async () => {
       localStorage.setItem(
         'persistCode',
-        `sketch001 = startSketchOn(XZ)
+        `@settings(defaultLengthUnit=in)
+sketch001 = startSketchOn(XZ)
   |> startProfileAt([4.61, -10.01], %)
   |> line(end = [12.73, -0.09])
   |> tangentialArcTo([24.95, -0.38], %)
@@ -654,26 +657,29 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
     await expect(page.locator('.cm-content')).not.toHaveText(prevContent)
 
     // expect the code to have changed
-    await expect(page.locator('.cm-content'))
+    await editor.expectEditor.toContain(
-      .toHaveText(`sketch001 = startSketchOn(XZ)
+      `sketch001 = startSketchOn(XZ)
     |> startProfileAt([7.12, -12.68], %)
     |> line(end = [12.68, -1.09])
     |> tangentialArcTo([24.89, 0.68], %)
     |> close()
-    |> extrude(length = 5)
+    |> extrude(length = 5)`,
-  `)
+      { shouldNormalise: true }
+    )
   })
 
   test('Can edit a sketch that has been revolved in the same pipe', async ({
     page,
     homePage,
     scene,
+    editor,
   }) => {
     const u = await getUtils(page)
     await page.addInitScript(async () => {
       localStorage.setItem(
         'persistCode',
-        `sketch001 = startSketchOn(XZ)
+        `@settings(defaultLengthUnit=in)
+sketch001 = startSketchOn(XZ)
   |> startProfileAt([4.61, -14.01], %)
   |> line(end = [12.73, -0.09])
   |> tangentialArcTo([24.95, -5.38], %)
@@ -758,14 +764,16 @@ sketch001 = startProfileAt([12.34, -12.34], sketch002)
     await expect(page.locator('.cm-content')).not.toHaveText(prevContent)
 
     // expect the code to have changed
-    await expect(page.locator('.cm-content'))
+    await editor.expectEditor.toContain(
-      .toHaveText(`sketch001 = startSketchOn(XZ)
+      `sketch001 = startSketchOn(XZ)
   |> startProfileAt([6.44, -12.07], %)
   |> line(end = [14.72, 1.97])
   |> tangentialArcTo([24.95, -5.38], %)
   |> line(end = [1.97, 2.06])
   |> close()
-  |> revolve(axis = "X")`)
+  |> revolve(axis = "X")`,
+      { shouldNormalise: true }
+    )
   })
   test('Can add multiple sketches', async ({ page, homePage }) => {
     const u = await getUtils(page)

e2e/playwright/snapshot-tests.spec.ts

@@ -455,7 +455,7 @@ test(
     await page.waitForTimeout(700) // TODO detect animation ending, or disable animation
 
     await page.mouse.click(startXPx + PUR * 10, 500 - PUR * 10)
-    code += `profile001 = startProfileAt([7.19, -9.7], sketch001)`
+    code += `profile001 = startProfileAt([182.59, -246.32], sketch001)`
     await expect(page.locator('.cm-content')).toHaveText(code)
     await page.waitForTimeout(100)
 
@@ -473,7 +473,7 @@ test(
     await page.waitForTimeout(500)
 
     code += `
-  |> xLine(length = 7.25)`
+  |> xLine(length = 184.3)`
     await expect(page.locator('.cm-content')).toHaveText(code)
 
     await page
@@ -631,7 +631,7 @@ test(
       mask: [page.getByTestId('model-state-indicator')],
     })
     await expect(page.locator('.cm-content')).toHaveText(
-      `sketch001 = startSketchOn(XZ)profile001 = circle(sketch001, center = [14.44, -2.44], radius = 1)`
+      `sketch001 = startSketchOn(XZ)profile001 = circle(sketch001, center = [366.89, -62.01], radius = 1)`
     )
   }
 )
@@ -668,7 +668,7 @@ test.describe(
 
       const startXPx = 600
       await page.mouse.click(startXPx + PUR * 10, 500 - PUR * 10)
-      code += `profile001 = startProfileAt([7.19, -9.7], sketch001)`
+      code += `profile001 = startProfileAt([182.59, -246.32], sketch001)`
       await expect(u.codeLocator).toHaveText(code)
       await page.waitForTimeout(100)
 
@@ -676,7 +676,7 @@ test.describe(
       await page.waitForTimeout(100)
 
       code += `
-  |> xLine(length = 7.25)`
+  |> xLine(length = 184.3)`
       await expect(u.codeLocator).toHaveText(code)
 
       await page
@@ -691,7 +691,7 @@ test.describe(
       await page.mouse.click(startXPx + PUR * 30, 500 - PUR * 20)
 
       code += `
-  |> tangentialArcTo([21.7, -2.44], %)`
+  |> tangentialArcTo([551.2, -62.01], %)`
       await expect(u.codeLocator).toHaveText(code)
 
       // click tangential arc tool again to unequip it

e2e/playwright/snapshots/prompt-to-edit/prompt-to-edit-snapshot-tests-spec-ts--edit-with-ai-example-snapshots--change-colour.snap.json

@@ -29,5 +29,5 @@
       }
     }
   ],
-  "kcl_version": "0.2.54"
+  "kcl_version": "0.2.57"
 }

src/clientSideScene/sceneInfra.ts

@@ -123,7 +123,6 @@ export class SceneInfra {
   readonly camControls: CameraControls
   private readonly fov = 45
   isFovAnimationInProgress = false
-  _baseUnit: BaseUnit = 'mm'
   _baseUnitMultiplier = 1
   _theme: Themes = Themes.System
   readonly extraSegmentTexture: Texture
@@ -154,7 +153,6 @@ export class SceneInfra {
     this.selected = null // following selections between callbacks being set is too tricky
   }
   set baseUnit(unit: BaseUnit) {
-    this._baseUnit = unit
     this._baseUnitMultiplier = baseUnitTomm(unit)
     this.scene.scale.set(
       this._baseUnitMultiplier,

src/components/UnitsMenu.tsx

@@ -26,6 +26,7 @@ export function UnitsMenu() {
       {({ close }) => (
         <>
           <Popover.Button
+            data-testid="units-menu"
             className={`flex items-center gap-2 px-3 py-1 
         text-xs text-primary bg-chalkboard-10/70 dark:bg-chalkboard-100/80 backdrop-blur-sm 
         border !border-primary/50 rounded-full`}

src/lang/KclSingleton.ts

@@ -4,6 +4,7 @@ import type {
   ModelingCmdReq_type,
 } from '@kittycad/lib/dist/types/src/models'
 
+import type { KclValue } from '@rust/kcl-lib/bindings/KclValue'
 import type { Node } from '@rust/kcl-lib/bindings/Node'
 import type { Operation } from '@rust/kcl-lib/bindings/Operation'
 
@@ -19,7 +20,6 @@ import { topLevelRange } from '@src/lang/util'
 import type {
   ArtifactGraph,
   ExecState,
-  KclValue,
   PathToNode,
   Program,
   SourceRange,
@@ -34,11 +34,17 @@ import {
 } from '@src/lang/wasm'
 import type { ArtifactIndex } from '@src/lib/artifactIndex'
 import { buildArtifactIndex } from '@src/lib/artifactIndex'
-import { EXECUTE_AST_INTERRUPT_ERROR_MESSAGE } from '@src/lib/constants'
+import {
+  DEFAULT_DEFAULT_LENGTH_UNIT,
+  EXECUTE_AST_INTERRUPT_ERROR_MESSAGE,
+} from '@src/lib/constants'
 import { markOnce } from '@src/lib/performance'
 import type { Selections } from '@src/lib/selections'
 import { handleSelectionBatch } from '@src/lib/selections'
-import type { KclSettingsAnnotation } from '@src/lib/settings/settingsTypes'
+import type {
+  BaseUnit,
+  KclSettingsAnnotation,
+} from '@src/lib/settings/settingsTypes'
 import { jsAppSettings } from '@src/lib/settings/settingsUtils'
 import {
   codeManager,
@@ -110,6 +116,7 @@ export class KclManager {
   private _diagnosticsCallback: (errors: Diagnostic[]) => void = () => {}
   private _wasmInitFailedCallback: (arg: boolean) => void = () => {}
   private _executeCallback: () => void = () => {}
+  sceneInfraBaseUnitMultiplierSetter: (unit: BaseUnit) => void = () => {}
 
   get ast() {
     return this._ast
@@ -780,6 +787,9 @@ export class KclManager {
 
   set fileSettings(settings: KclSettingsAnnotation) {
     this._fileSettings = settings
+    this.sceneInfraBaseUnitMultiplierSetter(
+      settings?.defaultLengthUnit || DEFAULT_DEFAULT_LENGTH_UNIT
+    )
   }
 }
 

src/lib/singletons.ts

@@ -7,6 +7,7 @@ import { uuidv4 } from '@src/lib/utils'
 
 import { SceneEntities } from '@src/clientSideScene/sceneEntities'
 import { SceneInfra } from '@src/clientSideScene/sceneInfra'
+import type { BaseUnit } from '@src/lib/settings/settingsTypes'
 
 export const codeManager = new CodeManager()
 
@@ -28,6 +29,9 @@ engineCommandManager.kclManager = kclManager
 
 export const sceneInfra = new SceneInfra(engineCommandManager)
 engineCommandManager.camControlsCameraChange = sceneInfra.onCameraChange
+kclManager.sceneInfraBaseUnitMultiplierSetter = (unit: BaseUnit) => {
+  sceneInfra.baseUnit = unit
+}
 
 // This needs to be after sceneInfra and engineCommandManager are is created.
 export const editorManager = new EditorManager()

src/machines/settingsMachine.ts

@@ -174,14 +174,6 @@ export const settingsMachine = setup({
     }),
   },
   actions: {
-    setClientSideSceneUnits: ({ context, event }) => {
-      const newBaseUnit =
-        event.type === 'set.modeling.defaultUnit'
-          ? (event.data.value as BaseUnit)
-          : context.modeling.defaultUnit.current
-      if (!sceneInfra) return
-      sceneInfra.baseUnit = newBaseUnit
-    },
     setEngineTheme: ({ context }) => {
       if (engineCommandManager && context.app.theme.current) {
         engineCommandManager
@@ -372,7 +364,7 @@ export const settingsMachine = setup({
   ],
   states: {
     idle: {
-      entry: ['setThemeClass', 'setClientSideSceneUnits', 'sendThemeToWatcher'],
+      entry: ['setThemeClass', 'sendThemeToWatcher'],
 
       on: {
         '*': {
@@ -415,12 +407,7 @@ export const settingsMachine = setup({
         'set.modeling.defaultUnit': {
           target: 'persisting settings',
 
-          actions: [
+          actions: ['setSettingAtLevel', 'toastSuccess', 'Execute AST'],
-            'setSettingAtLevel',
-            'toastSuccess',
-            'setClientSideSceneUnits',
-            'Execute AST',
-          ],
         },
 
         'set.app.theme': {
@@ -474,7 +461,6 @@ export const settingsMachine = setup({
             'resetSettings',
             'setThemeClass',
             'setEngineTheme',
-            'setClientSideSceneUnits',
             'setThemeColor',
             'Execute AST',
             'setClientTheme',
@@ -488,7 +474,6 @@ export const settingsMachine = setup({
             'setAllSettings',
             'setThemeClass',
             'setEngineTheme',
-            'setClientSideSceneUnits',
             'setThemeColor',
             'Execute AST',
             'setClientTheme',
@@ -549,7 +534,6 @@ export const settingsMachine = setup({
             'setAllSettings',
             'setThemeClass',
             'setEngineTheme',
-            'setClientSideSceneUnits',
             'setThemeColor',
             'setClientTheme',
             'setAllowOrbitInSketchMode',
@@ -579,7 +563,6 @@ export const settingsMachine = setup({
             'setAllSettings',
             'setThemeClass',
             'setEngineTheme',
-            'setClientSideSceneUnits',
             'setThemeColor',
             'Execute AST',
             'setClientTheme',