From 230ee6c73acd3f79a92b61ff5e665e7dffd78334 Mon Sep 17 00:00:00 2001
From: Mitchell Lee <mitch@evildev.net>
Date: Fri, 19 Aug 2022 17:38:47 -0500
Subject: [PATCH] Fix issue with selecting objects while panning, addressing
 previous feedback (#58)

Co-authored-by: Mitchell Lee <mitchlee@amazon.com>
---
 packages/scene-composer/package.json          |  8 +--
 .../src/components/Tree/TreeItem.tsx          | 12 ++---
 .../SceneHierarchyTreeItem.tsx                |  2 +-
 .../SceneHierarchyTreeItem.spec.tsx.snap      |  2 +-
 .../__tests__/SubModelTree.spec.tsx           |  8 +--
 .../components/SubModelTree/index.tsx         | 22 ++++----
 .../__tests__/EntityGroup.spec.tsx            | 52 ++++---------------
 .../three-fiber/EntityGroup/index.tsx         | 37 +++++++------
 .../EntityGroup/useCallbackWhenNotPanning.ts  | 47 +++++++++++++++++
 9 files changed, 103 insertions(+), 87 deletions(-)
 create mode 100644 packages/scene-composer/src/components/three-fiber/EntityGroup/useCallbackWhenNotPanning.ts

diff --git a/packages/scene-composer/package.json b/packages/scene-composer/package.json
index db0150957..c60e3edcd 100644
--- a/packages/scene-composer/package.json
+++ b/packages/scene-composer/package.json
@@ -151,10 +151,10 @@
   "jest": {
     "coverageThreshold": {
       "global": {
-        "lines": 76.85,
-        "statements": 75.98,
-        "functions": 75.29,
-        "branches": 62.1,
+        "lines": 76.64,
+        "statements": 75.77,
+        "functions": 75.05,
+        "branches": 61.93,
         "branchesTrue": 100
       }
     }
diff --git a/packages/scene-composer/src/components/Tree/TreeItem.tsx b/packages/scene-composer/src/components/Tree/TreeItem.tsx
index 57661f6de..3a5d23448 100644
--- a/packages/scene-composer/src/components/Tree/TreeItem.tsx
+++ b/packages/scene-composer/src/components/Tree/TreeItem.tsx
@@ -81,13 +81,11 @@ const TreeItem = React.forwardRef<HTMLLIElement, TreeItemProps>(
       >
         <TreeItemInner selected={selected} selectable={selectable} onActivated={onActivated} onSelected={onSelected}>
           {expandable && (
-            <>
-              <Button
-                variant='inline-icon'
-                onClick={expandHandler}
-                iconName={`treeview-${expanded ? 'collapse' : 'expand'}`}
-              />
-            </>
+            <Button
+              variant='inline-icon'
+              onClick={expandHandler}
+              iconName={`treeview-${expanded ? 'collapse' : 'expand'}`}
+            />
           )}
           {labelText}
         </TreeItemInner>
diff --git a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/SceneHierarchyTreeItem.tsx b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/SceneHierarchyTreeItem.tsx
index 8ca8a7d9b..2ff034fbc 100644
--- a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/SceneHierarchyTreeItem.tsx
+++ b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/SceneHierarchyTreeItem.tsx
@@ -107,7 +107,7 @@ const SceneHierarchyTreeItem: FC<SceneHierarchyTreeItemProps> = ({
           {childNodes.map((node) => (
             <SceneHierarchyTreeItem key={node.objectRef} enableDragAndDrop={enableDragAndDrop} {...node} />
           ))}
-          {showSubModel && <SubModelTree parentRef={key} expanded={false} object={model!} selectable />}
+          {showSubModel && <SubModelTree parentRef={key} expanded={false} object3D={model!} selectable />}
         </EnhancedTree>
       )}
     </EnhancedTreeItem>
diff --git a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/__tests__/__snapshots__/SceneHierarchyTreeItem.spec.tsx.snap b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/__tests__/__snapshots__/SceneHierarchyTreeItem.spec.tsx.snap
index f925bd431..138acece0 100644
--- a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/__tests__/__snapshots__/SceneHierarchyTreeItem.spec.tsx.snap
+++ b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SceneHierarchyTree/__tests__/__snapshots__/SceneHierarchyTreeItem.spec.tsx.snap
@@ -99,7 +99,7 @@ exports[`SceneHierarchyTreeItem should render SubModelTree when item has a model
     >
       <div
         data-mocked="SubModelTree"
-        object="[object Object]"
+        object3d="[object Object]"
         parentref="1"
       />
     </ol>
diff --git a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/__tests__/SubModelTree.spec.tsx b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/__tests__/SubModelTree.spec.tsx
index a83e0be29..7bffda35d 100644
--- a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/__tests__/SubModelTree.spec.tsx
+++ b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/__tests__/SubModelTree.spec.tsx
@@ -41,7 +41,7 @@ describe('SubModelTree', () => {
 
     const parentRef = '112';
 
-    const { container } = render(<SubModelTree parentRef={parentRef} object={object} />);
+    const { container } = render(<SubModelTree parentRef={parentRef} object3D={object} />);
     expect(container).toMatchSnapshot();
   });
 
@@ -58,7 +58,7 @@ describe('SubModelTree', () => {
 
     (useMaterialEffect as jest.Mock).mockImplementation(() => [transform, restore]);
 
-    const { findByText } = render(<SubModelTree parentRef={parentRef} object={object} />);
+    const { findByText } = render(<SubModelTree parentRef={parentRef} object3D={object} />);
 
     const label = await findByText('RootObject');
 
@@ -92,7 +92,7 @@ describe('SubModelTree', () => {
 
       (useMaterialEffect as jest.Mock).mockImplementation(() => [jest.fn(), jest.fn()]);
 
-      render(<SubModelTree parentRef={parentRef} object={object} />);
+      render(<SubModelTree parentRef={parentRef} object3D={object} />);
 
       const [onVisibilityToggled] = callbacks;
 
@@ -120,7 +120,7 @@ describe('SubModelTree', () => {
 
       (useMaterialEffect as jest.Mock).mockImplementation(() => [jest.fn(), jest.fn()]);
 
-      render(<SubModelTree parentRef={parentRef} object={object} />);
+      render(<SubModelTree parentRef={parentRef} object3D={object} />);
 
       // Act
       const [, onCreate] = callbacks;
diff --git a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/index.tsx b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/index.tsx
index 810a5571b..69d335936 100644
--- a/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/index.tsx
+++ b/packages/scene-composer/src/components/panels/SceneHierarchyPanel/components/SubModelTree/index.tsx
@@ -13,7 +13,7 @@ import TreeItemLabel from './SubModelTreeItemLabel';
 
 export interface SubModelTreeProps {
   parentRef: string;
-  object: Object3D<Event>;
+  object3D: Object3D<Event>;
   selected?: boolean;
   visible?: boolean;
   selectable?: boolean;
@@ -22,7 +22,7 @@ export interface SubModelTreeProps {
 
 const SubModelTree: FC<SubModelTreeProps> = ({
   parentRef,
-  object,
+  object3D,
   expanded: defaultExpanded = true,
   visible: defaultVisible = true,
 }) => {
@@ -34,7 +34,7 @@ const SubModelTree: FC<SubModelTreeProps> = ({
 
   const hoverColor = new Color(0x00ff00);
 
-  const { name, children: allNodes } = object;
+  const { name, children: allNodes } = object3D;
   const nodes = allNodes.filter((n) => !!n.name); // Only nodes with Names will appear as viable submodels
 
   const [transform, restore] = useMaterialEffect(
@@ -43,33 +43,33 @@ const SubModelTree: FC<SubModelTreeProps> = ({
         o.material.color = hoverColor;
       }
     },
-    object,
+    object3D,
   );
 
   const onVisibilityToggled = useCallback((show) => {
-    object.visible = show;
+    object3D.visible = show;
     setVisible(show);
   }, []);
 
   const onCreate = useCallback(() => {
-    const nodeRef = `${parentRef}#${object.id}`;
+    const nodeRef = `${parentRef}#${object3D.id}`;
     const subModelComponent: ISubModelRefComponent = {
       type: KnownComponentType.SubModelRef,
       parentRef,
       ref: generateUUID(),
-      selector: object.name,
+      selector: object3D.name,
     };
 
     const node = {
       ref: nodeRef,
-      name: object.name,
+      name: object3D.name,
       components: [subModelComponent as ISceneComponentInternal],
       parentRef,
     } as ISceneNodeInternal;
 
     appendSceneNodeInternal(node);
-    setSceneNodeObject3DMapping(nodeRef, object); // Cache Reference
-  }, [object]);
+    setSceneNodeObject3DMapping(nodeRef, object3D); // Cache Reference
+  }, [object3D]);
 
   const onHover = useCallback((e) => {
     e.preventDefault();
@@ -105,7 +105,7 @@ const SubModelTree: FC<SubModelTreeProps> = ({
       {nodes.length > 0 && (
         <Tree className={'tm-submodel-tree'}>
           {nodes.map((c) => (
-            <SubModelTree key={c.id} parentRef={parentRef} object={c} />
+            <SubModelTree key={c.id} parentRef={parentRef} object3D={c} />
           ))}
         </Tree>
       )}
diff --git a/packages/scene-composer/src/components/three-fiber/EntityGroup/__tests__/EntityGroup.spec.tsx b/packages/scene-composer/src/components/three-fiber/EntityGroup/__tests__/EntityGroup.spec.tsx
index a1a02e684..0163b5d4b 100644
--- a/packages/scene-composer/src/components/three-fiber/EntityGroup/__tests__/EntityGroup.spec.tsx
+++ b/packages/scene-composer/src/components/three-fiber/EntityGroup/__tests__/EntityGroup.spec.tsx
@@ -1,8 +1,8 @@
 import React from 'react';
-import { fireEvent, render } from '@testing-library/react';
+import { render } from '@testing-library/react';
 
 import EntityGroup from '..';
-import { useEditorState, useSceneDocument } from '../../../../store';
+import { useSceneDocument } from '../../../../store';
 import { fakeSceneNode } from '../fakers';
 
 jest.mock('../../../../store', () => ({
@@ -18,6 +18,14 @@ jest.mock('../../../../store', () => ({
   })),
 }));
 
+jest.mock('../useCallbackWhenNotPanning', () => (cb) => [
+  jest.fn(),
+  function Hack(e) {
+    console.log(e);
+    cb(e);
+  },
+]);
+
 describe('<EntityGroup />', () => {
   it('should render expected DOM when rendered', () => {
     const childRefs = [1, 2, 3].map((i) => fakeSceneNode(`${i}`));
@@ -34,44 +42,4 @@ describe('<EntityGroup />', () => {
     const { container } = render(<EntityGroup node={node} />);
     expect(container).toMatchSnapshot();
   });
-
-  it('should select the corresponding scene node when the group is clicked', async () => {
-    const node = fakeSceneNode('EntityGroup');
-    const setSelectedSceneNodeRef = jest.fn();
-
-    (useEditorState as jest.Mock).mockImplementation(() => ({
-      selectedSceneNodeRef: undefined,
-      setSelectedSceneNodeRef,
-      getObject3DBySceneNodeRef: jest.fn(),
-      setSceneNodeObject3DMapping: jest.fn(),
-    }));
-
-    const { container } = render(<EntityGroup node={node} />);
-
-    const group = container.children[0];
-
-    fireEvent.click(group);
-
-    expect(setSelectedSceneNodeRef).toBeCalledWith(node.ref);
-  });
-
-  it('should deselect the corresponding scene node when it is clicked', async () => {
-    const node = fakeSceneNode('EntityGroup');
-    const setSelectedSceneNodeRef = jest.fn();
-
-    (useEditorState as jest.Mock).mockImplementation(() => ({
-      selectedSceneNodeRef: node.ref,
-      setSelectedSceneNodeRef,
-      getObject3DBySceneNodeRef: jest.fn(),
-      setSceneNodeObject3DMapping: jest.fn(),
-    }));
-
-    const { container } = render(<EntityGroup node={node} />);
-
-    const group = container.children[0];
-
-    fireEvent.click(group);
-
-    expect(setSelectedSceneNodeRef).toBeCalledWith(undefined);
-  });
 });
diff --git a/packages/scene-composer/src/components/three-fiber/EntityGroup/index.tsx b/packages/scene-composer/src/components/three-fiber/EntityGroup/index.tsx
index ff38185ca..7a40e8bea 100644
--- a/packages/scene-composer/src/components/three-fiber/EntityGroup/index.tsx
+++ b/packages/scene-composer/src/components/three-fiber/EntityGroup/index.tsx
@@ -12,6 +12,7 @@ import { sceneComposerIdContext, useSceneComposerId } from '../../../common/scen
 import { getChildrenGroupName, getEntityGroupName } from '../../../utils/objectThreeUtils';
 import { KnownComponentType } from '../../../interfaces';
 
+import useCallbackWhenNotPanning from './useCallbackWhenNotPanning';
 import ComponentGroup from './ComponentGroup';
 
 interface IEntityGroupProps {
@@ -50,8 +51,9 @@ const EntityGroup = ({ node }: IEntityGroupProps) => {
   const { getObject3DBySceneNodeRef, selectedSceneNodeRef, setSceneNodeObject3DMapping, setSelectedSceneNodeRef } =
     useEditorState(sceneComposerId);
 
-  const onClick = useCallback(
+  const [onPointerDown, onPointerUp] = useCallbackWhenNotPanning(
     (e) => {
+      console.log('got here');
       e.stopPropagation(); // the most nested object in the click scope should get selected, and not bubble up to the parent.
       if (selectedSceneNodeRef === nodeRef) {
         setSelectedSceneNodeRef(undefined);
@@ -62,6 +64,8 @@ const EntityGroup = ({ node }: IEntityGroupProps) => {
     [selectedSceneNodeRef, nodeRef],
   );
 
+  console.log(onPointerUp);
+
   const setEntityGroupObject3DRef = useCallback(
     (obj3d: any) => {
       object3dRef.current = obj3d;
@@ -88,22 +92,21 @@ const EntityGroup = ({ node }: IEntityGroupProps) => {
   );
 
   return (
-    <Fragment>
-      <group
-        name={getEntityGroupName(node.ref)}
-        key={node.ref}
-        ref={setEntityGroupObject3DRef}
-        position={position}
-        rotation={new Euler(...rotation, 'XYZ')}
-        scale={scale}
-        dispose={null}
-        onClick={onClick}
-        userData={{ nodeRef, componentTypes }}
-      >
-        <ComponentGroup node={node} components={node.components} />
-        <ChildGroup node={node} />
-      </group>
-    </Fragment>
+    <group
+      name={getEntityGroupName(node.ref)}
+      key={node.ref}
+      ref={setEntityGroupObject3DRef}
+      position={position}
+      rotation={new Euler(...rotation, 'XYZ')}
+      scale={scale}
+      dispose={null}
+      onPointerDown={onPointerDown}
+      onPointerUp={onPointerUp}
+      userData={{ nodeRef, componentTypes }}
+    >
+      <ComponentGroup node={node} components={node.components} />
+      <ChildGroup node={node} />
+    </group>
   );
 };
 
diff --git a/packages/scene-composer/src/components/three-fiber/EntityGroup/useCallbackWhenNotPanning.ts b/packages/scene-composer/src/components/three-fiber/EntityGroup/useCallbackWhenNotPanning.ts
new file mode 100644
index 000000000..34d4d9293
--- /dev/null
+++ b/packages/scene-composer/src/components/three-fiber/EntityGroup/useCallbackWhenNotPanning.ts
@@ -0,0 +1,47 @@
+import { ThreeEvent } from '@react-three/fiber';
+import { useRef, useCallback } from 'react';
+
+/**
+ * This is not generic enough a hook to be used in the rest of the application, and has
+ * a dumb name but I can't think of a better one. It's necessary to avoid selecting things
+ * unncessarily, but it's also kind of use case specific. Let's see if similar use cases
+ * preset themselves int he future, and we can get a better idea of the right way to design
+ * this.
+ * @param callback Callback method to call when the state is valid, and the user clicks the target
+ * @param deps An array of objects that should trigger a rereder of this hook
+ * @param acceptableDriftDistance optional override of the acceptable mouse move distance to before we treat something as being "dragged"
+ * @returns onPointerDown and onPointerUp event handlers (you should assign these to your component)
+ */
+const useCallbackWhenNotPanning = (callback, deps, acceptableDriftDistance = 1) => {
+  const lastPointerDownLocation = useRef<[number, number] | null>(null);
+
+  const onPointerUp = useCallback(
+    (e: ThreeEvent<MouseEvent>) => {
+      const lastPointerPosition = lastPointerDownLocation.current;
+      const { clientX, clientY } = e;
+      if (!lastPointerPosition) {
+        return;
+      }
+
+      const [lastX, lastY] = lastPointerPosition;
+      const isPanning: boolean =
+        Math.abs(clientX - lastX) > acceptableDriftDistance || Math.abs(clientY - lastY) > acceptableDriftDistance;
+
+      if (isPanning) return;
+
+      callback(e);
+    },
+    [callback, ...deps],
+  );
+
+  const onPointerDown = useCallback(
+    ({ clientX, clientY }) => {
+      lastPointerDownLocation.current = [clientX, clientY];
+    },
+    [callback, ...deps],
+  );
+
+  return [onPointerDown, onPointerUp];
+};
+
+export default useCallbackWhenNotPanning;