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

Examples: Clean Up #21449

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Examples: Clean Up #21449

merged 1 commit into from
Mar 12, 2021

Conversation

Dvvarf
Copy link
Contributor

@Dvvarf Dvvarf commented Mar 11, 2021

Related issue: -

Description

This simple fix changes two instances of data.count to count, which allows any BufferAttribute to be used with LineSegments2 and Wireframe classes, not only InterleavedBufferAttribute.

Description+

Method computeLineDistances (from classes LineSegments2 and Wireframe) expected instanceStart and instanceEnd to have data.count property, but this property only exists on InterleavedBufferAttribute class. At the same time InterleavedBufferAttribute exposes count property (passed-through data.count property) which corresponds to count property from BufferAttribute.

@WestLangley WestLangley added this to the r127 milestone Mar 11, 2021
@WestLangley WestLangley changed the title Examples: Fix error when not using InterleavedBufferAttribute Examples: Support both BufferAttribute and InterleavedBufferAttribute Mar 11, 2021
@WestLangley
Copy link
Collaborator

@Dvvarf This change is an improvement, but I'm wondering if it is correct to say the class now "supports BufferAttribute"...

Can you please explain what, exactly, are you doing in your app?

@Dvvarf
Copy link
Contributor Author

Dvvarf commented Mar 11, 2021

@WestLangley I agree that stating support for BufferAttribute is a bit much, I've only tried to use it with InstancedBufferAttribute. I'm not sure about that, but I believe it was intended to be only used with instanced buffers... Perhaps a check should be added for this.

For me using InterleavedBufferAttribute is not convenient as I has an array of points with array of line lengths and not looking to double my memory usage. Turned out that it was entirely possible to directly use LineSegmentsGeometry, manually setting it's instanceStart and instanceEnd attributes. That's when I tried to use InstancedBuffer and encountered an error in LineSegments2, because it doesn't have data property, but has count. Applying this quick fix solved the problem.

@WestLangley WestLangley changed the title Examples: Support both BufferAttribute and InterleavedBufferAttribute Examples: Clean Up Mar 11, 2021
@WestLangley
Copy link
Collaborator

Agreed, I do not think you are adding additional functionality.

@gkjohnson Does this change look OK to you?

@gkjohnson
Copy link
Collaborator

@gkjohnson Does this change look OK to you?

Yeah this looks good since it's just pass through. The raycast code already uses instanceStart.count so this should make things more consistent, as well.

Demos look good, too:

https://raw.githack.com/Dvvarf/three.js/dev/examples/webgl_lines_fat.html

https://raw.githack.com/Dvvarf/three.js/dev/examples/webgl_lines_fat_wireframe.html

@mrdoob mrdoob merged commit 629e1fd into mrdoob:dev Mar 12, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2021

Thanks!

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.

4 participants