Skip to content
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

Support InterleavedBufferAttributes #147

Closed
4 tasks done
gkjohnson opened this issue Sep 20, 2020 · 7 comments
Closed
4 tasks done

Support InterleavedBufferAttributes #147

gkjohnson opened this issue Sep 20, 2020 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@gkjohnson
Copy link
Owner

gkjohnson commented Sep 20, 2020

Every for loop over attribute arrays needs to account for attribute.stride (default to attribute.itemSize) and attribute.offset (defaulting to 0).

Change from something like this:

const position = geo.attributes.position.array;
const index = geo.index.array;
const triCount = index.length / 3;
const triangleBounds = new Float32Array( triCount * 6 );

for ( let tri = 0; tri < triCount; tri ++ ) {

    const i0 = tri * 3 + 0;

    // ...

    const vx0 = position[ i0 * 3 + 0 ]
    const vy0 = position[ i0 * 3 + 1 ]
    const vz0 = position[ i0 * 3 + 2 ]

    // ...

}

to

const positionAttr = geo.attributes.position;
const indexAttr = geo.index;
const triCount = indexAttr.count / 3;
const triangleBounds = new Float32Array( triCount * 6 );

const index = indexAttr.array;
const position = positionAttr.array;

const indexStride = indexAttr.stride || indexAttr.itemSize;
const indexOffset = indexAttr.offset || 0;

const positionStride = positionAttr.stride || positionAttr.itemSize;
const positionOffset = positionAttr.offset || 0;

for ( let tri = 0; tri < triCount; tri ++ ) {

    const i0 = ( tri * 3 + 0 ) * indexStride + indexOffset;

    // ...

    const vx0 = position[ ( i0 * 3 ) * positionStride + positionOffset + 0 ]
    const vy0 = position[ ( i0 * 3 ) * positionStride + positionOffset + 1 ]
    const vz0 = position[ ( i0 * 3 + 2 ) * positionStride + positionOffset + 2 ]

    // ...

}

(performance and behavior should be verified)

TODO

  • Add InterleavedBufferAttribute test
  • Remove error log for interleaved position buffer attributes

Locations to Update

  • computeTriangleBounds function (uses index, position buffers)
    • using the built in getters increases benchmark from ~170ms to ~270ms
  • refit (position, index)
    • increases benchmark w/o hints from ~13ms to ~17ms
    • with hints from ~0.6ms to ~0.7ms

Index Only

  • ensureIndex function (uses position)
  • buildTree function (uses index)
  • partition (index -- the most complicated / problematic case because it tightly sorts values)
    • See how perf is impacted?
    • create a temporary buffer before copying back?
    • provide option to create a duplicate index buffer for this purpose?
    • require users to de-interleave the index buffer?
    • Might be okay to just not support interleaved index buffer to start.

Correctly Used

  • intersectsGeometry function (uses position)
  • closestPointToGeometry (position, index)
  • ThreeIntersectionUtilities
  • getRootRanges (index)
@gkjohnson gkjohnson added the enhancement New feature or request label Sep 20, 2020
@gkjohnson gkjohnson added the help wanted Extra attention is needed label Mar 5, 2021
@Ben-Mack
Copy link

+1 Really hope support for InterleavedBufferAttributes would land on the next release.
I'm trying to integrate three-mesh-bvh but got stucked because it isn't compatible with Meshopt (an incredible compression codec btw) which uses InterleavedBufferAttributes.
About performance, I don't know much but I think even if bvh performance for InterleavedBufferAttributes is not as good as normal BufferAttr, it will still be much better than non-bvh at all.

@gkjohnson
Copy link
Owner Author

I'd be happy to take a contribution that implements this and adds tests for interleaved attributes but unfortunately I don't have the time to work on this myself -- I just have too much else on my list. If you (or anyone else) are interested in working on this I'd recommend working off of the v0.4.0 branch which I'll hopefully be able to merge within the next few weeks and contains some significant changes to the BVH generation code.

For this feature I'd want to ensure current benchmark timings minimally change if at all. Ideally there wouldn't have to be multiple code paths to support interleaved buffer attributes, too, but if the changes I suggested above lead to performance degradation I'd want to consider alternative solutions so that should be tested first.

@gkjohnson
Copy link
Owner Author

gkjohnson commented Jul 17, 2021

@Ben-Mack I was able to add support for interleaved position buffer attributes without degrading performance so that will be available in the next release. Interleaved index buffers are still not supported because it's a bit more complicated to add. Do any of your GLTF models use interleaved index buffers? The test compressed GLTF file in the three.js repo only includes interleaved position buffers so it's not clear to me if interleaved index buffers are all that common.

If they're not then I'll plan to close this for v0.4.2.

@Ben-Mack
Copy link

Thanks for the update!

Maybe the model in this MeshOpt example is the one you looking for.
Or you could generate one by using gltfpack with -cc flag: https://github.com/zeux/meshoptimizer/tree/master/gltf

@gkjohnson
Copy link
Owner Author

Yes I'm aware of the three.js MeshOpt example and have already tested with that. But you wanted this feature so I'm asking if you could check the models you had problems with and let me know if they use interleaved buffer attributes for the index buffer.

@Ben-Mack
Copy link

@gkjohnson Sorry for misunderstanding the prev question.

I've tested on various models compressed using MeshOpt and three-mesh-bvh works perfectly fine.
I haven't seen any interleaved buffer attributes used for index.
Only position and normal are using Interleaved type, uv and index use normal BufferAttribute

Again, Thanks a lot for this update!

@gkjohnson
Copy link
Owner Author

Great thanks! I'll close this, then, and if people need interleaved index attributes they can submit a new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants