fix(3dtiles): apply matrixWorld to boundingVolume.box #788
Conversation
peppsac
force-pushed
the
fix_3dtiles_boundingvolume
branch
from
e8805b9 to
ab5d42e
Compare
|
Updated with some unittest. Note that something is broken with the distance computation when using region but I couldn't fix it. |
an issue should be opened to specify the problem |
I've added 2 unit tests that need to be fixed. That's visible everytime a developer runs the tests so it's good enough:
|
Done: #796 |
|
@gchoqueux anything else bothering you on this PR or can we move forward and merge it? |
src/Process/3dTilesProcessing.js
Outdated
| function computeNodeSSE(camera, node) { | ||
| const boundingVolumeBox = new THREE.Box3(); | ||
| const boundingVolumeSphere = new THREE.Sphere(); | ||
| export function computeNodeSSE(camera, node) { | ||
| node.distance = 0; | ||
| if (node.boundingVolume.region) { | ||
| worldPosition.setFromMatrixPosition(node.boundingVolume.region.matrixWorld); |
There was a problem hiding this comment.
the test works with
if (node.boundingVolume.region) {
boundingVolumeBox.copy(node.boundingVolume.region.box3D);
boundingVolumeBox.applyMatrix4(node.boundingVolume.region.matrixWorld);
node.distance = boundingVolumeBox.distanceToPoint(camera.camera3D.position);
}There was a problem hiding this comment.
Fixed and pushed. Thanks
src/Process/3dTilesProcessing.js
Outdated
| boundingVolumeSphere.copy(node.boundingVolume.sphere); | ||
| boundingVolumeSphere.applyMatrix4(node.matrixWorld); | ||
| node.distance = Math.max(0.0, | ||
| boundingVolumeSphere.distanceToPoint(camera.camera3D.position)); |
There was a problem hiding this comment.
Relying on camera.camera3D.position assumes that the camera's matrix is equal to its matrixWorld (as when the camera is a direct child of the scene). I would rather not have this assumption but if we have, let's write it somewhere.
To be conservative, but correct, I think you should apply node.matrixWorld * camera.matrixWorldInverse instead and measure the distance to the origin.
There was a problem hiding this comment.
Agreed. As there are other places that make the same assumption I suggest to not fix this in this PR.
I can add a // TODO to not forget about this?
There was a problem hiding this comment.
it would seem wise:
- to create an issue, to specify the problem and indicate where are the other places that make the same assumption (because apparently, this can be a source of bugs)
- apply the solution here and propose it also in the same issue
There was a problem hiding this comment.
apply the solution here and propose it also in the same issue
It's not free performance-wise so I won't sneak this small change in an unrelated PR.
I've opened the following issue to not forget about it: #800
There was a problem hiding this comment.
I can add a // TODO to not forget about this?
just add the TODO
peppsac
force-pushed
the
fix_3dtiles_boundingvolume
branch
from
60bfc5a to
e6986d8
Compare
|
Squashed and ready to merge. |
src/Process/3dTilesProcessing.js
Outdated
| boundingVolumeSphere.copy(node.boundingVolume.sphere); | ||
| boundingVolumeSphere.applyMatrix4(node.matrixWorld); | ||
| node.distance = Math.max(0.0, | ||
| boundingVolumeSphere.distanceToPoint(camera.camera3D.position)); |
There was a problem hiding this comment.
I can add a // TODO to not forget about this?
just add the TODO
Before this commit we only used the translation part of the transform. We're now using the full 4x4 matrix to match the wording of the 3dtiles specification: "The transform property applies to [...] tile.boundingVolume , except when tile.boundingVolume.region is defined"
peppsac
force-pushed
the
fix_3dtiles_boundingvolume
branch
from
e6986d8 to
e845505
Compare
Before this commit we only used the translation part of the transform.
We're now using the full 4x4 matrix to match the wording of the 3dtiles
specification: