Add basic horizontal and vertical snapping for Line tool (#4465)

* Add on-click snapping behavior

* Fix math error with horizontal/vertical determination

* Move snap constant to where other sketch constants are

* fmt

* Fixing up some of the tests, some still need updating

* A couple other little PW test issues

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

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

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

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

* Fix remaining tests that need updating

* Make `yarn lint` happy

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

e2e/playwright/basic-sketch.spec.ts

@@ -67,15 +67,15 @@ async function doBasicSketch(page: Page, openPanes: string[]) {
   if (openPanes.includes('code')) {
     await expect(u.codeLocator).toHaveText(`sketch001 = startSketchOn('XZ')
   |> startProfileAt(${commonPoints.startAt}, %)
-  |> line([${commonPoints.num1}, 0], %)`)
+  |> xLine(${commonPoints.num1}, %)`)
   }
   await page.waitForTimeout(500)
   await page.mouse.click(startXPx + PUR * 20, 500 - PUR * 20)
   if (openPanes.includes('code')) {
     await expect(u.codeLocator).toHaveText(`sketch001 = startSketchOn('XZ')
   |> startProfileAt(${commonPoints.startAt}, %)
-  |> line([${commonPoints.num1}, 0], %)
+  |> xLine(${commonPoints.num1}, %)
-  |> line([0, ${commonPoints.num1 + 0.01}], %)`)
+  |> yLine(${commonPoints.num1 + 0.01}, %)`)
   } else {
     await page.waitForTimeout(500)
   }
@@ -84,9 +84,9 @@ async function doBasicSketch(page: Page, openPanes: string[]) {
   if (openPanes.includes('code')) {
     await expect(u.codeLocator).toHaveText(`sketch001 = startSketchOn('XZ')
   |> startProfileAt(${commonPoints.startAt}, %)
-  |> line([${commonPoints.num1}, 0], %)
+  |> xLine(${commonPoints.num1}, %)
-  |> line([0, ${commonPoints.num1 + 0.01}], %)
+  |> yLine(${commonPoints.num1 + 0.01}, %)
-  |> lineTo([0, ${commonPoints.num3}], %)`)
+  |> xLine(${commonPoints.num2 * -1}, %)`)
   }
 
   // deselect line tool
@@ -142,9 +142,9 @@ async function doBasicSketch(page: Page, openPanes: string[]) {
   await u.openKclCodePanel()
   await expect(u.codeLocator).toHaveText(`sketch001 = startSketchOn('XZ')
   |> startProfileAt(${commonPoints.startAt}, %)
-  |> line([${commonPoints.num1}, 0], %, $seg01)
+  |> xLine(${commonPoints.num1}, %, $seg01)
-  |> line([0, ${commonPoints.num1 + 0.01}], %)
+  |> yLine(${commonPoints.num1 + 0.01}, %)
-  |> angledLine([180, segLen(seg01)], %)`)
+  |> xLine(-segLen(seg01), %)`)
 }
 
 test.describe('Basic sketch', () => {

e2e/playwright/point-click.spec.ts

@@ -452,7 +452,7 @@ sketch002 = startSketchOn(extrude001, seg03)
   )
 })
 
-test(`Verify axis and origin snapping`, async ({
+test(`Verify axis, origin, and horizontal snapping`, async ({
   app,
   editor,
   toolbar,
@@ -505,7 +505,7 @@ test(`Verify axis and origin snapping`, async ({
   const expectedCodeSnippets = {
     sketchOnXzPlane: `sketch001 = startSketchOn('XZ')`,
     pointAtOrigin: `startProfileAt([${originSloppy.kcl[0]}, ${originSloppy.kcl[1]}], %)`,
-    segmentOnXAxis: `lineTo([${xAxisSloppy.kcl[0]}, ${xAxisSloppy.kcl[1]}], %)`,
+    segmentOnXAxis: `xLine(${xAxisSloppy.kcl[0]}, %)`,
     afterSegmentDraggedOffYAxis: `startProfileAt([${offYAxis.kcl[0]}, ${offYAxis.kcl[1]}], %)`,
     afterSegmentDraggedOnYAxis: `startProfileAt([${yAxisSloppy.kcl[0]}, ${yAxisSloppy.kcl[1]}], %)`,
   }

e2e/playwright/sketch-tests.spec.ts

@@ -115,7 +115,7 @@ test.describe('Sketch tests', () => {
         'persistCode',
         `sketch001 = startSketchOn('XZ')
   |> startProfileAt([4.61, -14.01], %)
-  |> line([12.73, -0.09], %)
+  |> xLine(12.73, %)
   |> tangentialArcTo([24.95, -5.38], %)`
       )
     })
@@ -156,7 +156,7 @@ test.describe('Sketch tests', () => {
       await expect.poll(u.normalisedEditorCode, { timeout: 1000 })
         .toBe(`sketch001 = startSketchOn('XZ')
   |> startProfileAt([12.34, -12.34], %)
-  |> line([-12.34, 12.34], %)
+  |> yLine(12.34, %)
 
 `)
     }).toPass({ timeout: 40_000, intervals: [1_000] })
@@ -645,7 +645,7 @@ test.describe('Sketch tests', () => {
     await u.openDebugPanel()
 
     const center = { x: viewportSize.width / 2, y: viewportSize.height / 2 }
-    const { toSU, click00r } = getMovementUtils({ center, page })
+    const { toSU, toU, click00r } = getMovementUtils({ center, page })
 
     await expect(
       page.getByRole('button', { name: 'Start Sketch' })
@@ -674,16 +674,15 @@ test.describe('Sketch tests', () => {
 
     await click00r(50, 0)
     await page.waitForTimeout(100)
-    codeStr += `  |> lineTo(${toSU([50, 0])}, %)`
+    codeStr += `  |> xLine(${toU(50, 0)[0]}, %)`
     await expect(u.codeLocator).toHaveText(codeStr)
 
     await click00r(0, 50)
-    codeStr += `  |> line(${toSU([0, 50])}, %)`
+    codeStr += `  |> yLine(${toU(0, 50)[1]}, %)`
     await expect(u.codeLocator).toHaveText(codeStr)
 
-    let clickCoords = await click00r(-50, 0)
+    await click00r(-50, 0)
-    expect(clickCoords).not.toBeUndefined()
+    codeStr += `  |> xLine(${toU(-50, 0)[0]}, %)`
-    codeStr += `  |> lineTo(${toSU(clickCoords!)}, %)`
     await expect(u.codeLocator).toHaveText(codeStr)
 
     // exit the sketch, reset relative clicker
@@ -712,15 +711,15 @@ test.describe('Sketch tests', () => {
     // TODO: I couldn't use `toSU` here because of some rounding error causing
     // it to be off by 0.01
     await click00r(30, 0)
-    codeStr += `  |> lineTo([4.07, 0], %)`
+    codeStr += `  |> xLine(2.04, %)`
     await expect(u.codeLocator).toHaveText(codeStr)
 
     await click00r(0, 30)
-    codeStr += `  |> line([0, -2.03], %)`
+    codeStr += `  |> yLine(-2.03, %)`
     await expect(u.codeLocator).toHaveText(codeStr)
 
     await click00r(-30, 0)
-    codeStr += `  |> line([-2.04, 0], %)`
+    codeStr += `  |> xLine(-2.04, %)`
     await expect(u.codeLocator).toHaveText(codeStr)
 
     await click00r(undefined, undefined)
@@ -744,8 +743,8 @@ test.describe('Sketch tests', () => {
 
       const code = `sketch001 = startSketchOn('-XZ')
     |> startProfileAt([${roundOff(scale * 69.6)}, ${roundOff(scale * 34.8)}], %)
-    |> line([${roundOff(scale * 139.19)}, 0], %)
+    |> xLine(${roundOff(scale * 139.19)}, %)
-    |> line([0, -${roundOff(scale * 139.2)}], %)
+    |> yLine(-${roundOff(scale * 139.2)}, %)
     |> lineTo([profileStartX(%), profileStartY(%)], %)
     |> close(%)`
 

e2e/playwright/snapshot-tests.spec.ts

@@ -462,7 +462,7 @@ test(
     await page.waitForTimeout(100)
 
     code += `
-  |> line([7.25, 0], %)`
+  |> xLine(7.25, %)`
     await expect(page.locator('.cm-content')).toHaveText(code)
 
     await page
@@ -647,7 +647,7 @@ test.describe(
       await page.waitForTimeout(100)
 
       code += `
-  |> line([7.25, 0], %)`
+  |> xLine(7.25, %)`
       await expect(u.codeLocator).toHaveText(code)
 
       await page
@@ -752,7 +752,7 @@ test.describe(
       await page.waitForTimeout(100)
 
       code += `
-  |> line([184.3, 0], %)`
+  |> xLine(184.3, %)`
       await expect(u.codeLocator).toHaveText(code)
 
       await page

e2e/playwright/test-network-and-connection-issues.spec.ts

@@ -141,7 +141,7 @@ test.describe('Test network and connection issues', () => {
     await expect(page.locator('.cm-content'))
       .toHaveText(`sketch001 = startSketchOn('XZ')
     |> startProfileAt(${commonPoints.startAt}, %)
-    |> line([${commonPoints.num1}, 0], %)`)
+    |> xLine(${commonPoints.num1}, %)`)
 
     // Expect the network to be up
     await expect(networkToggle).toContainText('Connected')
@@ -207,7 +207,7 @@ test.describe('Test network and connection issues', () => {
     await expect.poll(u.normalisedEditorCode)
       .toBe(`sketch001 = startSketchOn('XZ')
   |> startProfileAt([12.34, -12.34], %)
-  |> line([12.34, 0], %)
+  |> xLine(12.34, %)
   |> line([-12.34, 12.34], %)
 
 `)
@@ -217,9 +217,9 @@ test.describe('Test network and connection issues', () => {
     await expect.poll(u.normalisedEditorCode)
       .toBe(`sketch001 = startSketchOn('XZ')
   |> startProfileAt([12.34, -12.34], %)
-  |> line([12.34, 0], %)
+  |> xLine(12.34, %)
   |> line([-12.34, 12.34], %)
-  |> lineTo([0, -12.34], %)
+  |> xLine(-12.34, %)
 
 `)
 

e2e/playwright/test-utils.ts

@@ -305,7 +305,7 @@ export const getMovementUtils = (opts: any) => {
     return [last.x, last.y]
   }
 
-  return { toSU, click00r }
+  return { toSU, toU, click00r }
 }
 
 async function waitForAuthAndLsp(page: Page) {

e2e/playwright/testing-selections.spec.ts

@@ -32,10 +32,17 @@ test.describe('Testing selections', () => {
       await u.waitForAuthSkipAppStart()
       await u.openDebugPanel()
 
-      const xAxisClick = () =>
+      const yAxisClick = () =>
-        page.mouse.click(700, 253).then(() => page.waitForTimeout(100))
+        test.step('Click on Y axis', async () => {
+          await page.mouse.move(600, 200, { steps: 5 })
+          await page.mouse.click(600, 200)
+          await page.waitForTimeout(100)
+        })
       const xAxisClickAfterExitingSketch = () =>
-        page.mouse.click(639, 278).then(() => page.waitForTimeout(100))
+        test.step(`Click on X axis after exiting sketch, which shifts it at the moment`, async () => {
+          await page.mouse.click(639, 278)
+          await page.waitForTimeout(100)
+        })
       const emptySpaceHover = () =>
         test.step('Hover over empty space', async () => {
           await page.mouse.move(700, 143, { steps: 5 })
@@ -80,23 +87,23 @@ test.describe('Testing selections', () => {
       await expect(page.locator('.cm-content'))
         .toHaveText(`sketch001 = startSketchOn('XZ')
     |> startProfileAt(${commonPoints.startAt}, %)
-    |> line([${commonPoints.num1}, 0], %)`)
+    |> xLine(${commonPoints.num1}, %)`)
 
       await page.waitForTimeout(100)
       await page.mouse.click(startXPx + PUR * 20, 500 - PUR * 20)
       await expect(page.locator('.cm-content'))
         .toHaveText(`sketch001 = startSketchOn('XZ')
     |> startProfileAt(${commonPoints.startAt}, %)
-    |> line([${commonPoints.num1}, 0], %)
+    |> xLine(${commonPoints.num1}, %)
-    |> line([0, ${commonPoints.num1 + 0.01}], %)`)
+    |> yLine(${commonPoints.num1 + 0.01}, %)`)
       await page.waitForTimeout(100)
       await page.mouse.click(startXPx, 500 - PUR * 20)
       await expect(page.locator('.cm-content'))
         .toHaveText(`sketch001 = startSketchOn('XZ')
     |> startProfileAt(${commonPoints.startAt}, %)
-    |> line([${commonPoints.num1}, 0], %)
+    |> xLine(${commonPoints.num1}, %)
-    |> line([0, ${commonPoints.num1 + 0.01}], %)
+    |> yLine(${commonPoints.num1 + 0.01}, %)
-    |> lineTo([0, ${commonPoints.num3}], %)`)
+    |> xLine(${commonPoints.num2 * -1}, %)`)
 
       // deselect line tool
       await page.getByRole('button', { name: 'line Line', exact: true }).click()
@@ -121,29 +128,31 @@ test.describe('Testing selections', () => {
         // now check clicking works including axis
 
         // click a segment hold shift and click an axis, see that a relevant constraint is enabled
-        await topHorzSegmentClick()
-        await page.keyboard.down('Shift')
         const constrainButton = page.getByRole('button', {
           name: 'Length: open menu',
         })
-        const absYButton = page.getByRole('button', { name: 'Absolute Y' })
+        const absXButton = page.getByRole('button', { name: 'Absolute X' })
+
+        await test.step(`Select a segment and an axis, see that a relevant constraint is enabled`, async () => {
+          await topHorzSegmentClick()
+          await page.keyboard.down('Shift')
           await constrainButton.click()
-        await expect(absYButton).toBeDisabled()
+          await expect(absXButton).toBeDisabled()
           await page.waitForTimeout(100)
-        await xAxisClick()
+          await yAxisClick()
           await page.keyboard.up('Shift')
           await constrainButton.click()
-        await absYButton.and(page.locator(':not([disabled])')).waitFor()
+          await absXButton.and(page.locator(':not([disabled])')).waitFor()
-        await expect(absYButton).not.toBeDisabled()
+          await expect(absXButton).not.toBeDisabled()
+        })
 
-        // clear selection by clicking on nothing
         await emptySpaceClick()
-
         await page.waitForTimeout(100)
-        // same selection but click the axis first
+
-        await xAxisClick()
+        await test.step(`Same selection but click the axis first`, async () => {
+          await yAxisClick()
           await constrainButton.click()
-        await expect(absYButton).toBeDisabled()
+          await expect(absXButton).toBeDisabled()
           await page.keyboard.down('Shift')
           await page.waitForTimeout(100)
           await topHorzSegmentClick()
@@ -151,23 +160,26 @@ test.describe('Testing selections', () => {
 
           await page.keyboard.up('Shift')
           await constrainButton.click()
-        await expect(absYButton).not.toBeDisabled()
+          await expect(absXButton).not.toBeDisabled()
+        })
 
         // clear selection by clicking on nothing
         await emptySpaceClick()
 
         // check the same selection again by putting cursor in code first then selecting axis
+        await test.step(`Same selection but code selection then axis`, async () => {
           await page
-          .getByText(`  |> lineTo([0, ${commonPoints.num3}], %)`)
+            .getByText(`  |> xLine(${commonPoints.num2 * -1}, %)`)
             .click()
           await page.keyboard.down('Shift')
           await constrainButton.click()
-        await expect(absYButton).toBeDisabled()
+          await expect(absXButton).toBeDisabled()
           await page.waitForTimeout(100)
-        await xAxisClick()
+          await yAxisClick()
           await page.keyboard.up('Shift')
           await constrainButton.click()
-        await expect(absYButton).not.toBeDisabled()
+          await expect(absXButton).not.toBeDisabled()
+        })
 
         // clear selection by clicking on nothing
         await emptySpaceClick()
@@ -182,9 +194,7 @@ test.describe('Testing selections', () => {
           process.platform === 'linux' ? 'Control' : 'Meta'
         )
         await page.waitForTimeout(100)
-        await page
+        await page.getByText(`  |> xLine(${commonPoints.num2 * -1}, %)`).click()
-          .getByText(`  |> lineTo([0, ${commonPoints.num3}], %)`)
-          .click()
 
         await expect(page.locator('.cm-cursor')).toHaveCount(2)
         await page.waitForTimeout(500)
@@ -928,6 +938,7 @@ sketch002 = startSketchOn(extrude001, $seg01)
     // test fillet button with the body in the scene
     const codeToAdd = `${await u.codeLocator.allInnerTexts()}
 extrude001 = extrude(10, sketch001)`
+    await u.codeLocator.clear()
     await u.codeLocator.fill(codeToAdd)
     await selectSegment()
     await expect(page.getByRole('button', { name: 'Fillet' })).toBeEnabled()

src/clientSideScene/sceneEntities.ts

@@ -17,6 +17,7 @@ import {
   Vector3,
 } from 'three'
 import {
+  ANGLE_SNAP_THRESHOLD_DEGREES,
   ARROWHEAD,
   AXIS_GROUP,
   DRAFT_POINT,
@@ -95,6 +96,7 @@ import { CSS2DObject } from 'three/examples/jsm/renderers/CSS2DRenderer'
 import { Point3d } from 'wasm-lib/kcl/bindings/Point3d'
 import { SegmentInputs } from 'lang/std/stdTypes'
 import { Node } from 'wasm-lib/kcl/bindings/Node'
+import { radToDeg } from 'three/src/math/MathUtils'
 
 type DraftSegment = 'line' | 'tangentialArcTo'
 
@@ -798,11 +800,24 @@ export class SceneEntities {
             (sceneObject) => sceneObject.object.name === X_AXIS
           )
 
-          const lastSegment = sketch.paths.slice(-1)[0]
+          const lastSegment = sketch.paths.slice(-1)[0] || sketch.start
           const snappedPoint = {
             x: intersectsYAxis ? 0 : intersection2d.x,
             y: intersectsXAxis ? 0 : intersection2d.y,
           }
+          // Get the angle between the previous segment (or sketch start)'s end and this one's
+          const angle = Math.atan2(
+            snappedPoint.y - lastSegment.to[1],
+            snappedPoint.x - lastSegment.to[0]
+          )
+
+          const isHorizontal =
+            radToDeg(Math.abs(angle)) < ANGLE_SNAP_THRESHOLD_DEGREES ||
+            Math.abs(radToDeg(Math.abs(angle) - Math.PI)) <
+              ANGLE_SNAP_THRESHOLD_DEGREES
+          const isVertical =
+            Math.abs(radToDeg(Math.abs(angle) - Math.PI / 2)) <
+            ANGLE_SNAP_THRESHOLD_DEGREES
 
           let resolvedFunctionName: ToolTip = 'line'
 
@@ -810,6 +825,12 @@ export class SceneEntities {
           // case-based logic for different segment types
           if (lastSegment.type === 'TangentialArcTo') {
             resolvedFunctionName = 'tangentialArcTo'
+          } else if (isHorizontal) {
+            // If the angle between is 0 or 180 degrees (+/- the snapping angle), make the line an xLine
+            resolvedFunctionName = 'xLine'
+          } else if (isVertical) {
+            // If the angle between is 90 or 270 degrees (+/- the snapping angle), make the line a yLine
+            resolvedFunctionName = 'yLine'
           } else if (snappedPoint.x === 0 || snappedPoint.y === 0) {
             // We consider a point placed on axes or origin to be absolute
             resolvedFunctionName = 'lineTo'

src/clientSideScene/sceneInfra.ts

@@ -50,6 +50,8 @@ export const RAYCASTABLE_PLANE = 'raycastable-plane'
 
 export const X_AXIS = 'xAxis'
 export const Y_AXIS = 'yAxis'
+/** If a segment angle is less than this many degrees off a meanginful angle it'll snap to it */
+export const ANGLE_SNAP_THRESHOLD_DEGREES = 3
 /** the THREEjs representation of the group surrounding a "snapped" point that is not yet placed */
 export const DRAFT_POINT_GROUP = 'draft-point-group'
 /** the THREEjs representation of a "snapped" point that is not yet placed */