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

WebGLObjects: Optimize InstancedMesh updates. #26300

Merged
merged 2 commits into from
Jun 20, 2023
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jun 20, 2023

Related issue: #26293

Description

I've noticed updating InstancedMesh can be improved by checking the frame value similar to how geometries are updated.

The PR also refactors the skeleton update by removing Skeleton.frame. The frame is now stored in the internal update map of WebGLObjects.

@Mugen87 Mugen87 marked this pull request as ready for review June 20, 2023 08:48
@Mugen87 Mugen87 added this to the r154 milestone Jun 20, 2023
@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
642.8 kB (159.4 kB) 642.8 kB (159.4 kB) -16 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
436.1 kB (105.6 kB) 436.1 kB (105.6 kB) -2 B

@Mugen87 Mugen87 merged commit 7b419c9 into mrdoob:dev Jun 20, 2023
@Amatewasu
Copy link

Amatewasu commented Feb 8, 2024

Hi @Mugen87, thanks for the improvement.

With @TeoAvignon, we are trying to upgrade the three.js version of our project and it appears that the v154 breaks our mouseenter and other mouse related events on InstancedMesh. We think it is related to this change.

We struggle to create a minimal reproductive case though.

The following piece of code triggers well the onPointerOver events with three v153 but not in v154. (sorry it has been written with react-three-fiber)

<instancedMesh
          ref={
            stocklinesMeshRef as Ref<
              THREE.InstancedMesh<THREE.BufferGeometry, THREE.RawShaderMaterial | THREE.RawShaderMaterial[]>
            >
          }
          args={[undefined, undefined, nMaxSlot]}
          onPointerOver={(e) => {
            const instanceId = e.instanceId;
            const stocklineId = instanceId !== undefined ? instanceIdToStocklineId.current.get(instanceId) : undefined;
            setHovered(stocklineId !== undefined ? { id: stocklineId.id, position: stocklineId.position } : null);
          }}
          onClick={handleClick}
          onPointerLeave={() => setHovered(null)}
          frustumCulled={false}
        >
          <planeGeometry attach="geometry" args={[1, 1, 1]}>
            <instancedBufferAttribute attach="attributes-uvOffset" args={[uvOffset, 2]} />
            <instancedBufferAttribute attach="attributes-opacity" args={[opacity, 1]} />
          </planeGeometry>
          <rawShaderMaterial
            opacity={stocklinesOpacity}
            transparent
            depthWrite={false}
            onBeforeCompile={(shader) => {
              shader.uniforms.map = { value: textureAtlas };
              shader.uniforms.atlasRows = { value: atlasRows };
              shader.uniforms.atlasColumns = { value: atlasColumns };

              shader.vertexShader = vertexShader;
              shader.fragmentShader = fragmentShader;
            }}
          />
        </instancedMesh>

Would you have an idea on what could lead to this change?
(we tested with simpler materials and simpler geometries and in our project the behavior stay the same)

We will try to update the issue with a codesandbox as soon as we have something to reproduce.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 8, 2024

It's best if you ask for help at the forum first. If it turns out to be a bug in the engine, open a new issue.

I doubt that this particular PR introduces the regression. Maybe the bounding volume addition in r151 has an unexpected side effect in your app. Whenever you transform instances of InstancedMesh, make sure to update the bounding volumes via computeBoundingBox() and computeBoundingSphere(). Otherwise they don't reflect the real bounds of the instanced mesh which could lead to unexpected culling or buggy raycasting.

@TeoAvignon
Copy link

That's absolutely this ! Computing the BBox and BSphere gives back the previous behavior we had.
Thank you very much for your quick reply @Mugen87 !

Have a good day !

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.

3 participants