Skip to content
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

Add menu item and hotkey to center view on current selection #4068

Merged
merged 17 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion e2e/playwright/fixtures/sceneFixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ type mouseParams = {
pixelDiff: number
}

type SceneSerialised = {
camera: {
position: [number, number, number]
target: [number, number, number]
}
}

export class SceneFixture {
public page: Page

Expand All @@ -22,6 +29,22 @@ export class SceneFixture {
this.page = page
this.reConstruct(page)
}
private _serialiseScene = async (): Promise<SceneSerialised> => {
const camera = await this.getCameraInfo()

return {
camera,
}
}

expectState = async (expected: SceneSerialised) => {
return expect
.poll(() => this._serialiseScene(), {
message: `Expected scene state to match`,
})
.toEqual(expected)
}

reConstruct = (page: Page) => {
this.page = page

Expand All @@ -31,7 +54,7 @@ export class SceneFixture {
makeMouseHelpers = (
x: number,
y: number,
{ steps }: { steps: number } = { steps: 5000 }
{ steps }: { steps: number } = { steps: 20 }
) =>
[
(clickParams?: mouseParams) => {
Expand Down Expand Up @@ -87,6 +110,36 @@ export class SceneFixture {
)
await closeDebugPanel(this.page)
}
/** Forces a refresh of the camera position and target displayed
* in the debug panel and then returns the values of the fields
*/
async getCameraInfo() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love our new fixture stuff :')

await openAndClearDebugPanel(this.page)
await sendCustomCmd(this.page, {
type: 'modeling_cmd_req',
cmd_id: uuidv4(),
cmd: {
type: 'default_camera_get_settings',
},
})
await this.waitForExecutionDone()
const position = await Promise.all([
this.page.getByTestId('cam-x-position').inputValue().then(Number),
this.page.getByTestId('cam-y-position').inputValue().then(Number),
this.page.getByTestId('cam-z-position').inputValue().then(Number),
])
const target = await Promise.all([
this.page.getByTestId('cam-x-target').inputValue().then(Number),
this.page.getByTestId('cam-y-target').inputValue().then(Number),
this.page.getByTestId('cam-z-target').inputValue().then(Number),
])
await closeDebugPanel(this.page)
return {
position,
target,
}
}

waitForExecutionDone = async () => {
await expect(this.exeIndicator).toBeVisible()
}
Expand Down Expand Up @@ -114,4 +167,17 @@ export class SceneFixture {
)
})
}

get gizmo() {
return this.page.locator('[aria-label*=gizmo]')
}

async clickGizmoMenuItem(name: string) {
await this.gizmo.click({ button: 'right' })
const buttonToTest = this.page.getByRole('button', {
name: name,
})
await expect(buttonToTest).toBeVisible()
await buttonToTest.click()
}
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
96 changes: 74 additions & 22 deletions e2e/playwright/testing-gizmo.spec.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import { test, expect } from '@playwright/test'

import { _test, _expect } from './playwright-deprecated'
import { test } from './fixtures/fixtureSetup'
Copy link
Contributor

Choose a reason for hiding this comment

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

I mustve missed the memo - what's going on here? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll undo this change in a separate PR per Slack discussion

import { getUtils, setup, tearDown } from './test-utils'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we revert this name change and do something like

import { test, expect } from '@playwright/test'
import { test as fixtureSetupTest } from './fixtures/fixtureSetup'

I am confused to why we have a './playwright-deprecated' when we can just do alias importing?

Copy link
Collaborator

@nadr0 nadr0 Oct 4, 2024

Choose a reason for hiding this comment

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

After talking with the team, you do not have to fix this within this PR. We can address this importing stuff on the web team. This pattern is fine within this file for this PR.

import { uuidv4 } from 'lib/utils'
import { TEST_CODE_GIZMO } from './storageStates'

test.beforeEach(async ({ context, page }, testInfo) => {
_test.beforeEach(async ({ context, page }, testInfo) => {
await setup(context, page, testInfo)
})

test.afterEach(async ({ page }, testInfo) => {
_test.afterEach(async ({ page }, testInfo) => {
await tearDown(page, testInfo)
})

test.describe('Testing Gizmo', () => {
_test.describe('Testing Gizmo', () => {
const cases = [
{
testDescription: 'top view',
Expand Down Expand Up @@ -57,7 +57,7 @@ test.describe('Testing Gizmo', () => {
expectedCameraTarget,
testDescription,
} of cases) {
test(`check ${testDescription}`, async ({ page, browserName }) => {
_test(`check ${testDescription}`, async ({ page, browserName }) => {
const u = await getUtils(page)
await page.addInitScript((TEST_CODE_GIZMO) => {
localStorage.setItem('persistCode', TEST_CODE_GIZMO)
Expand Down Expand Up @@ -117,30 +117,30 @@ test.describe('Testing Gizmo', () => {

await Promise.all([
// position
expect(page.getByTestId('cam-x-position')).toHaveValue(
_expect(page.getByTestId('cam-x-position')).toHaveValue(
expectedCameraPosition.x.toString()
),
expect(page.getByTestId('cam-y-position')).toHaveValue(
_expect(page.getByTestId('cam-y-position')).toHaveValue(
expectedCameraPosition.y.toString()
),
expect(page.getByTestId('cam-z-position')).toHaveValue(
_expect(page.getByTestId('cam-z-position')).toHaveValue(
expectedCameraPosition.z.toString()
),
// target
expect(page.getByTestId('cam-x-target')).toHaveValue(
_expect(page.getByTestId('cam-x-target')).toHaveValue(
expectedCameraTarget.x.toString()
),
expect(page.getByTestId('cam-y-target')).toHaveValue(
_expect(page.getByTestId('cam-y-target')).toHaveValue(
expectedCameraTarget.y.toString()
),
expect(page.getByTestId('cam-z-target')).toHaveValue(
_expect(page.getByTestId('cam-z-target')).toHaveValue(
expectedCameraTarget.z.toString()
),
])
})
}

test('Context menu and popover menu', async ({ page }) => {
_test('Context menu and popover menu', async ({ page }) => {
const testCase = {
testDescription: 'Right view',
expectedCameraPosition: { x: 5660.02, y: -152, z: 26 },
Expand Down Expand Up @@ -196,7 +196,7 @@ test.describe('Testing Gizmo', () => {
const buttonToTest = page.getByRole('button', {
name: testCase.testDescription,
})
await expect(buttonToTest).toBeVisible()
await _expect(buttonToTest).toBeVisible()
await buttonToTest.click()

// Now assert we've moved to the correct view
Expand All @@ -215,23 +215,23 @@ test.describe('Testing Gizmo', () => {

await Promise.all([
// position
expect(page.getByTestId('cam-x-position')).toHaveValue(
_expect(page.getByTestId('cam-x-position')).toHaveValue(
testCase.expectedCameraPosition.x.toString()
),
expect(page.getByTestId('cam-y-position')).toHaveValue(
_expect(page.getByTestId('cam-y-position')).toHaveValue(
testCase.expectedCameraPosition.y.toString()
),
expect(page.getByTestId('cam-z-position')).toHaveValue(
_expect(page.getByTestId('cam-z-position')).toHaveValue(
testCase.expectedCameraPosition.z.toString()
),
// target
expect(page.getByTestId('cam-x-target')).toHaveValue(
_expect(page.getByTestId('cam-x-target')).toHaveValue(
testCase.expectedCameraTarget.x.toString()
),
expect(page.getByTestId('cam-y-target')).toHaveValue(
_expect(page.getByTestId('cam-y-target')).toHaveValue(
testCase.expectedCameraTarget.y.toString()
),
expect(page.getByTestId('cam-z-target')).toHaveValue(
_expect(page.getByTestId('cam-z-target')).toHaveValue(
testCase.expectedCameraTarget.z.toString()
),
])
Expand All @@ -242,8 +242,60 @@ test.describe('Testing Gizmo', () => {
const gizmoPopoverButton = page.getByRole('button', {
name: 'view settings',
})
await expect(gizmoPopoverButton).toBeVisible()
await _expect(gizmoPopoverButton).toBeVisible()
await gizmoPopoverButton.click()
await expect(buttonToTest).toBeVisible()
await _expect(buttonToTest).toBeVisible()
})
})

test.describe(`Testing gizmo, fixture-based`, () => {
test('Center on selection from menu', async ({
app,
cmdBar,
editor,
toolbar,
scene,
}) => {
test.skip(
process.platform === 'win32',
'Fails on windows in CI, can not be replicated locally on windows.'
)

await test.step(`Setup`, async () => {
const file = await app.getInputFile('test-circle-extrude.kcl')
await app.initialise(file)
await scene.expectState({
camera: {
position: [4982.21, -23865.37, 13810.64],
target: [4982.21, 0, 2737.1],
},
})
})
const [clickCircle, moveToCircle] = scene.makeMouseHelpers(582, 217)

await test.step(`Select an edge of this circle`, async () => {
const circleSnippet =
'circle({ center: [318.33, 168.1], radius: 182.8 }, %)'
await moveToCircle()
await clickCircle()
await editor.expectState({
activeLines: [circleSnippet.slice(-5)],
highlightedCode: circleSnippet,
diagnostics: [],
})
})

await test.step(`Center on selection from menu`, async () => {
await scene.clickGizmoMenuItem('Center view on selection')
})

await test.step(`Verify the camera moved`, async () => {
await scene.expectState({
camera: {
position: [0, -23865.37, 11073.54],
target: [0, 0, 0],
},
})
})
})
})
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"@fortawesome/react-fontawesome": "^0.2.0",
"@headlessui/react": "^1.7.19",
"@headlessui/tailwindcss": "^0.2.0",
"@kittycad/lib": "^2.0.1",
"@kittycad/lib": "2.0.7",
"@lezer/highlight": "^1.2.1",
"@lezer/lr": "^1.4.1",
"@react-hook/resize-observer": "^2.0.1",
Expand Down
1 change: 1 addition & 0 deletions src/clientSideScene/CameraControls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,7 @@ export class CameraControls {
type: 'zoom_to_fit',
object_ids: [], // leave empty to zoom to all objects
padding: 0.2, // padding around the objects
animated: false, // don't animate the zoom for now
jtran marked this conversation as resolved.
Show resolved Hide resolved
},
})
}
Expand Down
10 changes: 10 additions & 0 deletions src/components/Gizmo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
import { Popover } from '@headlessui/react'
import { CustomIcon } from './CustomIcon'
import { reportRejection } from 'lib/trap'
import { useModelingContext } from 'hooks/useModelingContext'

const CANVAS_SIZE = 80
const FRUSTUM_SIZE = 0.5
Expand Down Expand Up @@ -62,6 +63,7 @@ export default function Gizmo() {
const raycasterIntersect = useRef<Intersection<Object3D> | null>(null)
const cameraPassiveUpdateTimer = useRef(0)
const raycasterPassiveUpdateTimer = useRef(0)
const { send: modelingSend } = useModelingContext()
const menuItems = useMemo(
() => [
...Object.entries(axisNamesSemantic).map(([axisName, axisSemantic]) => (
Expand All @@ -76,13 +78,21 @@ export default function Gizmo() {
{axisSemantic} view
</ContextMenuItem>
)),
<ContextMenuDivider />,
<ContextMenuItem
onClick={() => {
sceneInfra.camControls.resetCameraPosition().catch(reportRejection)
}}
>
Reset view
</ContextMenuItem>,
<ContextMenuItem
onClick={() => {
modelingSend({ type: 'Center camera on selection' })
}}
>
Center view on selection
</ContextMenuItem>,
<ContextMenuDivider />,
<ContextMenuItemRefresh />,
],
Expand Down
17 changes: 17 additions & 0 deletions src/components/ModelingMachineProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import {
} from 'lang/std/engineConnection'
import { submitAndAwaitTextToKcl } from 'lib/textToCad'
import { useFileContext } from 'hooks/useFileContext'
import { uuidv4 } from 'lib/utils'

type MachineContext<T extends AnyStateMachine> = {
state: StateFrom<T>
Expand Down Expand Up @@ -243,6 +244,17 @@ export const ModelingMachineProvider = ({
return {}
},
}),
'Center camera on selection': () => {
engineCommandManager
.sendSceneCommand({
type: 'modeling_cmd_req',
cmd_id: uuidv4(),
cmd: {
type: 'default_camera_center_to_selection',
},
})
.catch(reportRejection)
Comment on lines +248 to +256
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO should be in the CameraControls file but we can refactor later!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah good idea, that's better

},
'Set sketchDetails': assign(({ context: { sketchDetails }, event }) => {
if (event.type !== 'Delete segment') return {}
if (!sketchDetails) return {}
Expand Down Expand Up @@ -1037,6 +1049,11 @@ export const ModelingMachineProvider = ({
modelingSend({ type: 'Delete selection' })
})

// Allow ctrl+alt+c to center to selection
useHotkeys(['mod + alt + c'], () => {
modelingSend({ type: 'Center camera on selection' })
})

useStateMachineCommands({
machineId: 'modeling',
state: modelingState,
Expand Down
1 change: 1 addition & 0 deletions src/lang/KclSingleton.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ export class KclManager {
type: 'zoom_to_fit',
object_ids: zoomObjectId ? [zoomObjectId] : [], // leave empty to zoom to all objects
padding: 0.1, // padding around the objects
animated: false, // don't animate the zoom for now
},
})
}
Expand Down
7 changes: 7 additions & 0 deletions src/lib/settings/initialKeybindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,13 @@ export const interactionMap: Record<
description:
'Available while modeling with either a face selected or an empty selection, when not typing in the code editor.',
},
{
name: 'center-on-selection',
sequence: `${PRIMARY}+Alt+C`,
title: 'Center on selection',
description:
'Centers the view on the selected geometry, or everything if nothing is selected.',
},
],
'Code Editor': [
{
Expand Down
1 change: 1 addition & 0 deletions src/lib/singletons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ if (typeof window !== 'undefined') {
type: 'zoom_to_fit',
object_ids: [], // leave empty to zoom to all objects
padding: 0.2, // padding around the objects
animated: false, // don't animate the zoom for now
},
})
}
Loading
Loading