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

Line: Add support for interleaved geometries in raycast(). #20366

Merged

Conversation

manthrax
Copy link
Contributor

Correctly compute stride and offset for reading vertices from InterleavedBufferAttribute.

#20365

Correctly compute stride and offset for reading vertices from InterleavedBufferAttribute
Fix lint errors.
@manthrax manthrax changed the title Update Line.js Update Line.js - Fix raycasting for Lines based on InterleavedBufferAttributes Sep 17, 2020
@gkjohnson
Copy link
Collaborator

Does it make sense to use Vector3.fromBufferAttribute here instead? Then it's agnostic to the underlying attribute type.

@manthrax
Copy link
Contributor Author

Yup that makes great sense.

@manthrax
Copy link
Contributor Author

manthrax commented Sep 17, 2020

@gkjohnson I looked into the implementation of fromBufferAttribute, and it looks like it involves more function calls and index multiplies than computing the indexing inline.. and this is a pretty performance critical piece of code, and am leery of changing the runtime characteristics. I already agonized over the 2 tests of attributes.position.isInterleavedBufferAttribute but I figure the jit can optimize it trivially. I'm not as confident that it will inline the operations invoked by fromBufferAttribute..

fromBufferAttribute calls:
attribute.getX( index ) <- returns this.array[ index * this.itemSize ]
attribute.getY( index ); <-returns this.array[ index * this.itemSize + 1 ]
attribute.getZ( index ); <-returns this.array[ index * this.itemSize + 2 ]

whereas the inline version only has to do index * this.itemSize once, and then fetches consecutive vertices.

on a 1million point/line cloud that could be 2m more multiplies and 3m function calls on a raycast...

Of course I could be wrong and the jit inlines all of it no problem, but on less capable browsers it may still be relevant.

Thoughts?

@usefulthink
Copy link
Contributor

Here's the alternate version using fromBufferAttribute(): dev...usefulthink:patch-1

Now, I'm not sure if we will ever see any more types of BufferAttributes (only I could think of would be float64-emulation like deckgl does it), in which case the version using fromBufferAttribute would be preferrable. I think something could be done to improve the perfomance in that implementation (maybe adding BufferAttribute.toVector3(index, targetVector) or something like that.

Also the js raycaster already isn't very high-performing to begin with and I think most people dealing with larger geometries will go for alternatives like GPU picking anyway.

@manthrax
Copy link
Contributor Author

Sounds good to me. :)

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2020

Please use Vector3.fromBufferAttribute(). It's not acceptable that interleaved buffer logic leaks out to the raycasting code.

@mrdoob mrdoob added this to the rXXX milestone Sep 17, 2020
@Mugen87 Mugen87 changed the title Update Line.js - Fix raycasting for Lines based on InterleavedBufferAttributes Line: Add support for interleaved geometries in raycast(). Sep 17, 2020
@manthrax
Copy link
Contributor Author

@Mugen87 Can you take @usefulthink s patch as is? If it's more helpful to you, i can update this PR...

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2020

I guess it's easier if you just update the PR 👍

@manthrax
Copy link
Contributor Author

Updated PR.. thanks @Mugen87, @usefulthink. :)

@Mugen87 Mugen87 modified the milestones: rXXX, r121 Sep 18, 2020
@mrdoob mrdoob merged commit 271a103 into mrdoob:dev Sep 20, 2020
@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2020

Thanks!

@manthrax manthrax deleted the fix-raycast-line-interleavedbufferattribute branch September 29, 2020 01:01
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.

None yet

5 participants