BufferGeometry: Better Compatibility to InterleavedBufferAttribute #10296

Merged
merged 4 commits into from Dec 6, 2016

Projects

None yet

3 participants

@Mugen87
Contributor
Mugen87 commented Dec 6, 2016 edited

see #9711

This PR changed .computeBoundingBox() and .computeBoundingSphere() of BufferGeometry, so they work with InterleavedBufferAttribute. Instead of using a plain array for calculating bounding volumes, the implementation now uses the position attribute (BufferAttribute) and the new Box3 method .setFromBuffer().

@mrdoob
Owner
mrdoob commented Dec 6, 2016

Looks good. Maybe calling it .setFromBufferAttribute() would be more clear though?
Any ideas about what's the performance impact also?

@Mugen87
Contributor
Mugen87 commented Dec 6, 2016 edited

I like the method name .setFromBufferAttribute() 😊

The new implementation means that we have to execute three additional method calls (getX, getY and getZ) in order to receive a vertex in .setFromBufferAttribute(). TBH, i don't know how this actually affects the performance. But i assume this will be the tradeoff for a more generic implementation.

@Mugen87
Contributor
Mugen87 commented Dec 6, 2016

Is there a way we can measure this without much effort?

@mrdoob
Owner
mrdoob commented Dec 6, 2016 edited

Is there a way we can measure this without much effort?

You can just try doing adding console.time( 'computeBoundingBox' ) and console.timeEnd( 'computeBoundingBox' ) around a geometry.computeBoundingBox() call and reload the page a bunch of times 😅

@Mugen87
Contributor
Mugen87 commented Dec 6, 2016

OMG, of course 😁 . I'll try it out...

@Mugen87
Contributor
Mugen87 commented Dec 6, 2016 edited

I don't know why this happens, but the performance is better than before 😳

.setFromArray(): about 11ms
.setFromBufferAttribute(): about 3.5ms

Same result in FF and Chrome. Weird...

@WestLangley
Collaborator

Box3.setFromObject() supports interleaved-BufferGeometry. Should that be refactored?

@Mugen87
Contributor
Mugen87 commented Dec 6, 2016 edited

Box3.setFromObject() supports interleaved-BufferGeometry. Should that be refactored?

Good point! I think we could rewrite the logic so it doesn't matter if an instance of BufferGeometry consists of InterleavedBufferAttribute's.

@mrdoob
Owner
mrdoob commented Dec 6, 2016 edited

I don't know why this happens, but the performance is better than before 😳

.setFromArray(): about 11ms
.setFromBufferAttribute(): about 3.5ms

Same result in FF and Chrome. Weird...

Weird indeed!

@mrdoob
Owner
mrdoob commented Dec 6, 2016 edited

Box3.setFromObject() supports interleaved-BufferGeometry. Should that be refactored?

Good point! I think we could rewrite the logic so it doesn't matter if an instance of BufferGeometry consists of InterleavedBufferAttribute's.

👍

@mrdoob
Owner
mrdoob commented Dec 6, 2016

Merging this by now.

@mrdoob mrdoob merged commit bc3ebe9 into mrdoob:dev Dec 6, 2016
@mrdoob
Owner
mrdoob commented Dec 6, 2016

Thanks!

@mrdoob
Owner
mrdoob commented Dec 14, 2016

Does this close #9242 and related PRs?

@Mugen87
Contributor
Mugen87 commented Dec 14, 2016 edited

#9242 contains some changes that are not yet implemented. But i will try to split the pending code in separate, upcoming PRs. Then we can discuss the changes step by step...

@Mugen87
Contributor
Mugen87 commented Dec 14, 2016

PR #9275 can be closed.

@Mugen87
Contributor
Mugen87 commented Dec 15, 2016

PR #9277 is obsolete now, too.

@Mugen87
Contributor
Mugen87 commented Dec 20, 2016

PR #9278 can be close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment