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

Introduce intersectsFrustum so that objects can override it #25525

Closed
wants to merge 2 commits into from

Conversation

OndrejSpanel
Copy link
Contributor

@OndrejSpanel OndrejSpanel commented Feb 17, 2023

Description

Introduce a method intersectsFrustum into Object3D so that objects can customize it. The intended use is to implement frustum culling for InstancedMesh, but it could be used for other purposes as well, e.g. some objects might prefer culling against box instead of sphere, or implement some other culling implementations based on its internal knowledge.

The PR is inspired by https://discourse.threejs.org/t/how-to-do-frustum-culling-with-instancedmesh/22633/6, but attempts to be more flexible.

If you are willing to compute the bounding sphere on your own, culling is easy:

class InstancedMeshCulled extends InstancedMesh {
	constructor( geometry, material, count ) {

		super(geometry, material, count);

		this.frustumCulled = true;
	}

	intersectsFrustum( frustum ) {
		frustum.intersectsSphere( this.boundingSphere );
	}
}

I will provide a function to compute the boundingSphere for InstancedMesh later.

@OndrejSpanel
Copy link
Contributor Author

I have added a class InstancedMeshCulled with a function computeBoundingSphere.

The function assumes no scaling, as determining scale would require something like computing matrix determinant, which I do not feel like doing. If you use scaling, you'd better compute the bounding sphere on your own.

Note: I did not test the function, as I am using my own computation of the bounding sphere, I find it easier to compute it from the same source data I use to compute the instancing matrices, but I think having it could be useful for users which want to have something ready to use.

@OndrejSpanel OndrejSpanel marked this pull request as ready for review February 17, 2023 15:51
@OndrejSpanel
Copy link
Contributor Author

@Mugen87 @WestLangley Do you have any feedback on this? Merging the second commit with InstancedMeshCulled is optional, but the first commit introducing frustum culling customization is very important for me.

I have a scene with a lot of instancing and CSM used for shadows. As shadow frustums are much smaller than the whole scene, having an option to cull instanced objects is very important and reduces rendering of instanced objects to about 1/10, causing very significant performance gain.

@WestLangley
Copy link
Collaborator

The first commit seams reasonable to me. Perhaps that is the API we should have had in the first place.

//

This PR raises some current nomenclature issues...

Frustum.intersectsObject( object ), assumes object has a .geometry. So it is not any object.

But then there is Frustum.intersectsSprite( sprite ). A Sprite is an object.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2023

TBH, I favor a different approach which has already been implemented in #21507. The idea is to introduce bounding volumes for certain types of Object3Ds and prioritize them for certain operations.

To be clear, the real issue are incorrect bounding volumes. Configurable/custom frustum intersection tests do not really address it.

@OndrejSpanel
Copy link
Contributor Author

That one would work for me well. The benefit of this one is its simplicity - that PR is open for two years and not merged yet.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2023

We really need to fix the root cause of this issue and I believe #21507 goes in the right direction. #21507 just lacked of attention and support. The code itself is not complex to review. It's more about making design choice.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 2, 2023

#25591 is merged so this PR can be closed now.

@Mugen87 Mugen87 closed this Mar 2, 2023
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