Race condition in collision detection

i-am-the-slime commented 2 years ago

Hey @clauderic 👋🏻,

thank you for this library! I've encountered a problem that took me a while to figure out. Every second one of my droppables would simply not activate. After a while I found out that they actually do activate just only on the right:

https://user-images.githubusercontent.com/1588055/158953067-d4bef4a2-a394-4799-93d1-7f4cecef1ece.mp4

Today I finally found out what it was: In a CollisionDetection the rect refs seem to be snapshots from somewhere during my animation. Somehow half of them were still in the small state.

After changing to reading the rects from the node instead, the collision actually works.

Now I wonder if this is on purpose due to performance reasons or an actual oversight.

Code that won't help anyone

```purescript marksPointerWithin ∷ CollisionDetection marksPointerWithin = mkEffectFn1 \({ droppableContainers, pointerCoordinates } ∷ CollisionArgs) → Nullable.toMaybe pointerCoordinates # foldMap \mouseCoords → do let inRect = pointIsInRect mouseCoords withRect ← droppableContainers # filterA \{ node } → rectFromNodeRef node <#> any inRect pure $ withRect <#> toOutput where pointIsInRect { x, y } { top, bottom, left, right } = top < y && y < bottom && left < x && x < right toOutput droppableContainer = { id: droppableContainer.id , data: { droppableContainer, value: 1.0 } } rectFromNodeRef nodeRef = runMaybeT do node ← readRefMaybe nodeRef # MaybeT htmlElement ← HTMLElement.fromNode node # pure # MaybeT getBoundingClientRect htmlElement # liftEffect ```

clauderic commented 2 years ago

Can you try upgrading to the versions of @dnd-kit tagged with next on npm? I think this is fixed by this PR I merged a few days ago https://github.com/clauderic/dnd-kit/pull/656 (will be part of the next release https://github.com/clauderic/dnd-kit/pull/657)

i-am-the-slime commented 2 years ago

Wow, very nice @clauderic! Unfortunately upgrading to next gives me this error as soon as I attempt to start dragging:

This is independent of the collision detection, however.

clauderic commented 2 years ago

Whoops. Can you try @dnd-kit/core@5.1.0-next-20222200858?

Looking at your example, you may also need to manually schedule measurement of the droppable rects when the sidebar animation finishes.

Something like:

import {useDndContext} from '@dnd-kit/core';

function MyApp() {
  const {measureDroppableContainers} = useDndContext();
  const handleSidebarAnimationEnd = () => {
    measureDroppableContainers();
  }
}

i-am-the-slime commented 2 years ago

Thanks Claudéric 💪🏻!

So I get no crash when I update core to 5.1.0-next-20222200858 but the droppable still is too small.

I haven't tried measuring droppables in the way that you suggested yet because I have the following concern: Wouldn't that mean that during the animation the containers are wrong and only correct when the animation is finished? I guess I remeasure in a debounced way to alleviate this and stop remeasuring on animation end 🤔

clauderic / dnd-kit

Race condition in collision detection #663