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

fix(composer): selectedDataBinding not able to update selected node #274

Merged
merged 1 commit into from
Oct 11, 2022
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
19 changes: 17 additions & 2 deletions packages/scene-composer/src/SceneViewer.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -52,6 +56,14 @@ describe('SceneViewer', () => {
container = renderer.create(<SceneViewer sceneLoader={mockSceneLoader} selectedDataBinding={mockLabel} />);
});

// not called before scene is loaded
expect(mockSceneComposerApi.findSceneNodeRefBy).not.toBeCalled();
expect(mockSceneComposerApi.setCameraTarget).not.toBeCalled();

onSceneLoadedCb();
container.update(<SceneViewer sceneLoader={mockSceneLoader} selectedDataBinding={mockLabel} />);

// called after scene is loaded
expect(mockSceneComposerApi.findSceneNodeRefBy).toBeCalledTimes(1);
expect(mockSceneComposerApi.findSceneNodeRefBy).toBeCalledWith(mockLabel, [KnownComponentType.Tag]);
expect(mockSceneComposerApi.setCameraTarget).toBeCalledTimes(1);
Expand All @@ -75,6 +87,9 @@ describe('SceneViewer', () => {
container = renderer.create(<SceneViewer sceneLoader={mockSceneLoader} selectedDataBinding={mockLabel} />);
});

onSceneLoadedCb();
container.update(<SceneViewer sceneLoader={mockSceneLoader} selectedDataBinding={mockLabel} />);

expect(mockSceneComposerApi.findSceneNodeRefBy).toBeCalledTimes(1);
expect(mockSceneComposerApi.setCameraTarget).toBeCalledTimes(0);
expect(mockSceneComposerApi.setSelectedSceneNodeRef).toBeCalledTimes(1);
Expand Down
16 changes: 14 additions & 2 deletions packages/scene-composer/src/SceneViewer.tsx
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -25,11 +25,18 @@ export const SceneViewer: React.FC<SceneViewerProps> = ({ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a lot of this stuff is done in the wrong place hence the issue with it not being loaded. I think you should consider putting it where we handle active camera instead and try to have it be handled by the state manager

https://github.com/awslabs/iot-app-kit/blob/main/packages/scene-composer/src/components/StateManager.tsx#L102

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the selectedDataBinding feature is currently a viewer only feature and is not tested with editing mode at all. so I think it's more clear to be in this component at least for now.
also, no matter this logic is here or in state manager, we will have the same not loaded issue.

Expand All @@ -44,7 +51,11 @@ export const SceneViewer: React.FC<SceneViewerProps> = ({ sceneComposerId, confi
} else {
composerApis.setSelectedSceneNodeRef(undefined);
}
}, [props.selectedDataBinding]);
}, [props.selectedDataBinding, sceneLoaded]);

const onSceneLoaded = useCallback(() => {
setSceneLoaded(true);
}, [setSceneLoaded]);

return (
<SceneComposerContainer data-testid={'webgl-root'}>
Expand All @@ -54,6 +65,7 @@ export const SceneViewer: React.FC<SceneViewerProps> = ({ sceneComposerId, confi
...(config || {}),
mode: 'Viewing',
}}
onSceneLoaded={onSceneLoaded}
{...props}
/>
</SceneComposerContainer>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,12 @@ exports[`SceneViewer should render correctly 1`] = `
className="c0"
data-testid="webgl-root"
>
<SceneComposerInternal
config={
Object {
"mode": "Viewing",
}
}
<div
config="{\\"mode\\":\\"Viewing\\"}"
data-mocked="SceneComposerInternal"
onSceneLoaded={[Function]}
sceneComposerId="123"
sceneLoader={
Object {
"getSceneObject": [MockFunction],
"getSceneUri": [Function],
"getSceneUrl": [Function],
}
}
sceneLoader="{}"
/>
</div>
`;
20 changes: 20 additions & 0 deletions packages/scene-composer/src/components/StateManager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const StateManager: React.FC<SceneComposerInternalProps> = ({
sceneLoader,
config,
onSceneUpdated,
onSceneLoaded,
valueDataBindingProvider,
showAssetBrowserCallback,
onWidgetClick,
Expand All @@ -55,6 +56,7 @@ const StateManager: React.FC<SceneComposerInternalProps> = ({
setDataInput,
setDataBindingTemplate,
loadScene,
sceneLoaded,
selectedSceneNodeRef,
setSelectedSceneNodeRef,
getSceneNodeByRef,
Expand All @@ -68,6 +70,7 @@ const StateManager: React.FC<SceneComposerInternalProps> = ({
const messages = useStore(sceneComposerId)((state) => state.getMessages());

const dataProviderRef = useRef<ProviderWithViewport<TimeSeriesData[]> | undefined>(undefined);
const prevSelection = useRef<string | undefined>(undefined);

const { setActiveCameraSettings, setActiveCameraName } = useActiveCamera();

Expand Down Expand Up @@ -105,6 +108,13 @@ const StateManager: React.FC<SceneComposerInternalProps> = ({
}, [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) ?? [];
Expand Down Expand Up @@ -212,6 +222,16 @@ const StateManager: React.FC<SceneComposerInternalProps> = ({
}
}, [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) {
Expand Down
41 changes: 25 additions & 16 deletions packages/scene-composer/src/components/three-fiber/EditorCamera.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const EditorMainCamera = React.forwardRef<Camera>((_, forwardedRef) => {
const [cameraTarget, setCameraTarget] = useState<{ target?: FixedCameraTarget; shouldTween?: boolean }>();
const [controlsRemove, setControlsRemove] = useState(false);
const [setTween, updateTween] = useTween<TweenValueObject>();
const [mounted, setMounted] = useState(false);

const activeCamera = useActiveCamera();

Expand Down Expand Up @@ -77,15 +78,32 @@ export const EditorMainCamera = React.forwardRef<Camera>((_, 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(() => {
Expand Down Expand Up @@ -136,15 +154,6 @@ export const EditorMainCamera = React.forwardRef<Camera>((_, 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(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ export const GLTFModelComponent: React.FC<GLTFModelProps> = ({
}
});

if (Date.now() - lastPointerMove >= CURSOR_VISIBILITY_TIMEOUT && !addingWidget) {
if (Date.now() - lastPointerMove >= CURSOR_VISIBILITY_TIMEOUT && !addingWidget && cursorVisible) {
setCursorVisible(false);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export type ShowAssetBrowserCallback = (callback: AssetBrowserResultCallback) =>

export interface SceneComposerInternalProps extends SceneViewerPropsShared {
onSceneUpdated?: OnSceneUpdateCallback;
onSceneLoaded?: () => void;

valueDataBindingProvider?: IValueDataBindingProvider;
showAssetBrowserCallback?: ShowAssetBrowserCallback;
Expand Down
4 changes: 3 additions & 1 deletion packages/scene-composer/stories/SceneViewer.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export default {
} as ComponentMeta<typeof SceneViewer>;

export const Default: ComponentStory<typeof SceneViewer> = (args: SceneViewerProps & { sceneId?: string }) => {
const [selected, setSelected] = useState<any>(undefined);
const [selected, setSelected] = useState<any>({ entityId: 'room1', componentName: 'temperatureSensor2' });
const loader = useMemo(() => {
return args.sceneLoader;
}, [args.sceneId]);
Expand All @@ -163,6 +163,8 @@ export const Default: ComponentStory<typeof SceneViewer> = (args: SceneViewerPro

const onSelectionChanged = useCallback((e: ISelectionChangedEvent) => {
const dataBindingContext = e.additionalComponentData?.[0].dataBindingContext;
console.log('onSelectionChanged', dataBindingContext);

setSelected(
dataBindingContext
? {
Expand Down
84 changes: 84 additions & 0 deletions packages/scene-composer/tests/StateManager.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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(
<StateManager
sceneLoader={mockSceneLoader}
config={{ dracoDecoder: true } as any}
onSceneLoaded={onSceneLoaded}
/>,
);
});

useStore('default').setState({
...baseState,
sceneLoaded: true,
});

container.update(
<StateManager
sceneLoader={mockSceneLoader}
config={{ dracoDecoder: true } as any}
onSceneLoaded={onSceneLoaded}
/>,
);
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(
<StateManager
sceneLoader={mockSceneLoader}
config={{ dracoDecoder: true } as any}
onSelectionChanged={onSelectionChanged}
/>,
);
});
expect(onSelectionChanged).not.toBeCalled();

// called when selection is changed
useStore('default').setState({
...baseState,
selectedSceneNodeRef: 'abc',
});
container.update(
<StateManager
sceneLoader={mockSceneLoader}
config={{ dracoDecoder: true } as any}
onSelectionChanged={onSelectionChanged}
/>,
);
expect(onSelectionChanged).toBeCalledTimes(1);

// not called when selection is not changed
useStore('default').setState({
...baseState,
selectedSceneNodeRef: 'abc',
});
container.update(
<StateManager
sceneLoader={mockSceneLoader}
config={{ dracoDecoder: true } as any}
onSelectionChanged={onSelectionChanged}
/>,
);
expect(onSelectionChanged).toBeCalledTimes(1);
});
});