search

xeokit / xeokit-sdk

Open source JavaScript SDK for viewing high-detail, full-precision 3D BIM and AEC models in the Web browser.
https://xeokit.io
Other
715 stars 286 forks source link

[With solution proposal]: Wrong initialization of instanced AABB's #1102

Closed tmarti closed 1 year ago

tmarti commented 1 year ago

The problem

When loading a XKT file using instancing, the initalization of AABB's for instanced objects consists of getting the reference geometry AABB, and applying the mesh matrix to the AABB.

This is an optimization over transforming the reference geometry positions, and then getting the AABB of the transformed positions.

When the instancing matrix is non-TRS-separable (in tests, when it has a skew component), this optimization breaks and the AABB's are wrongly estimated. The effect this has is that when the model is loaded (when using non-TRS-separable instancing matrices), sometimes the AABB's are so off of the reality that the initial viewer.cameraFlight.flyTo() call will zoom out so strongly that the model will vanish.

The fix

The fix consists of transforming the positions of the reference geometry with the instance matrix and then getting its AABB, instead of directly/"only" transforming the AABB of the reference geometry.

The fix: data-textures

The fix there is easy because when loading instanced portions the code has access still to the instanced geometry positions.

I can do a PR here when you finish the refactors you're carrying currently @xeolabs. Will wait to not complicate the merge on what you're doing.

The fix: TrianglesInstancingLayer

The fix here is a bit harder: today, after the geometry positions have been passed to the TrianglesInstancingLayer, they are loaded into WebGL buffers and then discarded from JS memory. And, when an instance is ingested later by the TrianglesInstancingLayer, it does not have access anymore to the original geometry positions (they are not kept, just used initially when loading the geometry and then discarded).

The fix here would involve temporarily keeping the positons of the instanced geometry until the TrianglesInstancingLayer is finalized. Open to talk about this in 1 or 2 weeks if you want @xeolabs!

tmarti commented 1 year ago

Solved by the combination of:

tmarti commented 1 year ago

Solved when merging #1115 and #1116