spite / THREE.MeshLine

Mesh replacement for THREE.Line
MIT License
2.13k stars 379 forks source link

Delay when using declarative widthCallback #116

Closed withintheruins14 closed 3 years ago

withintheruins14 commented 3 years ago

I am seeing a delay when using the widthCallback. My code is simple enough:

<mesh
  ref={mesh}
  rotation={[0, Math.PI, 0]}
>
  <meshLine
    attach="geometry"
    points={points}
    widthCallback={pointWidth => pointWidth * 5 * Math.random()}
  />
  <meshLineMaterial
    attach="material"
    lineWidth={width}
    color={color}
  />
</mesh>

I see this:

Screen Shot 2020-10-22 at 6 24 30 PM

and then after waiting a long time (1-3 minutes), I get the desired:

Screen Shot 2020-10-22 at 6 28 26 PM

Any guess? The function component doesn't appear to be stale - it's on a timer and rendering twice a second

withintheruins14 commented 3 years ago

I tested the widthCallback using the imperative api via setPoints in canvas-sketch and widthCallback fired right away - it worked. I created a new example with create-react-app and used the declarative api and observed that the widthCallback was not called. Is this a bug? I was expecting the widthCallback to fire immediately.

withintheruins14 commented 3 years ago

First call of MeshLine's process method at 126100 ms:

Screen Shot 2020-10-29 at 11 34 10 AM

bottom up call stack:

Screen Shot 2020-10-29 at 11 36 12 AM
ryanking1809 commented 3 years ago

Hmm looks like it doesn't redraw with widthCallback. If you set the widthCallback before you set the points it should work. I imagine the delay is a result of waiting for the component to rerender in which setPoints is called again. Should be able to do a PR to fix.

<meshLine
    attach="geometry"
    widthCallback={pointWidth => pointWidth * 5 * Math.random()}
    points={points}
  />
withintheruins14 commented 3 years ago

You rock @ryanking1809! Thanks - as always. This worked right away, however I'm not sure the fix is as simple as rendering the component again - I'd had it rendering many times before finally kicking in. Would love to help on the PR, let's see if I can figure it out

ryanking1809 commented 3 years ago

I should be able to find the time to test it later in the weekend but if you have the time basically this.widthCallback needs to be replaced with a getter for declarative architectures so we can do a redraw. I imagine if you added the following to Object.defineProperties on line 32 it should work.

      widthFunction: {
        enumerable: true,
        get: function() {
          return this.widthCallback;
        },
        set: function(value) {
          this.widthCallback= value;
          // this should redraw the line with the new width function
          // it might be necessary to check if points exist before running this
          // as it can be executed before points are defined
          this.process();
        },
      },

Then we use the new getter

<meshLine
    attach="geometry"
    points={points}
    widthFunction={pointWidth => pointWidth * 5 * Math.random()}
  />

You could alternatively keep the widthCallback name and rename all the existing reference to _widthCallback