Fix aggregate keyframe dragging stopping in an edge case when the key for the drag element changes (#189)

Co-authored-by: Cole Lawrence <cole@colelawrence.com>

theatre/studio/src/panels/SequenceEditorPanel/DopeSheet/Right/AggregatedKeyframeTrack/AggregateKeyframeEditor/AggregateKeyframeDot.tsx

@@ -1,19 +1,11 @@
-import {val} from '@theatre/dataverse'
+import React from 'react'
-import React, {useMemo, useRef} from 'react'
-import {AggregateKeyframePositionIsSelected} from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/AggregatedKeyframeTrack/AggregatedKeyframeTrack'
 import useRefAndState from '@theatre/studio/utils/useRefAndState'
-import type {UseDragOpts} from '@theatre/studio/uiComponents/useDrag'
-import type {CommitOrDiscard} from '@theatre/studio/StudioStore/StudioStore'
-import getStudio from '@theatre/studio/getStudio'
-import useDrag from '@theatre/studio/uiComponents/useDrag'
 import {useLogger} from '@theatre/studio/uiComponents/useLogger'
 import useContextMenu from '@theatre/studio/uiComponents/simpleContextMenu/useContextMenu'
-import {useLockFrameStampPosition} from '@theatre/studio/panels/SequenceEditorPanel/FrameStampPositionProvider'
-import {useCssCursorLock} from '@theatre/studio/uiComponents/PointerEventsHandler'
-import DopeSnap from '@theatre/studio/panels/SequenceEditorPanel/RightOverlay/DopeSnap'
 import type {IAggregateKeyframeEditorProps} from './AggregateKeyframeEditor'
 import type {IAggregateKeyframeEditorUtils} from './useAggregateKeyframeEditorUtils'
 import {AggregateKeyframeVisualDot, HitZone} from './AggregateKeyframeVisualDot'
+import getStudio from '@theatre/studio/getStudio'
 import {
   copyableKeyframesFromSelection,
   keyframesWithPaths,
@@ -21,12 +13,7 @@ import {
 import type {KeyframeWithPathToPropFromCommonRoot} from '@theatre/studio/store/types/ahistoric'
 import {commonRootOfPathsToProps} from '@theatre/shared/utils/addresses'
 import type {ILogger} from '@theatre/shared/logger'
-import {
+import DopeSnap from '@theatre/studio/panels/SequenceEditorPanel/RightOverlay/DopeSnap'
-  collectKeyframeSnapPositions,
-  snapToNone,
-  snapToSome,
-} from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/KeyframeSnapTarget'
-import type {Keyframe} from '@theatre/core/projects/store/types/SheetState_Historic'
 
 type IAggregateKeyframeDotProps = {
   editorProps: IAggregateKeyframeEditorProps
@@ -40,18 +27,17 @@ export function AggregateKeyframeDot(
   const {cur} = props.utils
 
   const [ref, node] = useRefAndState<HTMLDivElement | null>(null)
-  const [isDragging] = useDragForAggregateKeyframeDot(node, props, {
-    onClickFromDrag(dragStartEvent) {
-      // TODO Aggregate inline keyframe editor
-      // openEditor(dragStartEvent, ref.current!)
-    },
-  })
 
   const [contextMenu] = useAggregateKeyframeContextMenu(props, logger, node)
 
   return (
     <>
-      <HitZone ref={ref} />
+      <HitZone
+        ref={ref}
+        // Need this for the dragging logic to be able to get the keyframe props
+        // based on the position.
+        {...DopeSnap.includePositionSnapAttrs(cur.position)}
+      />
       <AggregateKeyframeVisualDot
         isAllHere={cur.allHere}
         isSelected={cur.selected}
@@ -144,149 +130,3 @@ function useAggregateKeyframeContextMenu(
     },
   })
 }
-
-function useDragForAggregateKeyframeDot(
-  node: HTMLDivElement | null,
-  props: IAggregateKeyframeDotProps,
-  options: {
-    /**
-     * hmm: this is a hack so we can actually receive the
-     * {@link MouseEvent} from the drag event handler and use
-     * it for positioning the popup.
-     */
-    onClickFromDrag(dragStartEvent: MouseEvent): void
-  },
-): [isDragging: boolean] {
-  const propsRef = useRef(props.editorProps)
-  propsRef.current = props.editorProps
-  const keyframesRef = useRef(props.utils.cur.keyframes)
-  keyframesRef.current = props.utils.cur.keyframes
-
-  const useDragOpts = useMemo<UseDragOpts>(() => {
-    return {
-      debugName: 'AggregateKeyframeDot/useDragKeyframe',
-      onDragStart(event) {
-        const props = propsRef.current
-        const keyframes = keyframesRef.current
-
-        const tracksByObject = val(
-          getStudio()!.atomP.historic.coreByProject[
-            props.viewModel.sheetObject.address.projectId
-          ].sheetsById[props.viewModel.sheetObject.address.sheetId].sequence
-            .tracksByObject,
-        )!
-
-        // Calculate all the valid snap positions in the sequence editor,
-        // excluding the child keyframes of this aggregate, and any selection it is part of.
-        const snapPositions = collectKeyframeSnapPositions(
-          tracksByObject,
-          function shouldIncludeKeyfram(keyframe, {trackId, objectKey}) {
-            return (
-              // we exclude all the child keyframes of this aggregate keyframe from being a snap target
-              keyframes.every(
-                (kfWithTrack) => keyframe.id !== kfWithTrack.kf.id,
-              ) &&
-              !(
-                // if all of the children of the current aggregate keyframe are in a selection,
-                (
-                  props.selection &&
-                  // then we exclude them and all other keyframes in the selection from being snap targets
-                  props.selection.byObjectKey[objectKey]?.byTrackId[trackId]
-                    ?.byKeyframeId[keyframe.id]
-                )
-              )
-            )
-          },
-        )
-
-        snapToSome(snapPositions)
-
-        if (
-          props.selection &&
-          props.aggregateKeyframes[props.index].selected ===
-            AggregateKeyframePositionIsSelected.AllSelected
-        ) {
-          const {selection, viewModel} = props
-          const {sheetObject} = viewModel
-          const handlers = selection
-            .getDragHandlers({
-              ...sheetObject.address,
-              domNode: node!,
-              positionAtStartOfDrag: keyframes[0].kf.position,
-            })
-            .onDragStart(event)
-
-          return (
-            handlers && {
-              ...handlers,
-              onClick: options.onClickFromDrag,
-              onDragEnd: (...args) => {
-                handlers.onDragEnd?.(...args)
-                snapToNone()
-              },
-            }
-          )
-        }
-
-        const propsAtStartOfDrag = props
-        const toUnitSpace = val(
-          propsAtStartOfDrag.layoutP.scaledSpace.toUnitSpace,
-        )
-
-        let tempTransaction: CommitOrDiscard | undefined
-
-        return {
-          onDrag(dx, dy, event) {
-            const newPosition = Math.max(
-              // check if our event hoversover a [data-pos] element
-              DopeSnap.checkIfMouseEventSnapToPos(event, {
-                ignore: node,
-              }) ??
-                // if we don't find snapping target, check the distance dragged + original position
-                keyframes[0].kf.position + toUnitSpace(dx),
-              // sanitize to minimum of zero
-              0,
-            )
-
-            tempTransaction?.discard()
-            tempTransaction = undefined
-            tempTransaction = getStudio()!.tempTransaction(({stateEditors}) => {
-              for (const keyframe of keyframes) {
-                const original = keyframe.kf
-                stateEditors.coreByProject.historic.sheetsById.sequence.replaceKeyframes(
-                  {
-                    ...propsAtStartOfDrag.viewModel.sheetObject.address,
-                    trackId: keyframe.track.id,
-                    keyframes: [{...original, position: newPosition}],
-                    snappingFunction: val(
-                      propsAtStartOfDrag.layoutP.sheet,
-                    ).getSequence().closestGridPosition,
-                  },
-                )
-              }
-            })
-          },
-          onDragEnd(dragHappened) {
-            if (dragHappened) {
-              tempTransaction?.commit()
-            } else {
-              tempTransaction?.discard()
-            }
-
-            snapToNone()
-          },
-          onClick(ev) {
-            options.onClickFromDrag(ev)
-          },
-        }
-      },
-    }
-  }, [])
-
-  const [isDragging] = useDrag(node, useDragOpts)
-
-  useLockFrameStampPosition(isDragging, props.utils.cur.position)
-  useCssCursorLock(isDragging, 'draggingPositionInSequenceEditor', 'ew-resize')
-
-  return [isDragging]
-}

theatre/studio/src/panels/SequenceEditorPanel/DopeSheet/Right/AggregatedKeyframeTrack/AggregateKeyframeEditor/useAggregateKeyframeEditorUtils.tsx

@@ -24,7 +24,26 @@ export function useAggregateKeyframeEditorUtils(
   const {index, aggregateKeyframes, selection} = props
   const sheetObjectAddress = props.viewModel.sheetObject.address
 
-  return usePrism(() => {
+  return usePrism(getAggregateKeyframeEditorUtilsPrismFn(props), [
+    index,
+    aggregateKeyframes,
+    selection,
+    sheetObjectAddress,
+  ])
+}
+
+// I think this was pulled out for performance
+// 1/10: Not sure this is properly split up
+export function getAggregateKeyframeEditorUtilsPrismFn(
+  props: Pick<
+    IAggregateKeyframeEditorProps,
+    'index' | 'aggregateKeyframes' | 'selection' | 'viewModel'
+  >,
+) {
+  const {index, aggregateKeyframes, selection} = props
+  const sheetObjectAddress = props.viewModel.sheetObject.address
+
+  return () => {
     const cur = aggregateKeyframes[index]
     const next = aggregateKeyframes[index + 1]
 
@@ -80,5 +99,5 @@ export function useAggregateKeyframeEditorUtils(
       isAggregateEditingInCurvePopover,
       allConnections,
     }
-  }, [index, aggregateKeyframes, selection, sheetObjectAddress])
+  }
 }

theatre/studio/src/panels/SequenceEditorPanel/DopeSheet/Right/AggregatedKeyframeTrack/AggregatedKeyframeTrack.tsx

@@ -6,20 +6,28 @@ import type {
   SequenceEditorTree_PropWithChildren,
   SequenceEditorTree_SheetObject,
 } from '@theatre/studio/panels/SequenceEditorPanel/layout/tree'
-import type {Keyframe} from '@theatre/core/projects/store/types/SheetState_Historic'
 import {usePrism, useVal} from '@theatre/react'
 import type {Pointer} from '@theatre/dataverse'
-import {valueDerivation} from '@theatre/dataverse'
+import {prism, val, valueDerivation} from '@theatre/dataverse'
-import {val} from '@theatre/dataverse'
+import React, {useMemo, Fragment} from 'react'
-import React, {Fragment, useMemo} from 'react'
 import styled from 'styled-components'
 import type {IContextMenuItem} from '@theatre/studio/uiComponents/simpleContextMenu/useContextMenu'
 import useContextMenu from '@theatre/studio/uiComponents/simpleContextMenu/useContextMenu'
 import useRefAndState from '@theatre/studio/utils/useRefAndState'
-import type {IAggregateKeyframesAtPosition} from './AggregateKeyframeEditor/AggregateKeyframeEditor'
+import type {
+  IAggregateKeyframesAtPosition,
+  IAggregateKeyframeEditorProps,
+} from './AggregateKeyframeEditor/AggregateKeyframeEditor'
 import AggregateKeyframeEditor from './AggregateKeyframeEditor/AggregateKeyframeEditor'
 import type {AggregatedKeyframes} from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/collectAggregateKeyframes'
 import {useLogger} from '@theatre/studio/uiComponents/useLogger'
+import {getAggregateKeyframeEditorUtilsPrismFn} from './AggregateKeyframeEditor/useAggregateKeyframeEditorUtils'
+import DopeSnap from '@theatre/studio/panels/SequenceEditorPanel/RightOverlay/DopeSnap'
+import type {UseDragOpts} from '@theatre/studio/uiComponents/useDrag'
+import type {CommitOrDiscard} from '@theatre/studio/StudioStore/StudioStore'
+import useDrag from '@theatre/studio/uiComponents/useDrag'
+import {useLockFrameStampPositionRef} from '@theatre/studio/panels/SequenceEditorPanel/FrameStampPositionProvider'
+import {useCssCursorLock} from '@theatre/studio/uiComponents/PointerEventsHandler'
 import getStudio from '@theatre/studio/getStudio'
 import type {SheetObjectAddress} from '@theatre/shared/utils/addresses'
 import {
@@ -29,12 +37,18 @@ import {
 } from '@theatre/shared/utils/addresses'
 import type {SequenceTrackId} from '@theatre/shared/utils/ids'
 import type Sequence from '@theatre/core/sequences/Sequence'
-import type {KeyframeWithPathToPropFromCommonRoot} from '@theatre/studio/store/types'
-import {collectAggregateSnapPositions} from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/collectAggregateKeyframes'
 import KeyframeSnapTarget, {
   snapPositionsStateD,
 } from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/KeyframeSnapTarget'
 import {emptyObject} from '@theatre/shared/utils'
+import type {KeyframeWithPathToPropFromCommonRoot} from '@theatre/studio/store/types'
+import {
+  collectKeyframeSnapPositions,
+  snapToNone,
+  snapToSome,
+} from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/KeyframeSnapTarget'
+import {collectAggregateSnapPositions} from '@theatre/studio/panels/SequenceEditorPanel/DopeSheet/Right/collectAggregateKeyframes'
+import type {Keyframe} from '@theatre/core/projects/store/types/SheetState_Historic'
 
 const AggregatedKeyframeTrackContainer = styled.div`
   position: relative;
@@ -115,24 +129,47 @@ function AggregatedKeyframeTrack_memo(props: IAggregatedKeyframeTracksProps) {
     />
   ))
 
-  const keyframeEditors = posKfs.map(({position, keyframes}, index) => (
+  const keyframeEditorProps = posKfs.map(
-    <Fragment key={'agg-' + keyframes[0].kf.id}>
+    (
+      {position, keyframes},
+      index,
+    ): {editorProps: IAggregateKeyframeEditorProps; position: number} => ({
+      position,
+      editorProps: {
+        index,
+        layoutP,
+        viewModel,
+        aggregateKeyframes: posKfs,
+        selection: selectedPositions.has(position) ? selection : undefined,
+      },
+    }),
+  )
+
+  const [isDragging] = useDragForAggregateKeyframeDot(
+    containerNode,
+    (position) => {
+      return keyframeEditorProps.find(
+        (editorProp) => editorProp.position === position,
+      )?.editorProps
+    },
+    {
+      onClickFromDrag(dragStartEvent) {
+        // TODO Aggregate inline keyframe editor
+        // openEditor(dragStartEvent, ref.current!)
+      },
+    },
+  )
+
+  const keyframeEditors = keyframeEditorProps.map((props, i) => (
+    <Fragment key={'agg-' + posKfs[i].keyframes[0].kf.id}>
       {snapToAllKeyframes && (
         <KeyframeSnapTarget
           layoutP={layoutP}
           leaf={viewModel}
-          position={position}
+          position={props.position}
         />
       )}
-      <AggregateKeyframeEditor
+      <AggregateKeyframeEditor {...props.editorProps} />
-        index={index}
-        layoutP={layoutP}
-        viewModel={viewModel}
-        aggregateKeyframes={posKfs}
-        selection={
-          selectedPositions.has(position) === true ? selection : undefined
-        }
-      />
     </Fragment>
   ))
 
@@ -422,3 +459,163 @@ function earliestKeyframe(keyframes: Keyframe[]) {
   }
   return curEarliest
 }
+
+function useDragForAggregateKeyframeDot(
+  containerNode: HTMLDivElement | null,
+  getPropsForPosition: (
+    position: number,
+  ) => IAggregateKeyframeEditorProps | undefined,
+  options: {
+    /**
+     * hmm: this is a hack so we can actually receive the
+     * {@link MouseEvent} from the drag event handler and use
+     * it for positioning the popup.
+     */
+    onClickFromDrag(dragStartEvent: MouseEvent): void
+  },
+): [isDragging: boolean] {
+  const logger = useLogger('useDragForAggregateKeyframeDot')
+  const frameStampLock = useLockFrameStampPositionRef()
+  const useDragOpts = useMemo<UseDragOpts>(() => {
+    return {
+      debugName: 'AggregateKeyframeDot/useDragKeyframe',
+      onDragStart(event) {
+        logger._debug('onDragStart', {target: event.target})
+        console.log(event.target)
+        const positionToFind = Number((event.target as HTMLElement).dataset.pos)
+        const props = getPropsForPosition(positionToFind)
+        if (!props) {
+          console.log('exit')
+          logger._debug('no props found for ', {positionToFind})
+          return false
+        }
+
+        frameStampLock(true, positionToFind)
+        const keyframes = prism(
+          getAggregateKeyframeEditorUtilsPrismFn(props),
+        ).getValue().cur.keyframes
+
+        const tracksByObject = val(
+          getStudio()!.atomP.historic.coreByProject[
+            props.viewModel.sheetObject.address.projectId
+          ].sheetsById[props.viewModel.sheetObject.address.sheetId].sequence
+            .tracksByObject,
+        )!
+
+        // Calculate all the valid snap positions in the sequence editor,
+        // excluding the child keyframes of this aggregate, and any selection it is part of.
+        const snapPositions = collectKeyframeSnapPositions(
+          tracksByObject,
+          function shouldIncludeKeyfram(keyframe, {trackId, objectKey}) {
+            return (
+              // we exclude all the child keyframes of this aggregate keyframe from being a snap target
+              keyframes.every(
+                (kfWithTrack) => keyframe.id !== kfWithTrack.kf.id,
+              ) &&
+              !(
+                // if all of the children of the current aggregate keyframe are in a selection,
+                (
+                  props.selection &&
+                  // then we exclude them and all other keyframes in the selection from being snap targets
+                  props.selection.byObjectKey[objectKey]?.byTrackId[trackId]
+                    ?.byKeyframeId[keyframe.id]
+                )
+              )
+            )
+          },
+        )
+
+        snapToSome(snapPositions)
+
+        if (
+          props.selection &&
+          props.aggregateKeyframes[props.index].selected ===
+            AggregateKeyframePositionIsSelected.AllSelected
+        ) {
+          const {selection, viewModel} = props
+          const {sheetObject} = viewModel
+          const handlers = selection
+            .getDragHandlers({
+              ...sheetObject.address,
+              domNode: containerNode!,
+              positionAtStartOfDrag: keyframes[0].kf.position,
+            })
+            .onDragStart(event)
+
+          return (
+            handlers && {
+              ...handlers,
+              onClick: options.onClickFromDrag,
+              onDragEnd: (...args) => {
+                handlers.onDragEnd?.(...args)
+                snapToNone()
+              },
+            }
+          )
+        }
+
+        const propsAtStartOfDrag = props
+        const toUnitSpace = val(
+          propsAtStartOfDrag.layoutP.scaledSpace.toUnitSpace,
+        )
+
+        let tempTransaction: CommitOrDiscard | undefined
+
+        return {
+          onDrag(dx, dy, event) {
+            const newPosition = Math.max(
+              // check if our event hovers over a [data-pos] element
+              DopeSnap.checkIfMouseEventSnapToPos(event, {
+                // ignore: node,
+              }) ??
+                // if we don't find snapping target, check the distance dragged + original position
+                keyframes[0].kf.position + toUnitSpace(dx),
+              // sanitize to minimum of zero
+              0,
+            )
+
+            frameStampLock(true, newPosition)
+
+            tempTransaction?.discard()
+            tempTransaction = undefined
+            tempTransaction = getStudio().tempTransaction(({stateEditors}) => {
+              for (const keyframe of keyframes) {
+                const original = keyframe.kf
+                stateEditors.coreByProject.historic.sheetsById.sequence.replaceKeyframes(
+                  {
+                    ...propsAtStartOfDrag.viewModel.sheetObject.address,
+                    trackId: keyframe.track.id,
+                    keyframes: [{...original, position: newPosition}],
+                    snappingFunction: val(
+                      propsAtStartOfDrag.layoutP.sheet,
+                    ).getSequence().closestGridPosition,
+                  },
+                )
+              }
+            })
+          },
+          onDragEnd(dragHappened) {
+            frameStampLock(false, -1)
+            if (dragHappened) {
+              tempTransaction?.commit()
+            } else {
+              tempTransaction?.discard()
+              options.onClickFromDrag(event)
+            }
+
+            snapToNone()
+          },
+          onClick(ev) {
+            options.onClickFromDrag(ev)
+          },
+        }
+      },
+    }
+  }, [getPropsForPosition, options.onClickFromDrag])
+
+  const [isDragging] = useDrag(containerNode, useDragOpts)
+
+  useCssCursorLock(isDragging, 'draggingPositionInSequenceEditor', 'ew-resize')
+
+  return [isDragging]
+}

theatre/studio/src/panels/SequenceEditorPanel/FrameStampPositionProvider.tsx

@@ -123,6 +123,39 @@ const FrameStampPositionProvider: React.FC<{
 
 export const useFrameStampPositionD = () => useContext(context).currentD
 
+/** Version of {@link useLockFrameStampPosition} which allows you to directly set status of a lock. */
+export const useLockFrameStampPositionRef = () => {
+  const {getLock} = useContext(context)
+  const lockRef = useRef<undefined | ReturnType<typeof getLock>>()
+
+  useLayoutEffect(() => {
+    return () => {
+      lockRef.current!.unlock()
+    }
+  }, [val])
+
+  return useMemo(() => {
+    let prevShouldLock: false | {pos: number} = false
+    return (shouldLock: boolean, posValue: number) => {
+      if (shouldLock === prevShouldLock) return
+      if (shouldLock) {
+        if (!prevShouldLock) {
+          lockRef.current = getLock()
+          lockRef.current.set(posValue)
+          prevShouldLock = {pos: posValue}
+        } else if (prevShouldLock.pos !== posValue) {
+          lockRef.current?.set(posValue)
+        } else {
+          // all the same params
+        }
+      } else {
+        lockRef.current!.unlock()
+        prevShouldLock = false
+      }
+    }
+  }, [getLock])
+}
+
 export const useLockFrameStampPosition = (shouldLock: boolean, val: number) => {
   const {getLock} = useContext(context)
   const lockRef = useRef<undefined | ReturnType<typeof getLock>>()
@@ -179,7 +212,6 @@ const pointerPositionInUnitSpace = (
   return prism(() => {
     const rightDims = val(layoutP.rightDims)
     const clippedSpaceToUnitSpace = val(layoutP.clippedSpace.toUnitSpace)
-    const leftPadding = val(layoutP.scaledSpace.leftPadding)
 
     const mousePos = val(mousePositionD)
     if (!mousePos) return [-1, FrameStampPositionType.hidden]