Skip to content

Conversation

@zeux
Copy link
Contributor

@zeux zeux commented Nov 16, 2019

Instead of relying on three.js to compute bounding box/sphere, we now
compute them from attribute min/max specified in glTF file. These are
mandatory as per glTF spec.

This allows us to save time after loading the object - bounding sphere
is used for culling unconditionally.

Note that since glTF data doesn't directly specify bounding sphere data,
the bounding sphere is slightly larger because it's computed from min/max
values.

Fixes #17937

Instead of relying on three.js to compute bounding box/sphere, we now
compute them from attribute min/max specified in glTF file. These are
mandatory as per glTF spec.

This allows us to save time after loading the object - bounding sphere
is used for culling unconditionally.

Note that since glTF data doesn't directly specify bounding sphere data,
the bounding sphere is slightly larger because it's computed from min/max
values.
@mrdoob mrdoob added this to the r111 milestone Nov 16, 2019
@mrdoob mrdoob merged commit fb405ea into mrdoob:dev Nov 16, 2019
@mrdoob
Copy link
Owner

mrdoob commented Nov 16, 2019

Thanks!

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 7, 2019

It seems this change broke the VRM example:

https://threejs.org/examples/webgl_loader_vrm

The min and max accessor values are undefined und thus a runtime error occurs.

/cc @takahirox

@zeux zeux deleted the gltf-bounds branch December 7, 2019 14:33
@zeux
Copy link
Contributor Author

zeux commented Dec 7, 2019

@Mugen87 Thanks for the heads up! For what it's worth, I intentionally did not put any checks for min/max accessor presence because glTF specification mandates their presence on position data for both base mesh and morph targets.

Indeed, if I rename the file in question to .glb extension and run it through glTF validator, I get 563 errors, around half are in the form of

/meshes/2/primitives/0/targets/2/POSITION: accessor.min and accessor.max must be defined for POSITION attribute accessor.

We can obviously add handling for this to accommodate non-compliant assets but it seems like it would be better to fix the model in question, as my understanding is that VRM models need to be glTF 2.0 compliant?

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 7, 2019

I guess @takahirox can help here since the loader and example were introduced via #13877.

Maybe that's a good opportunity to update the model itself since it was a bit controversial^^ #13877 (comment)

@zeux
Copy link
Contributor Author

zeux commented Dec 7, 2019

Sounds good. If we need to support this in the loader it's simple, just need an early-out similar to the one we have for POSITION.

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.

GLTFLoader: Provide geometry.boundingBox and geometry.boundingSphere

3 participants