Skip to content

Commit

Permalink
fix(composer): reorder to same parent duplicates child
Browse files Browse the repository at this point in the history
  • Loading branch information
@TheEvilDev
Xinyi Xu authored and TheEvilDev committed Dec 2, 2022
1 parent 53675e9 commit b76057d
Show file tree
Hide file tree
Showing 6 changed files with 167 additions and 58 deletions.
81 changes: 36 additions & 45 deletions ...s/scene-composer/src/components/panels/SceneHierarchyPanel/SceneHierarchyDataProvider.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ const sortNodes = (a, b) => {
};

const SceneHierarchyDataProvider: FC<SceneHierarchyDataProviderProps> = ({ selectionMode, children }) => {
useLifecycleLogging('SceneHierarchyDataProvider');
const log = useLifecycleLogging('SceneHierarchyDataProvider');
const sceneComposerId = useSceneComposerId();
const { document, removeSceneNode } = useStore(sceneComposerId)((state) => state);
const { isEditing } = useStore(sceneComposerId)((state) => state);
Expand Down Expand Up @@ -192,52 +192,43 @@ const SceneHierarchyDataProvider: FC<SceneHierarchyDataProviderProps> = ({ selec
(objectRef: string, newParentRef?: string) => {
const originalObject = getSceneNodeByRef(objectRef);
const originalObject3D = getObject3DBySceneNodeRef(objectRef);
if (newParentRef) {
const objectToMoveRef = originalObject?.ref as string;
const oldParentRef = originalObject?.parentRef as string;
const newParent = getSceneNodeByRef(newParentRef);
const oldParent = getSceneNodeByRef(oldParentRef);
const oldParentChildren = oldParent?.childRefs.filter((child) => child !== objectToMoveRef);
const newParentObject = getObject3DBySceneNodeRef(newParentRef);
let maintainedTransform: any = null;
if (originalObject3D) {
const worldPosition = originalObject3D.getWorldPosition(new Vector3());
const worldRotation = new Euler().setFromQuaternion(originalObject3D.getWorldQuaternion(new Quaternion()));
const worldScale = originalObject3D.getWorldScale(new Vector3());
maintainedTransform = getFinalTransform(
{
position: worldPosition,
rotation: worldRotation,
scale: worldScale,
},
newParentObject,
);
}
// remove child ref from parent
if (!oldParentRef) {
const newRoots = document.rootNodeRefs.filter((ref) => ref !== objectRef);
updateDocumentInternal({ rootNodeRefs: newRoots });
} else {
updateSceneNodeInternal(oldParentRef, { childRefs: oldParentChildren });
}
// Create updates to the moving object
const partial: RecursivePartial<ISceneNodeInternal> = { parentRef: newParentRef };
if (maintainedTransform) {
// Update the node position to remain in its world space
partial.transform = {
position: maintainedTransform.position.toArray(),
rotation: maintainedTransform.rotation.toVector3().toArray(),
scale: maintainedTransform.scale.toArray(),
};
}
// update new parent to have new child
updateSceneNodeInternal(newParentRef, { childRefs: [...newParent!.childRefs, objectRef] });
// update node to have new parent
updateSceneNodeInternal(objectToMoveRef, partial);
// TODO: create single call to handle this
const oldParentRef = originalObject?.parentRef as string;

if (oldParentRef === newParentRef) {
log?.verbose('Parent ref unchanged. Do nothing.');
return;
}

const newParentObject = getObject3DBySceneNodeRef(newParentRef);
let maintainedTransform: any = null;
if (originalObject3D) {
const worldPosition = originalObject3D.getWorldPosition(new Vector3());
const worldRotation = new Euler().setFromQuaternion(originalObject3D.getWorldQuaternion(new Quaternion()));
const worldScale = originalObject3D.getWorldScale(new Vector3());
maintainedTransform = getFinalTransform(
{
position: worldPosition,
rotation: worldRotation,
scale: worldScale,
},
newParentObject,
);
}

// Create updates to the moving object
const partial: RecursivePartial<ISceneNodeInternal> = { parentRef: newParentRef };
if (maintainedTransform) {
// Update the node position to remain in its world space
partial.transform = {
position: maintainedTransform.position.toArray(),
rotation: maintainedTransform.rotation.toVector3().toArray(),
scale: maintainedTransform.scale.toArray(),
};
}

updateSceneNodeInternal(objectRef, partial);
},
[updateSceneNodeInternal, updateDocumentInternal, getSceneNodeByRef, nodeMap, document],
[updateSceneNodeInternal, getSceneNodeByRef, nodeMap, document],
);

const show = useCallback(
Expand Down
2 changes: 1 addition & 1 deletion ...nents/panels/SceneHierarchyPanel/components/SceneHierarchyTree/SceneHierarchyTreeItem.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ const SceneHierarchyTreeItem: FC<SceneHierarchyTreeItemProps> = ({
labelText={<SceneNodeLabel objectRef={key} labelText={labelText} componentTypes={componentTypes} />}
onExpand={onExpandNode}
expanded={expanded}
expandable={node && node.childRefs.length > 0 && !isSearching}
expandable={node && (node.childRefs.length > 0 || !!showSubModel) && !isSearching}
selected={selected === key}
selectionMode={selectionMode}
onSelected={isViewing() ? onActivated : onToggle}
Expand Down
8 changes: 8 additions & 0 deletions packages/scene-composer/src/store/slices/EditorStateSlice.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,21 @@ export const createEditStateSlice = (set: SetState<RootState>, get: GetState<Roo
},

setCursorVisible(isVisible: boolean) {
if (get().cursorVisible === isVisible) {
return;
}

set((draft) => {
draft.cursorVisible = isVisible;
draft.lastOperation = 'setCursorVisible';
});
},

setCursorStyle(style: CursorStyle) {
if (get().cursorStyle === style) {
return;
}

set((draft) => {
draft.cursorStyle = style;
draft.lastOperation = 'setCursorStyle';
Expand Down
30 changes: 29 additions & 1 deletion packages/scene-composer/src/store/slices/SceneDocumentSlice.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export interface ISceneDocumentSlice {
getSceneNodesByRefs(refs: (string | undefined)[]): (ISceneNodeInternal | undefined)[];
appendSceneNodeInternal(node: ISceneNodeInternal, parentRef?: string): void;
updateSceneNodeInternal(ref: string, partial: RecursivePartial<ISceneNodeInternal>, isTransient?: boolean): void;
updateDocumentInternal(partial: RecursivePartial<Pick<ISceneDocumentInternal, 'unit' | 'rootNodeRefs'>>): void;
updateDocumentInternal(partial: RecursivePartial<Pick<ISceneDocumentInternal, 'unit'>>): void;
listSceneRuleMapIds(): string[];
getSceneRuleMapById(id?: string): Readonly<IRuleBasedMapInternal> | undefined;
updateSceneRuleMapById(id: string, ruleMap: IRuleBasedMapInternal): void;
Expand Down Expand Up @@ -158,8 +158,36 @@ export const createSceneDocumentSlice = (
},

updateSceneNodeInternal: (ref, partial, isTransient) => {
const document = get().document;

set((draft) => {
// update target node
mergeDeep(draft.document.nodeMap[ref], partial);

// Reorder logics
if ('parentRef' in partial) {
const nodeToMove = document.nodeMap[ref];

const oldParentRef = nodeToMove?.parentRef;
const oldParent = document.nodeMap[oldParentRef || ''];

const newParentRef = partial.parentRef;

// remove target node from old parent
if (!oldParentRef) {
draft.document.rootNodeRefs = document.rootNodeRefs.filter((root) => root !== ref);
} else {
draft.document.nodeMap[oldParentRef].childRefs = oldParent.childRefs.filter((child) => child !== ref);
}

// update new parent to have target node as child
if (!newParentRef) {
draft.document.rootNodeRefs.push(ref);
} else {
draft.document.nodeMap[newParentRef].childRefs.push(ref);
}
}

draft.lastOperation = isTransient ? 'updateSceneNodeInternalTransient' : 'updateSceneNodeInternal';
});
},
Expand Down
4 changes: 2 additions & 2 deletions packages/scene-composer/tests/store/slices/EditorStateSlice.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ describe('createEditStateSlice', () => {
// Arrange
const cursorVisible = true;
const draft = { lastOperation: undefined, cursorVisible };
const get = jest.fn();
const get = jest.fn().mockReturnValue({});
const set = jest.fn(((callback) => callback(draft)) as any);

// Act
Expand All @@ -366,7 +366,7 @@ describe('createEditStateSlice', () => {
// Arrange
const cursorStyle = 'edit';
const draft = { lastOperation: undefined, cursorStyle };
const get = jest.fn();
const get = jest.fn().mockReturnValue({});
const set = jest.fn(((callback) => callback(draft)) as any);

// Act
Expand Down
100 changes: 91 additions & 9 deletions packages/scene-composer/tests/store/slices/SceneDocumentSlice.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
/* eslint-disable dot-notation, jest/no-conditional-expect */
import { cloneDeep } from 'lodash';

import serializationHelpers from '../../../src/store/helpers/serializationHelpers';
import interfaceHelpers from '../../../src/store/helpers/interfaceHelpers';
import { createSceneDocumentSlice } from '../../../src/store/slices/SceneDocumentSlice';
Expand Down Expand Up @@ -179,22 +181,102 @@ describe('createSceneDocumentSlice', () => {
});
});

[true, false].forEach((isTransient) => {
it(`should be able to updateSceneNodeInternal ${isTransient ? 'as' : 'as not'} transient`, () => {
describe('updateSceneNodeInternal', () => {
const document = {
nodeMap: {
testNode1: { ref: 'testNode1', childRefs: [], parentRef: 'testNode2' },
testNode2: { ref: 'testNode2', childRefs: ['testNode1'] },
testNode3: { ref: 'testNode3', childRefs: [] },
},
rootNodeRefs: ['testNode2', 'testNode3'],
};

[true, false].forEach((isTransient) => {
it(`should be able to updateSceneNodeInternal ${isTransient ? 'as' : 'as not'} transient`, () => {
// Arrange
const draft = { lastOperation: undefined, document: { nodeMap: { testNode: 'testNode' } } };
const get = jest.fn().mockReturnValue(draft); // fake out get call
const set = jest.fn(((callback) => callback(draft)) as any);

// Act
const { updateSceneNodeInternal } = createSceneDocumentSlice(set, get as any, undefined as any); // api is never used in the function, so it's not needed
updateSceneNodeInternal('testNode', { test: 'test' } as any, isTransient);

// Assert
expect(mergeDeep).toBeCalledWith('testNode', { test: 'test' });
expect(draft.lastOperation!).toEqual(
isTransient ? 'updateSceneNodeInternalTransient' : 'updateSceneNodeInternal',
);
});
});

it(`should be able to updateSceneNodeInternal with different parent`, () => {
// Arrange
const draft = { lastOperation: undefined, document: { nodeMap: { testNode: 'testNode' } } };
const get = jest.fn(); // fake out get call
const draft = { document: cloneDeep(document), lastOperation: undefined };
const get = jest.fn().mockReturnValue({ document }); // fake out get call
const set = jest.fn(((callback) => callback(draft)) as any);

// Act
const { updateSceneNodeInternal } = createSceneDocumentSlice(set, get as any, undefined as any); // api is never used in the function, so it's not needed
updateSceneNodeInternal('testNode', 'partial' as any, isTransient);
updateSceneNodeInternal('testNode1', { parentRef: 'testNode3' });

// Assert
expect(mergeDeep).toBeCalledWith('testNode', 'partial');
expect(draft.lastOperation!).toEqual(
isTransient ? 'updateSceneNodeInternalTransient' : 'updateSceneNodeInternal',
);
expect(mergeDeep).toBeCalledTimes(1);
expect(draft.lastOperation!).toEqual('updateSceneNodeInternal');
expect(draft.document!).toEqual({
nodeMap: {
testNode1: expect.anything(), // mergeDeep is mock and not doing things, therefore not verify it
testNode2: { ref: 'testNode2', childRefs: [] },
testNode3: { ref: 'testNode3', childRefs: ['testNode1'] },
},
rootNodeRefs: ['testNode2', 'testNode3'],
});
});

it(`should be able to updateSceneNodeInternal from child to root`, () => {
// Arrange
const draft = { document: cloneDeep(document), lastOperation: undefined };
const get = jest.fn().mockReturnValue({ document }); // fake out get call
const set = jest.fn(((callback) => callback(draft)) as any);

// Act
const { updateSceneNodeInternal } = createSceneDocumentSlice(set, get as any, undefined as any); // api is never used in the function, so it's not needed
updateSceneNodeInternal('testNode1', { parentRef: undefined });

// Assert
expect(mergeDeep).toBeCalledTimes(1);
expect(draft.lastOperation!).toEqual('updateSceneNodeInternal');
expect(draft.document!).toEqual({
nodeMap: {
testNode1: expect.anything(), // mergeDeep is mock and not doing things, therefore not verify it
testNode2: { ref: 'testNode2', childRefs: [] },
testNode3: { ref: 'testNode3', childRefs: [] },
},
rootNodeRefs: ['testNode2', 'testNode3', 'testNode1'],
});
});

it(`should be able to updateSceneNodeInternal from root to child`, () => {
// Arrange
const draft = { document: cloneDeep(document), lastOperation: undefined };
const get = jest.fn().mockReturnValue({ document }); // fake out get call
const set = jest.fn(((callback) => callback(draft)) as any);

// Act
const { updateSceneNodeInternal } = createSceneDocumentSlice(set, get as any, undefined as any); // api is never used in the function, so it's not needed
updateSceneNodeInternal('testNode3', { parentRef: 'testNode2' });

// Assert
expect(mergeDeep).toBeCalledTimes(1);
expect(draft.lastOperation!).toEqual('updateSceneNodeInternal');
expect(draft.document!).toEqual({
nodeMap: {
testNode1: { ref: 'testNode1', childRefs: [], parentRef: 'testNode2' },
testNode2: { ref: 'testNode2', childRefs: ['testNode1', 'testNode3'] },
testNode3: expect.anything(), // mergeDeep is mock and not doing things, therefore not verify it
},
rootNodeRefs: ['testNode2'],
});
});
});

Expand Down

0 comments on commit b76057d

Please sign in to comment.