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

BatchedMesh: Add pure annotations, fix camera position for sorting #27236

Merged
merged 2 commits into from
Nov 25, 2023

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #22376

Description

  • Add /*@__PURE__*/ annotations for global cache variables
  • Transform the camera position point into the local frame for sorting

@gkjohnson gkjohnson added this to the r159 milestone Nov 23, 2023
Comment on lines +903 to 906
// prepare the frustum in the local frame
if ( perObjectFrustumCulled ) {

_projScreenMatrix
Copy link
Collaborator Author

@gkjohnson gkjohnson Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurred to me that I don't fully understand how this frustum generation logic works - it was copied from WebGLRenderer and then ".muliply( this.matrixWorld )" was added - why is it that we use camera.matrixWorldInverse here instead of camera.matrixWorld to get the frustum in world frame?

_projScreenMatrix
   .multiplyMatrices( camera.projectionMatrix, camera.matrixWorldInverse )
   .multiply( this.matrixWorld );

Copy link
Collaborator

@Mugen87 Mugen87 Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In WebGLRenderer, _projScreenMatrix transforms from world space directly to clip space and maybe it would be less confusing to stick to this logic in BatchedMesh, too?

Meaning .multiply( this.matrixWorld ); should be removed and _frustum needs to check bounding spheres in world space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's significantly more matrix multiplications for no gain, though, since it means each bounding volume needs to have the world matrix multiplied in.

I just want to understand how the Frustum.setFromProjectionMatrix function works. It must take an inverted matrix? It must be true since camera.projectionMatrix is what compresses everything into the clip space box. Even if we just talk about what WebGLRenderer is doing - it wasn't clear why this results in a frustum that can check for intersections in world space since camera.matrixWorldInverse should transform into the local space of the camera.

_projScreenMatrix.multiplyMatrices( camera.projectionMatrix, camera.matrixWorldInverse )

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
668.4 kB (165.8 kB) 668.4 kB (165.9 kB) +67 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
449.6 kB (108.9 kB) 449.6 kB (108.9 kB) -56 B

@gkjohnson
Copy link
Collaborator Author

I'm not sure when the next release is but this should be merged before 159, I think, since it fixes a bug.

@Mugen87 Mugen87 merged commit 684de25 into mrdoob:dev Nov 25, 2023
12 checks passed
@gkjohnson gkjohnson deleted the small-batch-fixes branch November 25, 2023 14:43
@mrdoob
Copy link
Owner

mrdoob commented Nov 28, 2023

@gkjohnson

I'm not sure when the next release is but this should be merged before 159, I think, since it fixes a bug.

You can see when the next milestone is due here:

https://github.com/mrdoob/three.js/milestones

AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
…rdoob#27236)

* Add pure annotations, fix camera position for sorting

* Update annotations
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