Box3: Use index in Box3#setFromObject #28327
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
I agree with this change. For the sake of memory efficiency I think it would be good to more freely share attribute date. Not to make this more complex but there's a question of whether or not bounding boxes should account for
I'd be interested in what a full list of functions that needs to change looks like. At least I wonder if it's worth making some utility functions to make this easier, though I'm not immediately sure what those would look like. I often find myself iterating over just the attributes that are referenced by the indices. |
Related #8690. Honor indices, groups or draw ranges all share the same goal, to make the bounding volumes more tight in certain use cases. They all affect the draw start and count properties and I can see why adding support makes sense. However, all three attributes serve different purposes. Indices usually do not change similar to other vertex data. Groups enable multi material support and should not affect the overall draw count or start of the entire geometry. And draw range is (at least for me) a dynamic property that might change temporarily or over time. Honoring each property to optimize bounding volumes comes with the cost of additional code complexity. I'm willing to support this PR (or in general the idea to honor the index attribute in bounding volumes) but I would welcome to officially exclude support for groups and draw ranges. |
I agree which is why a utility function may be helpful to reduce code redundancy here. Though this likely comes at a non zero performance cost: geometry.forEachUsedVertexIndex( index => {
// iterate over only used indices in groups, draw range, index buffer
} )
// or
for ( const index of geometry.getUsedVertexIndices() ) {
// generator approach
}Either way these changes result in likely processing the same vertex index multiple times with indexed geometry, as well, which will impact perf.
Can you explain your use cases for draw range? I'm wondering if the value would have changed with more recent additions to three like instance and batched geometry. I've only used drawRange my CSG project to avoid recreating geometry to resize buffers and improve performance (which I think is a nice use case) but it does mean that bounds should update based on draw range and it's an issue right now that they don't. Don, apologies if this is outside the scope of the feedback you were interested in 😅 |
Animated lines was typical use case for me. Updating the bounding volumes per animation step is too costly so at least in my use cases it was more performant to accept larger bounding volumes. |
That's mainly what I had in mind – there might be some other places e.g. in raycasting.
I think these are OK as-is. We're generating a new attribute and it needs to have the length of the existing attributes. If the user wants to mutate the geometry without the overhead, something like the proposal of
Yes, this and other procedural geometry. We can support both indexed an non-indexed iteration, and the code remains pretty simple. The index cannot grow, so if a geometry starts out using only 10% of the vertices, it will never use more than that, only less with draw range... so I think there's a strong argument that the indices should be used here. I think ideally draw range would be respected by these methods, but it's beyond my intentions right now, and I'm a bit worried about the overhead of putting abstractions and callbacks around vertex iteration, I'm not sure on this. I don't feel that |
Either way this is an improvement but aren't these cases where you specifically don't want to rely on automatic bounds generation though and instead set it based on the known bounds of the animation?
I know users won't always respect the docs but groups explicitly are disallowed from overlapping. Right now raycasting will return two intersections if groups overlap. From the docs:
The "must not leave vertices or indices unused" is new to me and doesn't seem necessary but this is a digression 😅.
Just to be clear we will be iterating over some vertices twice (or a dozen times) when using indexed geometry due to them being referenced multiple times. Though understood you may mean iterating over the index range multiple times. |
By procedural I don't meant to imply animation – I remember a TiltBrush-like app using drawRange to allocate geometry for brushstrokes in advance, for example. You'd want the bounding box to be computed based on the draw range, and it's easy enough to recompute the bounding box when necessary (e.g. after each brush stroke). But somewhere we do have to draw the line ... if it's nice-to-have for drawRange but simply too much complexity/overhead and should be managed by the user I am OK with that too.
True, and true without indices as well... but maybe more importantly, I do not think |
Related:
Ideally, three.js would not be constrained to >=1 buffer per geometry. Buffer attributes or interleaved buffers could be shared between multiple geometries. See #27926 and #17089.
One relevant limitation today is that
Box3#setFromObjectand other methods iterate over buffer attributes without using the index. Doing so is slower and will overestimate the bounds, when the mesh uses only a small subset of the vertices in the buffer.If this approach looks OK, I'd like to make similar PRs for
BufferGeometry#computeBoundingBoxetc./cc @takahirox @gkjohnson