react-three-fiber icon indicating copy to clipboard operation
react-three-fiber copied to clipboard

[v9] fix(events): bubble up primitives

Open krispya opened this issue 10 months ago • 5 comments

Events were completely broken for primitive elements. The main reason is that with a primitive you can add a tree fragment to the scene but only the top most element gets processed for an ___r3f property. Since all children are missing this, when they get hit by a raycaster they then fail the getRootState call. Similarly, primitive elements were failing to get added to ineternal.interactions object as a primitive would get processed with applyProps before it had a parent. I don't know why this is. This PR does the following:

  • Filters out events from being written to objects unnecessarily.
  • Removes the parent check from the ineternal.interactions function. I'm not sure what this check was doing in the first place. If we need it, we'll have to figure out why applyProps is run before it has a parent.
  • When a hit is processed for handlers, recursively look up the tree until we get a state from getRootState.

An alternative would be to process all children of a primitive fragment to give them R3F metadata.

krispya avatar Oct 02 '23 06:10 krispya

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9a99a14a25e11e541d9c070c7550b6bb47586130:

Sandbox Source
example Configuration

codesandbox-ci[bot] avatar Oct 02 '23 06:10 codesandbox-ci[bot]

Does this work on v8? What's the cause of this discrepancy if we haven't changed events, but only internal reconciler code?

CodyJasonBennett avatar Oct 02 '23 10:10 CodyJasonBennett

Good question. I just checked. primitive still has applyProps called before it has a parent but something about the logic is different and the object is still added to interactions. applyProps had been completely rewritten so I'm not sure where the difference lies.

To real difference is here though. In v8, if there is no R3F prop on the object it just uses the default root store. This looks better than traversing. But I still wonder if it's still better to process all children of a primitive anyway.

function handleIntersects(
    intersections: Intersection[],
    event: DomEvent,
    delta: number,
    callback: (event: ThreeEvent<DomEvent>) => void,
  ) {
    const rootState = store.getState() // <--

    if (intersections.length) {
      const localState = { stopped: false }
      for (const hit of intersections) {
        const state = getRootState(hit.object) || rootState // <-- 

krispya avatar Oct 02 '23 16:10 krispya

We can only assume that imperative children are fully dynamic, so we can't rely on any of their properties to persist with time. I believe this issue was lost in a much earlier refactor when we braced against suspense -- applyProps needs to be set for all instances, but it's not set for primitives here which skip this first scope.

https://github.com/pmndrs/react-three-fiber/blob/c5694ee507874e0c2aacb19336072fbbdc3af8fd/packages/fiber/src/core/reconciler.tsx#L115-L138

This doesn't affect cases where events propagate up from imperative children to the primitive element though.

CodyJasonBennett avatar Oct 03 '23 01:10 CodyJasonBennett

That's a good catch. And yeah that makes sense about the children being dynamic. I think traversing up is the best solution so that portaling is respected.

krispya avatar Oct 03 '23 17:10 krispya