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

InstancedMesh: Incorrect bounding volumes. #18334

Closed
errrken opened this issue Jan 8, 2020 · 14 comments
Closed

InstancedMesh: Incorrect bounding volumes. #18334

errrken opened this issue Jan 8, 2020 · 14 comments
Milestone

Comments

@errrken
Copy link

errrken commented Jan 8, 2020

Geometry bounding box gives a somewhat misleading result when used in an InstancedMesh.

    var count = 10000;

    var mesh = new THREE.InstancedMesh( geometry, material, count );

    var dummy = new THREE.Object3D();

    for ( var i = 0; i < count; i ++ ) {

    	dummy.position.set(
		Math.random() * 20 - 10,
		Math.random() * 20 - 10,
		Math.random() * 20 - 10
	);

	dummy.updateMatrix();

	mesh.setMatrixAt( i, dummy.matrix );

    }

Using new Box3().setFromObject(mesh) only gives you the Box3 for the single geometry, and not for the whole instance.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 8, 2020

With "whole instance" you mean an AABB that encloses all instances of InstancedMesh, right?

@errrken
Copy link
Author

errrken commented Jan 8, 2020

@Mugen87 Yes.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 8, 2020

Similar to the conversation in #8953 about SkinnedMesh, I've been getting multiple issues reported on my viewer (donmccurdy/three-gltf-viewer#147) because the viewport (determined with Box3().setFromObject(...)) does not factor in skinning deformations. Those deformations can be 100x for anything converted from FBX.

I'm not sure that geometry.boundingBox should be any different than it is now, but maybe either Box3 or the Mesh subclasses like InstancedMesh and SkinnedMesh should have some method to provide a better bounding box?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 8, 2020

I think we have a design problem in the library since bounding volumes not only depend on pure geometry data. Classes like InstancedMesh or SkinnedMesh needs to be considered in order to compute proper AABBs and bounding spheres.

What bothers me is the fact that BufferGeometry.boundingBox and BufferGeometry.boundingSphere currently represent more or less useless bounding volumes in certain use cases right now. It would be great to find a solution that avoids this situation. Introducing bounding volumes on object level (and keeping the ones on geometry level) seems a bit of a workaround to me. Although it would be a pragmatic approach.

@gkjohnson
Copy link
Collaborator

Classes like InstancedMesh or SkinnedMesh needs to be considered in order to compute proper AABBs and bounding spheres.

I agree -- there are cases even with just a mesh where being able to specify the bounding volume per-object rather than per-geometry would be beneficial. Materials can change the apparent bounds of geometry, as well, with custom vertex shaders, instancing, and displacement maps (which I also think don't work correctly at the moment).

I still think it's valuable to store the bounding volume on the geometry, though, to share the volume instances. Maybe the responsibility of checking for frustum intersection should shift to the objects? Should the Mesh class define an intersectsFrustum function that can be implemented by derivative classes? I think this would help with the problems mentioned here. Here's a simple example:

Mesh.prototype.intersectsFrustum = function ( frustum ) {

  var geometry = this.geometry;
  var box = this.boundingBox || geometry.boundingBox;
  return frustum.intersectsBox( box );

}

This would allow a bounding box to optionally be set on the Mesh to account for cases where the material changes the visual bounds of the shape. Of course raycasting would also have to be updated to support custom intersections if desired but that's already override-able.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 9, 2020

I'm not sure about the best design right now. This needs some experimenting and studying. I think it's also worth to check how other engines solve this issue.

@webglzhang
Copy link
Contributor

Actually,I do some tries about InstancedMesh's boudingBox and boundingSpere. In my program,I make two bounding that one represents geometry data and other represents the mesh's real volume. I use mesh's real volume to render sort ,which make transparent rendering isn‘t right in some cases. I think maybe the method of sorting return Z should also be encapsulated under the subclass of mesh, such as instancedmesh, to make correct calculation and sorting according to different situations. These are just some of my thoughts,I hope useful.

@donmccurdy donmccurdy mentioned this issue Jan 19, 2020
3 tasks
@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 22, 2020

@Mugen87

I'm not sure about the best design right now. This needs some experimenting and studying. I think it's also worth to check how other engines solve this issue.

I can appreciate that. I do think it's worth noting that the design pattern chosen for raycast affords the type of flexibility we're talking about here, though. Because it's defined on Object3D rather than Ray the object has control over how to perform the raycasting and handle dependencies on material, shader, and render primitive. We'd be running into a similar situation if instead of Object3D.raycast we used Ray.intersectObject, I think. The Frustum.intersectsObject API feels a little inconsistent at the moment.

Edit: Box3.setFromObject likely suffers from similar issues.

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2020

As a (temporary?) workaround I changed frustumCulled to false by default. #18464

@gkjohnson
Copy link
Collaborator

As a (temporary?) workaround I changed frustumCulled to false by default. #18464

Should the same temporary fix be done for SkinnedMesh, as well? It suffers from the same problem.

Also I do hope this is just temporary and that we can come up with a better solution for frustum culling and raycasting in these cases.

@mrdoob
Copy link
Owner

mrdoob commented Jan 28, 2020

SkinnedMesh case is a less visible glitch.
But yes, we should solve this at some point.

@mrdoob mrdoob added this to the r114 milestone Jan 28, 2020
@mrdoob mrdoob modified the milestones: r114, r115 Feb 29, 2020
@mrdoob mrdoob modified the milestones: r115, r116 Mar 25, 2020
@mrdoob mrdoob modified the milestones: r116, r117 Apr 30, 2020
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob modified the milestones: r118, r119 Jun 24, 2020
@mrdoob mrdoob modified the milestones: r119, r120 Jul 29, 2020
@mrdoob mrdoob modified the milestones: r120, r121 Aug 25, 2020
@mrdoob mrdoob added this to the r141 milestone Apr 30, 2022
@Mugen87 Mugen87 changed the title InstancedMesh and computeBoundingBox InstancedMesh/SkinnedMesh and computeBoundingBox(). May 21, 2022
@mrdoob mrdoob modified the milestones: r141, r142 May 26, 2022
@mrdoob mrdoob modified the milestones: r142, r143 Jun 29, 2022
@mrdoob mrdoob modified the milestones: r143, r144 Jul 28, 2022
@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@mrdoob mrdoob modified the milestones: r147, r148 Nov 30, 2022
@mrdoob mrdoob modified the milestones: r148, r149 Dec 22, 2022
@mrdoob mrdoob modified the milestones: r149, r150 Jan 26, 2023
@mrdoob mrdoob modified the milestones: r150, r151 Feb 23, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 3, 2023

Closed via #25591.

@Mugen87 Mugen87 closed this as completed Mar 3, 2023
@Mugen87 Mugen87 changed the title InstancedMesh/SkinnedMesh and computeBoundingBox(). InstancedMesh: Incorrect bounding volumes. Mar 3, 2023
@jcowles
Copy link

jcowles commented Mar 6, 2023

This is still a problem for skinned mesh, correct?

@donmccurdy
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants