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

Suggestion for imperative update api #39

Closed
ryanking1809 opened this issue Mar 21, 2019 · 15 comments
Closed

Suggestion for imperative update api #39

ryanking1809 opened this issue Mar 21, 2019 · 15 comments

Comments

@ryanking1809
Copy link
Contributor

I'm finding this to be a very common pattern I'm using to avoid having react-three-fiber recreate new geometry for objects I need to regularly update. I'm wondering if there's a nicer api for this.

Let's say I'm creating a dynamic cube. I regularly do the following:

  • I will create store with its dimensions
  • create geometry and mesh instance, cached in the store
  • have a memoised / reactive function that updates the geometry / mesh based off the cube dimensions
  • apply the updated mesh to the primitive tag
class CubeStore {
    @observable x = 0
    @observable y = 0
    @observable z = 0

    constructor(x,y,z) {
        this.x = x
        this.y = y
        this.z = z
        this._geometry = getCubeGeometry(x,y,z)
    }

    @computed get geometry() {
         const geometry = this._geometry
         geometry.addAttribute('position', getCubeVertices(this.x, this.y, this.z))
         geometry.attributes.position.needsUpdate = true
         geometry.computeBoundingSphere()
         return geometry
     }
}

const geometry = ({cube}) => {
    return <primitive object={cube.geometry} />
}

I'm ending up with a lot of code in my stores which I feel should be in the component. And I know you can't avoid creating new instances because of the nature of three.js. But what I'm thinking is maybe if the component could have an update function, which react-three-fiber could use instead of reinstantiating the object on render.

Something like this would keep that logic inside the component and feels a little nicer:

class CubeStore {
    @observable x = 0
    @observable y = 0
    @observable z = 0

    constructor(x,y,z) {
        this.x = x
        this.y = y
        this.z = z
    }
}

const geometry = ({cube}) => {
    ref = useRef()
    // use this on update instead of creating a new geometry instance
    useImperativeUpdate( 
        () => {
             const geometry = ref.current
             geometry.addAttribute('position', getCubeVertices(cube.x, cube.y, cube.z))
             geometry.attributes.position.needsUpdate = true
             geometry.computeBoundingSphere()
        }, 
        [cube.x, cube.y, cube.z] // execute only if these change
    )
    return <bufferGeometry ref={ref} ... />
}

What's your thoughts on something like that?

@drcmda
Copy link
Member

drcmda commented Mar 21, 2019

I think the problem is always the same, we have data, and if we don't manage it, we end up with a salad of three js objects. So a possible solution would be to put as much into the graph as we can, so that only the raw data remains as state, while the view reflects, without re-creating objects.

In this case for instance, something like this could work:

function Test() {
  const vertices = useMemo(() => new Float32Array([
    -1.0, -1.0, 1.0,
    1.0, -1.0, 1.0,
    1.0, 1.0, 1.0,
    1.0, 1.0, 1.0,
    -1.0, 1.0, 1.0,
    -1.0, -1.0, 1.0,
  ]))
  const update = useCallback(({ parent }) => {
    parent.attributes.position.needsUpdate = true
    parent.computeBoundingSphere()
  }), [])
  return (
    <mesh>
      <bufferGeometry attach="geometry">
        <bufferAttribute
          attachObject={['attributes', 'position']}
          array={vertices} 
          itemSize={3}
          onUpdate={update} />
      </bufferGeometry>
      <meshBasicMaterial attach="material" />
    </mesh>
  )
}

It doesn't work right now, because THREE.BufferGeometry.attributes is an object. But i am thinking anyway to retreat from using "name". If we have:

  • attach={target} // just overwrites, whatever the target is
  • attachArray={target} // appends on mount, removes on unmount
  • attachObject={[target, name]} // adds on mount, deletes on unmount

Then it could know what to do, and we could drive any three object, no matter what it is.

Another thing that needs solving is the onUpdate callback. Not sure if the parent is available like this right now. But in any way it should be.

@drcmda
Copy link
Member

drcmda commented Mar 21, 2019

useImperativeUpdate is a nice idea btw, it could still be useful.

I don't see how yours would work, you don't pass a ref. It could return one maybe?

const ref = useUpdate( 
  geometry => {
    geometry.addAttribute('position', getCubeVertices(cube.x, cube.y, cube.z))
    geometry.attributes.position.needsUpdate = true
    geometry.computeBoundingSphere()
  }, 
  [cube.x, cube.y, cube.z] // execute only if these change
)
return <bufferGeometry ref={ref} />

Or, accept one as the 3rd param optionally, so that you can re-use refs for multiple purposes

const ref = useRef()
useUpdate(() => {}, [...], ref)
return <bufferGeometry ref={ref} />

@ryanking1809
Copy link
Contributor Author

Oh, I was thinking a more dumb run the update function instead of recreating the geometry - but this looks like it could work very nicely!

Perhaps you could just use refs in the update callback?

@ryanking1809
Copy link
Contributor Author

ryanking1809 commented Mar 21, 2019

I missed your other comment! I think I prefer the 3rd param ref option. It feels more similar to how I would use useMemo. You apply the ref to the component and use it in the update function.

@drcmda
Copy link
Member

drcmda commented Mar 21, 2019

Alright, i'll get to it then, we need this as well at work. I don't think i can finish this until next week though, maybe monday.

@ryanking1809
Copy link
Contributor Author

Oh, no rush at all! Thanks for all your effort in building this!

@drcmda drcmda reopened this Mar 21, 2019
@drcmda
Copy link
Member

drcmda commented Mar 28, 2019

@ryanking1809 the updates are in. useUpdate and attach/attachArray/attachObject https://github.com/drcmda/react-three-fiber/tree/2.0#objects-and-properties

Small warning, "name" has been deprecated on from the latest beta.

Makes it possible to handle buffer-attributes reactively

@ryanking1809
Copy link
Contributor Author

Great, I'll have a play! 2.0.0-beta.4?

@drcmda
Copy link
Member

drcmda commented Mar 28, 2019

Yep.

@ryanking1809
Copy link
Contributor Author

I'm just about to jump on a plane but might be able to make a sandbox later.
I have the following:

const BarGeometry = ({ bar }) => {
    const ref = useRef()
	console.log("TCL: BarGeometry -> bar", bar, ref)
    const vertices = verticeMap(bar.start, bar.end, bar.yIndex, 0)
    return (
        <bufferGeometry ref={ref}>
            <bufferAttribute attachObject={['attributes', 'position']} array={vertices} itemSize={3} />
        </bufferGeometry>
    )
}

export const BarMesh = ({bar}) => {
    const ref = useRef()
    console.log("TCL: BarMesh -> bar", bar, ref)
    return (
        <mesh ref={ref}>
            <BarGeometry attach="geometry" bar={bar} />
            <meshNormalMaterial attach="material" />
        </mesh>
    )
}

It doesn't appear to render BarMesh. When you look at the ref for BarGeometry the attribute look correct but boundingBox and boundingSphere are null. Is there a way to run the following functions?

geometry.attributes.position.needsUpdate = true;
geometry.computeBoundingSphere();

@drcmda
Copy link
Member

drcmda commented Mar 29, 2019

onUpdate={self => ...} . it gives you the instance, and it should have the parent as well.

@drcmda
Copy link
Member

drcmda commented Mar 29, 2019

Here's an example: https://codesandbox.io/s/v8k3z74kky

Had to make a change for this to work, it's in the latest beta. Also looks like array and itemSize wasn't enough, bufferAttribute needed the "count" property to function correctly.

@drcmda drcmda closed this as completed Mar 29, 2019
@ryanking1809
Copy link
Contributor Author

Great, that does the trick! Thanks!

Any reason why your example doesn't need to check the parent exists and mine does? I have to do self.parent && self.parent.computeBoundingSphere() Not a huge issue but it's a bit odd considering my code almost exactly the same.

export const BarMesh = ({ bar }) => {
    const vertices = useMemo(
        () => verticeMap(bar.start, bar.end, bar.yIndex, bar.selected ? 100 : 0),
        [bar.start, bar.end, bar.yIndex, bar.selected]
    )
    const update = useCallback(self => {
        self.needsUpdate = true
        self.parent && self.parent.computeBoundingSphere()
    }, [])
    return (
        <mesh>
            <bufferGeometry attach="geometry">
                <bufferAttribute
                    attachObject={['attributes', 'position']}
                    array={vertices}
                    count={vertices.length / 3}
                    itemSize={3}
                    onUpdate={update}
                />
            </bufferGeometry>
            <meshNormalMaterial attach="material" />
        </mesh>
    )
}

@ryanking1809
Copy link
Contributor Author

Oops ignore that - forgot to restart the server after updating to the latest beta!

@drcmda
Copy link
Member

drcmda commented Mar 30, 2019

There was indeed a bug, onUpdate was called too early.

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

No branches or pull requests

2 participants