-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Bounding Boxes #507
Comments
@mlimper are you suggesting that we put this in a 1.0.1 spec or in an extension? Either way, do you want to draft something up? I could see adding something like this to "mesh" : {
"boundingVolume" : { // optional
"box" : // ... optional
"sphere" : // ... optional
}
} We are having a similar discussion for 3D Tiles: CesiumGS/3d-tiles#10 (comment) |
+1 for the idea, obviously practical. not sure if it should be base spec or extension |
I would vote for an optional property in the base spec At least for our use cases, we always use a center/size or min/max representation of an axis-aligned bbox. I think center/size might be better with regards to string precision. Maybe something like the following:
I would guess that having an AABB is the most common use case... having a bounding sphere or OOBB, one could also compute an enclosing AABB on the client side, so we would be fine that. |
My preference is center/size and I agree that AABB is the most common case for bounding boxes. Seems fairly harmless to also have bounding sphere. I am assuming that an implementation is free to ignore these, that's it's just an optimization for runtime? |
Base spec for 1.0.1 is OK with me.
Exactly. I imagine an implementation could also use them for collision detection too (Cesium is interested in this), especially for a node with just a bounding volume (e.g., an artist could add a node with a bounding volume just for collision detection). @mlimper I would suggest a small tweak to make the naming less redundant and more precise: "mesh" : {
"boundingVolume" : { // optional
"box" : { // optional
"center" : [1,2,3] //mandatory
"size" : [4,5,6] //mandatory
},
"sphere" : { // optional
"center" : [1,2,3] //mandatory
"radius" : 4 //mandatory
}
}
} I suggest making If an animation targets a node's TRS, the bounding volume is also affected. When we all agree, I can help with the spec. |
Agreed, looks great! |
@tparisi any other thoughts here? If we are good with this, I can make a |
@pjcozzi |
@lexaknyazev this would be good to consider for 1.0.1. Do you have the bandwidth to proposal the spec/schema changes in a pull request? |
Otherwise, if the 1.0.1 scope is becoming too much, we can push this. |
Just started wrapping this up and found that lack of bounding volumes was the main reason to make It's important to remember here, that bounding volumes are defined for I can see several cases of possible bounding volume to mesh/node relations.
@pjcozzi @mlimper @lasalvavida What do you think? |
I don't think we really need to worry about this too much. It would the job of validators and optimizers to check for and correct this. Implementations should be able to assume the glTF is correct.
I'm not following here. Nodes don't have geometry, so their bounding volume doesn't exist if there are no meshes. In general, I agree that associating a bounding volume directly with a mesh is more ideal than requiring the runtime to check against the accessors based on attribute semantics. However, I do think maybe we need to decide on either a bounding sphere or a bounding box, having both seems like it could be complicating for implementations. |
This is exact reason why I'm asking. Does it make asset invalid?
As @pjcozzi said above:
|
Gotcha, thanks.
I would say that a mesh with points outside of its bounding volume would be considered invalid and need correcting. However, the bounding volume does not need to be a tight fit for the points. I would be tempted to enforce something similar on nodes (a parent's bounding volume must at least contain the bounding volumes of its children). |
Checking/enforcing this can be complicated in case where only root It's also worth noting here that we can compute/check/use bounding volumes only when |
@lexaknyazev at this point, I suggest we push this to 1.1 to keep 1.0.1 focused on validation. We, of course, can continue to discuss this now. What do you think? |
In the context of the first post of this issue, do we all agree that presence of bounding volumes has no effect on necessity of |
Yes, I see them as orthogonal...no pun intended. |
@pjcozzi Could you push this to 1.1? |
Yes, pushed. |
Small note: A bounding box does not have to enclose all points. Consider a transparent cutout, in which case one vertex can be outside the bounding box. For visibility, it can be still safely ignored because it would end up transparent anyway. Bounding boxes thus do make sense as a separate entity as they cannot be inferred for all cases (for most cases, the min/max of the position will suffice.) That said, I'd suggest to require either a bounding box or sphere, and optionally allow more complex bounding volumes (OBBs.) I'd suggest to not mix this with collision volumes which will be usually much more complicated (and would probably reference a separate mesh.) |
Great points, thanks @Anteru! |
We are open to an extension here if this is a priority for anyone in the community to design and implement; meanwhile, closing for the time being to tidy up this repo. Also see #1478 for world-space bounding boxes. |
We actually generate this now for all the glTF that Facebook generates (well, at least the code that I'm responsible for): "scenes": [
{
"nodes": [
0,
1
],
"extensions": {
"FB_geometry_metadata": {
"vertexCount": 9749,
"primitiveCount": 15292,
"worldBounds": {
"min": [
-237.65625,
-10.894416809082,
-260
],
"max": [
6.1412029266357,
70.546867370605,
-1
]
}
}
}
}
], I've punted on writing up a spec for this, since it doesn't address many valid concerns, e.g. it's only for static geometry before any animation is applied. But it sounds like at least making the PR is worthwhile, if only to provoke more in-depth discussion. |
I'm kind of surprised that there's still no bounding box support in glTF 2.0 (as I understand it, anyway). Are you guys waiting for someone to implement an extension? We might make our own, and I suppose we could submit it -- but it may not be exactly what everyone wants. |
I think part of the issue is precisely that — while it’s easy to say “bounding box” the fact is everyone needs something a little different. Does it get calculated per scene? Every node? Is it aware of static skinning pose? Animations? We put together a very simple extension here, if it’s helpful. But are you also saying glTF assets should be required to include bounding data to be compliant? Encouraged? |
I believe they should. Always computing bboxes at runtime during loading is a big waste of power most of the time. Sure, people might want more, or different from the spec, and that's what extensions are good for. But the spec should require something, or specifically opting out if bboxes don't "make sense" for the object. |
Having bounding box information is essential for loading and rendering of glTF scenes. Given this, it seems a bit odd that, currently, the bounding box of the model must be indirectly defined via the POSITION attribute.
One drawback of the current approach is that one could use custom shaders / attributes that encode the vertex positions differently (see WEB3D_quantized_attributes extension), and therefore have min/max values that differs from the bounding volume of the respective primitive / mesh - using min/max as bounding box would not work this way.
Also, with the current spec, reader / rendering implementations actually will have to find attributes of a mesh that correspond to the POSITION semantics, and then figure out the min/max values of that attribute. Even if we leave aside the aforementioned issue with the encoding of vertex positions, this seems like a error-prone process, and likely not all implementations will adhere to it: Instead, chances are high that implementers just try to use min/max of a "position" / "positions" / "POSITION" / "pos" / ... attribute, without checking the semantics, which makes the situation even worse, since this would assume a fixed naming of attributes in order to get bounding box information.
To solve these problems, it would probably be very useful to have (optional) bounding box definitions for "mesh" and / or "primitive".
It could also be useful to have bounding volumes inside the scene graph. The node doesn't have a volume property yet, it seems, just transformations.
The text was updated successfully, but these errors were encountered: