From c6d879d8eb74ec150cf699c84dc5bf0f1dfe6d30 Mon Sep 17 00:00:00 2001 From: Xinyi Xu Date: Tue, 11 Oct 2022 15:38:16 -0700 Subject: [PATCH] fix(composer): selectedDataBinding not able to update selected node (#274) --- .../scene-composer/src/SceneViewer.spec.tsx | 19 ++++- packages/scene-composer/src/SceneViewer.tsx | 16 +++- .../__snapshots__/SceneViewer.spec.tsx.snap | 18 ++-- .../src/components/StateManager.tsx | 20 +++++ .../components/three-fiber/EditorCamera.tsx | 41 +++++---- .../ModelRefComponent/GLTFModelComponent.tsx | 2 +- .../src/interfaces/sceneComposerInternal.ts | 1 + .../stories/SceneViewer.stories.tsx | 4 +- .../tests/StateManager.spec.tsx | 84 +++++++++++++++++++ 9 files changed, 170 insertions(+), 35 deletions(-) diff --git a/packages/scene-composer/src/SceneViewer.spec.tsx b/packages/scene-composer/src/SceneViewer.spec.tsx index e70e19f62..c6207cdea 100644 --- a/packages/scene-composer/src/SceneViewer.spec.tsx +++ b/packages/scene-composer/src/SceneViewer.spec.tsx @@ -5,20 +5,24 @@ const mockSceneComposerApi = { setSelectedSceneNodeRef: jest.fn(), }; +let onSceneLoadedCb; jest.doMock('./components/SceneComposerInternal', () => { const original = jest.requireActual('./components/SceneComposerInternal'); return { ...original, - SceneComposerInternal: 'SceneComposerInternal', + SceneComposerInternal: (props) => { + onSceneLoadedCb = props.onSceneLoaded; + return mockComponent('SceneComposerInternal')(props); + }, useSceneComposerApi: () => mockSceneComposerApi, } }); import * as React from 'react'; import renderer, { act } from 'react-test-renderer'; -import { useStore } from './store'; import { SceneViewer } from './SceneViewer'; import { KnownComponentType } from './interfaces'; +import mockComponent from '../__mocks__/mockComponent'; /* eslint-enable */ describe('SceneViewer', () => { @@ -52,6 +56,14 @@ describe('SceneViewer', () => { container = renderer.create(); }); + // not called before scene is loaded + expect(mockSceneComposerApi.findSceneNodeRefBy).not.toBeCalled(); + expect(mockSceneComposerApi.setCameraTarget).not.toBeCalled(); + + onSceneLoadedCb(); + container.update(); + + // called after scene is loaded expect(mockSceneComposerApi.findSceneNodeRefBy).toBeCalledTimes(1); expect(mockSceneComposerApi.findSceneNodeRefBy).toBeCalledWith(mockLabel, [KnownComponentType.Tag]); expect(mockSceneComposerApi.setCameraTarget).toBeCalledTimes(1); @@ -75,6 +87,9 @@ describe('SceneViewer', () => { container = renderer.create(); }); + onSceneLoadedCb(); + container.update(); + expect(mockSceneComposerApi.findSceneNodeRefBy).toBeCalledTimes(1); expect(mockSceneComposerApi.setCameraTarget).toBeCalledTimes(0); expect(mockSceneComposerApi.setSelectedSceneNodeRef).toBeCalledTimes(1); diff --git a/packages/scene-composer/src/SceneViewer.tsx b/packages/scene-composer/src/SceneViewer.tsx index b12642681..1f76aff14 100644 --- a/packages/scene-composer/src/SceneViewer.tsx +++ b/packages/scene-composer/src/SceneViewer.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useMemo, useRef } from 'react'; +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { v4 as uuid } from 'uuid'; import { isEqual } from 'lodash'; import styled from 'styled-components'; @@ -25,11 +25,18 @@ export const SceneViewer: React.FC = ({ sceneComposerId, confi }, [sceneComposerId]); const composerApis = useSceneComposerApi(composerId); const prevSelectedRef: any = useRef(); + const [sceneLoaded, setSceneLoaded] = useState(false); useEffect(() => { + // Do not update when scene is not loaded because nodes will be unavailable + if (!sceneLoaded) { + return; + } + // Do not update when there is no change if (isEqual(prevSelectedRef.current, props.selectedDataBinding)) { return; } + prevSelectedRef.current = props.selectedDataBinding; if (props.selectedDataBinding === undefined) { @@ -44,7 +51,11 @@ export const SceneViewer: React.FC = ({ sceneComposerId, confi } else { composerApis.setSelectedSceneNodeRef(undefined); } - }, [props.selectedDataBinding]); + }, [props.selectedDataBinding, sceneLoaded]); + + const onSceneLoaded = useCallback(() => { + setSceneLoaded(true); + }, [setSceneLoaded]); return ( @@ -54,6 +65,7 @@ export const SceneViewer: React.FC = ({ sceneComposerId, confi ...(config || {}), mode: 'Viewing', }} + onSceneLoaded={onSceneLoaded} {...props} /> diff --git a/packages/scene-composer/src/__snapshots__/SceneViewer.spec.tsx.snap b/packages/scene-composer/src/__snapshots__/SceneViewer.spec.tsx.snap index 0e3e8370b..30129c03c 100644 --- a/packages/scene-composer/src/__snapshots__/SceneViewer.spec.tsx.snap +++ b/packages/scene-composer/src/__snapshots__/SceneViewer.spec.tsx.snap @@ -18,20 +18,12 @@ exports[`SceneViewer should render correctly 1`] = ` className="c0" data-testid="webgl-root" > - `; diff --git a/packages/scene-composer/src/components/StateManager.tsx b/packages/scene-composer/src/components/StateManager.tsx index 1b3e7f645..ec5e338d4 100644 --- a/packages/scene-composer/src/components/StateManager.tsx +++ b/packages/scene-composer/src/components/StateManager.tsx @@ -36,6 +36,7 @@ const StateManager: React.FC = ({ sceneLoader, config, onSceneUpdated, + onSceneLoaded, valueDataBindingProvider, showAssetBrowserCallback, onWidgetClick, @@ -55,6 +56,7 @@ const StateManager: React.FC = ({ setDataInput, setDataBindingTemplate, loadScene, + sceneLoaded, selectedSceneNodeRef, setSelectedSceneNodeRef, getSceneNodeByRef, @@ -68,6 +70,7 @@ const StateManager: React.FC = ({ const messages = useStore(sceneComposerId)((state) => state.getMessages()); const dataProviderRef = useRef | undefined>(undefined); + const prevSelection = useRef(undefined); const { setActiveCameraSettings, setActiveCameraName } = useActiveCamera(); @@ -105,6 +108,13 @@ const StateManager: React.FC = ({ }, [activeCamera, selectedDataBinding]); useEffect(() => { + // This hook will somehow be triggered twice initially with selectedSceneNodeRef = undefined. + // Compare prevSelection with selectedSceneNodeRef to make sure the event is sent only when value + // is changed. + if (prevSelection.current === selectedSceneNodeRef) return; + + prevSelection.current = selectedSceneNodeRef; + if (onSelectionChanged) { const node = getSceneNodeByRef(selectedSceneNodeRef); const componentTypes = node?.components.map((component) => component.type) ?? []; @@ -212,6 +222,16 @@ const StateManager: React.FC = ({ } }, [sceneContent]); + useEffect(() => { + if (onSceneLoaded && sceneLoaded) { + // Delay the event handler to let other components finish loading, otherwise the consumer side will + // fail to update scene states + setTimeout(() => { + onSceneLoaded(); + }, 1); + } + }, [sceneLoaded, onSceneLoaded]); + // Subscribe to store update useEffect(() => { if (onSceneUpdated) { diff --git a/packages/scene-composer/src/components/three-fiber/EditorCamera.tsx b/packages/scene-composer/src/components/three-fiber/EditorCamera.tsx index bff772c89..4402bba9a 100644 --- a/packages/scene-composer/src/components/three-fiber/EditorCamera.tsx +++ b/packages/scene-composer/src/components/three-fiber/EditorCamera.tsx @@ -37,6 +37,7 @@ export const EditorMainCamera = React.forwardRef((_, forwardedRef) => { const [cameraTarget, setCameraTarget] = useState<{ target?: FixedCameraTarget; shouldTween?: boolean }>(); const [controlsRemove, setControlsRemove] = useState(false); const [setTween, updateTween] = useTween(); + const [mounted, setMounted] = useState(false); const activeCamera = useActiveCamera(); @@ -77,15 +78,32 @@ export const EditorMainCamera = React.forwardRef((_, forwardedRef) => { } }, []); - // translate camera command useEffect(() => { - log?.verbose('setting camera target by command', cameraCommand); + setMounted(true); + }, []); + + useEffect(() => { + // Don't update camera target before component is mounted because wrong bounding box will be calaulated. + if (!mounted) return; + + if (!cameraCommand) { + // update the camera position to default + const sceneObject = scene.getObjectByName(ROOT_OBJECT_3D_NAME); + log?.verbose('setting camera target to root object', cameraCommand); + + setCameraTarget({ + target: sceneObject && findBestViewingPosition(sceneObject, true, cameraControlsImplRef.current), + shouldTween: true, + }); + } else { + log?.verbose('setting camera target by command', cameraCommand); - setCameraTarget({ - target: getCameraTargetByCommand(cameraCommand), - shouldTween: cameraCommand?.mode === 'transition', - }); - }, [cameraCommand]); + setCameraTarget({ + target: getCameraTargetByCommand(cameraCommand), + shouldTween: cameraCommand?.mode === 'transition', + }); + } + }, [cameraCommand, mounted]); // execute camera command useEffect(() => { @@ -136,15 +154,6 @@ export const EditorMainCamera = React.forwardRef((_, forwardedRef) => { }; }, [transformControls]); - // on mount, update the camera position - useEffect(() => { - const sceneObject = scene.getObjectByName(ROOT_OBJECT_3D_NAME); - setCameraTarget({ - target: sceneObject && findBestViewingPosition(sceneObject, true, cameraControlsImplRef.current), - shouldTween: true, - }); - }, []); - useFrame(() => updateTween()); useEffect(() => { diff --git a/packages/scene-composer/src/components/three-fiber/ModelRefComponent/GLTFModelComponent.tsx b/packages/scene-composer/src/components/three-fiber/ModelRefComponent/GLTFModelComponent.tsx index a10d445b5..b0e68949f 100644 --- a/packages/scene-composer/src/components/three-fiber/ModelRefComponent/GLTFModelComponent.tsx +++ b/packages/scene-composer/src/components/three-fiber/ModelRefComponent/GLTFModelComponent.tsx @@ -143,7 +143,7 @@ export const GLTFModelComponent: React.FC = ({ } }); - if (Date.now() - lastPointerMove >= CURSOR_VISIBILITY_TIMEOUT && !addingWidget) { + if (Date.now() - lastPointerMove >= CURSOR_VISIBILITY_TIMEOUT && !addingWidget && cursorVisible) { setCursorVisible(false); } }); diff --git a/packages/scene-composer/src/interfaces/sceneComposerInternal.ts b/packages/scene-composer/src/interfaces/sceneComposerInternal.ts index f98ebf486..32d8d176f 100644 --- a/packages/scene-composer/src/interfaces/sceneComposerInternal.ts +++ b/packages/scene-composer/src/interfaces/sceneComposerInternal.ts @@ -31,6 +31,7 @@ export type ShowAssetBrowserCallback = (callback: AssetBrowserResultCallback) => export interface SceneComposerInternalProps extends SceneViewerPropsShared { onSceneUpdated?: OnSceneUpdateCallback; + onSceneLoaded?: () => void; valueDataBindingProvider?: IValueDataBindingProvider; showAssetBrowserCallback?: ShowAssetBrowserCallback; diff --git a/packages/scene-composer/stories/SceneViewer.stories.tsx b/packages/scene-composer/stories/SceneViewer.stories.tsx index 72b331363..e40b8e3b4 100644 --- a/packages/scene-composer/stories/SceneViewer.stories.tsx +++ b/packages/scene-composer/stories/SceneViewer.stories.tsx @@ -151,7 +151,7 @@ export default { } as ComponentMeta; export const Default: ComponentStory = (args: SceneViewerProps & { sceneId?: string }) => { - const [selected, setSelected] = useState(undefined); + const [selected, setSelected] = useState({ entityId: 'room1', componentName: 'temperatureSensor2' }); const loader = useMemo(() => { return args.sceneLoader; }, [args.sceneId]); @@ -163,6 +163,8 @@ export const Default: ComponentStory = (args: SceneViewerPro const onSelectionChanged = useCallback((e: ISelectionChangedEvent) => { const dataBindingContext = e.additionalComponentData?.[0].dataBindingContext; + console.log('onSelectionChanged', dataBindingContext); + setSelected( dataBindingContext ? { diff --git a/packages/scene-composer/tests/StateManager.spec.tsx b/packages/scene-composer/tests/StateManager.spec.tsx index 2c729c358..5daf3d2f4 100644 --- a/packages/scene-composer/tests/StateManager.spec.tsx +++ b/packages/scene-composer/tests/StateManager.spec.tsx @@ -56,6 +56,8 @@ describe('StateManager', () => { loadScene: jest.fn(), getMessages: jest.fn().mockReturnValue(['messagge']), getObject3DBySceneNodeRef, + selectedSceneNodeRef: undefined, + sceneLoaded: false, }; const mockSceneContent = 'This is test content'; const mockGetSceneObjectFunction = jest.fn(); @@ -338,4 +340,86 @@ describe('StateManager', () => { }); expect(useActiveCamera().setActiveCameraName).not.toBeCalled(); }); + + it('should call onSceneLoaded', async () => { + useStore('default').setState({ + ...baseState, + }); + const onSceneLoaded = jest.fn(); + + let container; + await act(async () => { + container = renderer.create( + , + ); + }); + + useStore('default').setState({ + ...baseState, + sceneLoaded: true, + }); + + container.update( + , + ); + await new Promise((resolve) => setTimeout(resolve, 1)); + + expect(onSceneLoaded).toBeCalledTimes(1); + }); + + it('should call onSelectionChanged as expected', async () => { + useStore('default').setState({ + ...baseState, + }); + const onSelectionChanged = jest.fn(); + + // not called when selection is not changed + let container; + await act(async () => { + container = renderer.create( + , + ); + }); + expect(onSelectionChanged).not.toBeCalled(); + + // called when selection is changed + useStore('default').setState({ + ...baseState, + selectedSceneNodeRef: 'abc', + }); + container.update( + , + ); + expect(onSelectionChanged).toBeCalledTimes(1); + + // not called when selection is not changed + useStore('default').setState({ + ...baseState, + selectedSceneNodeRef: 'abc', + }); + container.update( + , + ); + expect(onSelectionChanged).toBeCalledTimes(1); + }); });