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

LocalState for custom renderer node instances instead of userData #516

Closed
4 tasks done
alvarosabu opened this issue Jan 21, 2024 · 0 comments · Fixed by #522
Closed
4 tasks done

LocalState for custom renderer node instances instead of userData #516

alvarosabu opened this issue Jan 21, 2024 · 0 comments · Fixed by #522
Labels

Comments

@alvarosabu
Copy link
Member

alvarosabu commented Jan 21, 2024

Description

As the author and maintainer of TresJS, I would like to simplify and clean the custom renderer nodeOps to make the code more readable and easier to maintain.

At, we are using overpopulation the property userData for the scene and objects on the scene graph as a workaround for accessing state or adding extra properties and methods that we need for TresJS nodeOps like this:

scene.userData.tres__registerAtPointerEventHandler = registerObject

userData is a property that the end-user can also use, we need to add the .tres__ prefix to avoid naming collisions, adding an extra cognitive load to the code.

Code ends up being kind of difficult to read:

remove(node) {
    if (!node) return
    // remove is only called on the node being removed and not on child nodes.

    if (node.isObject3D) {
      const object3D = node as unknown as Object3D

      const disposeMaterialsAndGeometries = (object3D: Object3D) => {
        const tresObject3D = object3D as TresObject3D

        if (!object3D.userData.tres__materialViaProp) {
          tresObject3D.material?.dispose()
          tresObject3D.material = undefined
        }

        if (!object3D.userData.tres__geometryViaProp) {
          tresObject3D.geometry?.dispose()
          tresObject3D.geometry = undefined
        }
      }

      const deregisterAtPointerEventHandler = scene?.userData.tres__deregisterAtPointerEventHandler
      const deregisterBlockingObjectAtPointerEventHandler
        = scene?.userData.tres__deregisterBlockingObjectAtPointerEventHandler

      const deregisterAtPointerEventHandlerIfRequired = (object: TresObject) => {

        if (!deregisterBlockingObjectAtPointerEventHandler)
          throw 'could not find tres__deregisterBlockingObjectAtPointerEventHandler on scene\'s userData'

        scene?.userData.tres__deregisterBlockingObjectAtPointerEventHandler?.(object as Object3D)

        if (!deregisterAtPointerEventHandler)
          throw 'could not find tres__deregisterAtPointerEventHandler on scene\'s userData'

        if (
          object && supportedPointerEvents.some(eventName => object[eventName])
        )
          deregisterAtPointerEventHandler?.(object as Object3D)
      }

      const deregisterCameraIfRequired = (object: Object3D) => {
        const deregisterCamera = scene?.userData.tres__deregisterCamera

        if (!deregisterCamera)
          throw 'could not find tres__deregisterCamera on scene\'s userData'

        if ((object as Camera).isCamera)
          deregisterCamera?.(object as Camera)
      }

      node.removeFromParent?.()
      object3D.traverse((child: Object3D) => {
        disposeMaterialsAndGeometries(child)
        deregisterCameraIfRequired(child)
        deregisterAtPointerEventHandlerIfRequired?.(child as TresObject)
      })

      disposeMaterialsAndGeometries(object3D)
      deregisterCameraIfRequired(object3D)
      deregisterAtPointerEventHandlerIfRequired?.(object3D as TresObject)
    }

    node.dispose?.()
  },

Suggested solution

Add a local state for each instance called __tres which contains:

export interface LocalState = {
  type: string
  // objects and parent are used when children are added with `attach` instead of being added to the Object3D scene graph
  objects: Instance[]
  parent: Instance | null
  primitive?: boolean
  eventCount: number
  handlers: Partial<EventHandlers>
  memoizedProps: { [key: string]: any }
}
export interface TresObject3D extends THREE.Object3D<THREE.Object3DEventMap> {
  geometry?: THREE.BufferGeometry & TresBaseObject
  material?: THREE.Material & TresBaseObject
  __tres: LocalState
}

We could include the context of TresContextProvider on the scene object to handle the register of cameras and event handlers or even inside each LocalState on a potential property root

export type LocalState = {
  type: string
  // objects and parent are used when children are added with `attach` instead of being added to the Object3D scene graph
  root: TresContext,
  objects: Instance[]
  parent: Instance | null
  primitive?: boolean
  eventCount: number
  handlers: Partial<EventHandlers>
  memoizedProps: { [key: string]: any }
}

Alternative

No response

Additional context

No response

Validations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant