Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v9] fix(events): bubble up primitives #3032

Open
wants to merge 3 commits into
base: v9
Choose a base branch
from

Conversation

krispya
Copy link
Member

@krispya krispya commented Oct 2, 2023

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.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 2, 2023

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 9a99a14:

Sandbox Source
example Configuration

@krispya krispya changed the title Fix primitive events [v9] Fix primitive events Oct 2, 2023
@CodyJasonBennett
Copy link
Member

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

@krispya
Copy link
Member Author

krispya commented Oct 2, 2023

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 // <-- 

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Oct 3, 2023

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/facebook/react/issues/20271
// This will make sure events and attach are only handled once when trees are complete
function handleContainerEffects(parent: Instance, child: Instance, beforeChild?: Instance, replace: boolean = false) {
// Bail if tree isn't mounted or parent is not a container.
// This ensures that the tree is finalized and React won't discard results to Suspense
const state = child.root.getState()
if (!parent.parent && parent.object !== state.scene) return
// Create & link object on first run
if (!child.object) {
// Get target from catalogue
const name = `${child.type[0].toUpperCase()}${child.type.slice(1)}`
const target = catalogue[name]
// Create object
child.object = child.props.object ?? new target(...(child.props.args ?? []))
child.object.__r3f = child
// Set initial props
applyProps(child.object, child.props)
}
// Append instance
if (child.props.attach) {

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

@krispya
Copy link
Member Author

krispya commented Oct 3, 2023

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.

@CodyJasonBennett CodyJasonBennett changed the title [v9] Fix primitive events [v9] fix(events): bubble up primitives Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants