-
Notifications
You must be signed in to change notification settings - Fork 47
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
Changes from all commits
1c5fabd
dedf91a
45af033
27f7ae7
08e4ffb
fa5cd82
9fe4778
5b18a94
3abdd84
0e901c8
1a3ee0e
aa4cc0c
8d35830
ec8afa1
90a15de
e4475ce
dc66550
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mustve missed the memo - what's going on here? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we revert this name change and do something like
I am confused to why we have a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
|
@@ -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) | ||
|
@@ -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 }, | ||
|
@@ -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 | ||
|
@@ -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() | ||
), | ||
]) | ||
|
@@ -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], | ||
}, | ||
}) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO should be in the CameraControls file but we can refactor later! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 {} | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
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 :')