From 2696ddb996c5dda2a4ce79792cf1014cb1f75e0e Mon Sep 17 00:00:00 2001 From: Pierre Jacquier Date: Thu, 6 Mar 2025 11:19:13 -0500 Subject: [PATCH] Lf94/hidpi hovering fix (#5541) * Fix hover highlights on HiDPI screens * Fix flakey tests with new toolbar.exitSketch * tsc && lint && fmt * Disable pw electron thing again * Fix test --------- Co-authored-by: 49lf --- e2e/playwright/fixtures/toolbarFixture.ts | 10 ++++++ e2e/playwright/point-click.spec.ts | 8 +++-- e2e/playwright/testing-selections.spec.ts | 5 ++- src/clientSideScene/CameraControls.ts | 13 ++++++-- src/components/Stream.tsx | 6 ++-- src/hooks/useSetupEngineManager.ts | 5 +-- src/lang/std/engineConnection.ts | 40 +++++++++++------------ src/lib/selections.ts | 19 ++++++----- src/lib/utils.ts | 31 ++++++++---------- 9 files changed, 78 insertions(+), 59 deletions(-) diff --git a/e2e/playwright/fixtures/toolbarFixture.ts b/e2e/playwright/fixtures/toolbarFixture.ts index baf77890d..1f52efda6 100644 --- a/e2e/playwright/fixtures/toolbarFixture.ts +++ b/e2e/playwright/fixtures/toolbarFixture.ts @@ -84,6 +84,16 @@ export class ToolbarFixture { startSketchPlaneSelection = async () => doAndWaitForImageDiff(this.page, () => this.startSketchBtn.click(), 500) + exitSketch = async () => { + await this.exitSketchBtn.click() + await expect( + this.page.getByRole('button', { name: 'Start Sketch' }) + ).toBeVisible() + await expect( + this.page.getByRole('button', { name: 'Start Sketch' }) + ).not.toBeDisabled() + } + editSketch = async () => { await this.editSketchBtn.first().click() // One of the rare times we want to allow a arbitrary wait diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index bb19da818..2b7d508a6 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -168,8 +168,7 @@ test.describe('Point-and-click tests', () => { }) await test.step('Clean up so that `_sketchOnAChamfer` util can be called again', async () => { - await toolbar.exitSketchBtn.click() - await scene.waitForExecutionDone() + await toolbar.exitSketch() }) await test.step('Check there is no errors after code created in previous steps executes', async () => { await editor.expectState({ @@ -200,7 +199,9 @@ test.describe('Point-and-click tests', () => { }, file) await page.setBodyDimensions({ width: 1000, height: 500 }) await homePage.goToModelingScene() - await scene.waitForExecutionDone() + await expect( + page.getByTestId('model-state-indicator-receive-reliable') + ).toBeVisible() const sketchOnAChamfer = _sketchOnAChamfer(page, editor, toolbar, scene) @@ -388,6 +389,7 @@ profile001 = startProfileAt([205.96, 254.59], sketch002) }, file) await page.setBodyDimensions({ width: 1000, height: 500 }) await homePage.goToModelingScene() + await scene.waitForExecutionDone() const sketchOnAChamfer = _sketchOnAChamfer(page, editor, toolbar, scene) diff --git a/e2e/playwright/testing-selections.spec.ts b/e2e/playwright/testing-selections.spec.ts index 4e69c23ac..9909f527b 100644 --- a/e2e/playwright/testing-selections.spec.ts +++ b/e2e/playwright/testing-selections.spec.ts @@ -775,8 +775,11 @@ profile003 = startProfileAt([40.16, -120.48], sketch006) ], ) `) + await expect( - page.getByTestId('model-state-indicator-execution-done') + page + .getByTestId('model-state-indicator-receive-reliable') + .or(page.getByTestId('model-state-indicator-execution-done')) ).toBeVisible() await u.openAndClearDebugPanel() diff --git a/src/clientSideScene/CameraControls.ts b/src/clientSideScene/CameraControls.ts index 9b8657455..ad74f1feb 100644 --- a/src/clientSideScene/CameraControls.ts +++ b/src/clientSideScene/CameraControls.ts @@ -22,7 +22,7 @@ import { UnreliableSubscription, } from 'lang/std/engineConnection' import { EngineCommand } from 'lang/std/artifactGraph' -import { toSync, uuidv4 } from 'lib/utils' +import { toSync, uuidv4, getNormalisedCoordinates } from 'lib/utils' import { deg2Rad } from 'lib/utils2d' import { isReducedMotion, roundOff, throttle } from 'lib/utils' import * as TWEEN from '@tweenjs/tween.js' @@ -109,6 +109,7 @@ export class CameraControls { interactionGuards: MouseGuard = cameraMouseDragGuards.Zoo isFovAnimationInProgress = false perspectiveFovBeforeOrtho = 45 + // NOTE: Duplicated state across Provider and singleton. Mapped from settingsMachine _setting_allowOrbitInSketchMode = false get isPerspective() { @@ -458,11 +459,19 @@ export class CameraControls { if (this.syncDirection === 'engineToClient') { const newCmdId = uuidv4() + // Nonsense to do anything until the video stream is established. + if (!this.engineCommandManager.elVideo) return + + const { x, y } = getNormalisedCoordinates( + event, + this.engineCommandManager.elVideo, + this.engineCommandManager.streamDimensions + ) this.throttledEngCmd({ type: 'modeling_cmd_req', cmd: { type: 'highlight_set_entity', - selected_at_window: { x: event.clientX, y: event.clientY }, + selected_at_window: { x, y }, }, cmd_id: newCmdId, }) diff --git a/src/components/Stream.tsx b/src/components/Stream.tsx index 8ec2d1fbe..2057da33f 100644 --- a/src/components/Stream.tsx +++ b/src/components/Stream.tsx @@ -47,6 +47,8 @@ export const Stream = () => { overallState === NetworkHealthState.Ok || overallState === NetworkHealthState.Weak + engineCommandManager.elVideo = videoRef.current + /** * Execute code and show a "building scene message" * in Stream.tsx in the meantime. @@ -272,7 +274,7 @@ export const Stream = () => { if (btnName(e.nativeEvent).left) { // eslint-disable-next-line @typescript-eslint/no-floating-promises - sendSelectEventToEngine(e, videoRef.current) + sendSelectEventToEngine(e) } } @@ -294,7 +296,7 @@ export const Stream = () => { return } - sendSelectEventToEngine(e, videoRef.current) + sendSelectEventToEngine(e) .then(({ entity_id }) => { if (!entity_id) { // No entity selected. This is benign diff --git a/src/hooks/useSetupEngineManager.ts b/src/hooks/useSetupEngineManager.ts index 1b41ee5a2..80b47ed81 100644 --- a/src/hooks/useSetupEngineManager.ts +++ b/src/hooks/useSetupEngineManager.ts @@ -101,10 +101,7 @@ export function useSetupEngineManager( streamRef?.current?.offsetWidth ?? 0, streamRef?.current?.offsetHeight ?? 0 ) - engineCommandManager.handleResize({ - streamWidth: width, - streamHeight: height, - }) + engineCommandManager.handleResize(engineCommandManager.streamDimensions) }, 500) const onOnline = () => { diff --git a/src/lang/std/engineConnection.ts b/src/lang/std/engineConnection.ts index da77b05d8..44a92d073 100644 --- a/src/lang/std/engineConnection.ts +++ b/src/lang/std/engineConnection.ts @@ -1447,11 +1447,17 @@ export class EngineCommandManager extends EventTarget { commandId: string } settings: SettingsViaQueryString - width: number = 1337 - height: number = 1337 + + streamDimensions = { + // Random defaults that are overwritten pretty much immediately + width: 1337, + height: 1337, + } + + elVideo: HTMLVideoElement | null = null /** - * Export intent traxcks the intent of the export. If it is null there is no + * Export intent tracks the intent of the export. If it is null there is no * export in progress. Otherwise it is an enum value of the intent. * Another export cannot be started if one is already in progress. */ @@ -1554,15 +1560,14 @@ export class EngineCommandManager extends EventTarget { return } - this.width = width - this.height = height + this.streamDimensions = { + width, + height, + } // If we already have an engine connection, just need to resize the stream. if (this.engineConnection) { - this.handleResize({ - streamWidth: width, - streamHeight: height, - }) + this.handleResize(this.streamDimensions) return } @@ -1858,27 +1863,22 @@ export class EngineCommandManager extends EventTarget { return } - handleResize({ - streamWidth, - streamHeight, - }: { - streamWidth: number - streamHeight: number - }) { + handleResize({ width, height }: { width: number; height: number }) { if (!this.engineConnection?.isReady()) { return } - this.width = streamWidth - this.height = streamHeight + this.streamDimensions = { + width, + height, + } const resizeCmd: EngineCommand = { type: 'modeling_cmd_req', cmd_id: uuidv4(), cmd: { type: 'reconfigure_stream', - width: streamWidth, - height: streamHeight, + ...this.streamDimensions, fps: 60, }, } diff --git a/src/lib/selections.ts b/src/lib/selections.ts index b4ccc226f..a1502d027 100644 --- a/src/lib/selections.ts +++ b/src/lib/selections.ts @@ -646,16 +646,17 @@ export function codeToIdSelections( } export async function sendSelectEventToEngine( - e: MouseEvent | React.MouseEvent, - el: HTMLVideoElement + e: React.MouseEvent ) { - const { x, y } = getNormalisedCoordinates({ - clientX: e.clientX, - clientY: e.clientY, - el, - streamWidth: engineCommandManager.width, - streamHeight: engineCommandManager.height, - }) + // No video stream to normalise against, return immediately + if (!engineCommandManager.elVideo) + return Promise.reject('video element not ready') + + const { x, y } = getNormalisedCoordinates( + e, + engineCommandManager.elVideo, + engineCommandManager.streamDimensions + ) const res = await engineCommandManager.sendSceneCommand({ type: 'modeling_cmd_req', cmd: { diff --git a/src/lib/utils.ts b/src/lib/utils.ts index f051a6f00..f03a9d735 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -161,25 +161,20 @@ export function toSync>( } } -export function getNormalisedCoordinates({ - clientX, - clientY, - streamWidth, - streamHeight, - el, -}: { - clientX: number - clientY: number - streamWidth: number - streamHeight: number - el: HTMLElement -}) { - const { left, top, width, height } = el?.getBoundingClientRect() - const browserX = clientX - left - const browserY = clientY - top +export function getNormalisedCoordinates( + e: PointerEvent | React.MouseEvent, + elVideo: HTMLVideoElement, + streamDimensions: { + width: number + height: number + } +) { + const { left, top, width, height } = elVideo?.getBoundingClientRect() + const browserX = e.clientX - left + const browserY = e.clientY - top return { - x: Math.round((browserX / width) * streamWidth), - y: Math.round((browserY / height) * streamHeight), + x: Math.round((browserX / width) * streamDimensions.width), + y: Math.round((browserY / height) * streamDimensions.height), } }