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

[feat] allow GLBufferAttribute as an attribute type #314

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

mikkom
Copy link
Contributor

@mikkom mikkom commented Jan 6, 2023

This is the same change as #295, just retargeted against dev branch.

Why

The current typings don't allow setting GLBufferAttribute as an attribute in BufferGeometry. This functionality has been in Three.js for quite some time (originally introduced in mrdoob/three.js#13196 back in 2020).

Three.js master continues to support this feature in BufferGeometry (see e.g. https://github.com/mrdoob/three.js/blob/master/src/core/BufferGeometry.js#L371), documents the attribute type in https://threejs.org/docs/#api/en/core/GLBufferAttribute and has a usage example in https://github.com/mrdoob/three.js/blob/master/examples/webgl_buffergeometry_glbufferattribute.html#L115-L117.

What

This change adds GLBufferAttribute as a valid type for setAttribute and getAttribute methods in BufferGeometry. The type for GLBufferAttribute had already been defined and exported but it was not used anywhere else in the typings.

Note that the change can break consumers of the typings as GLBufferAttribute does not implement the same interface as BufferAttribute. The way to fix these issues is to use the boolean flags (isBufferAttribute, isInterleavedBufferAttribute, or isGLBufferAttribute) to discriminate between the different options (or alternatively by resorting to type assertions). Despite the potential breakage, I believe these typings are more correct.

Checklist

  • Checked the target branch (current goes master, next goes dev)
  • Added myself to contributors table
  • Ready to be merged

@0b5vr
Copy link
Contributor

0b5vr commented Jan 6, 2023

oh, thanks for retargeting the target branch

Comment on lines 64 to 65
readonly isInterleavedBufferAttribute: undefined;
readonly isGLBufferAttribute: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's undefined, we don't have to explicitly say this I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I just saw the message you left in the previous PR. #295 (comment)

If it's the solution I feel like we have to set isXXX to every Three.js class, and I don't think it's the right way.
(e.g. saying readonly isMesh: undefined in Matrix4 because someone might want to use that)
I believe people solve this kind of problems in other ways in the TypeScript realm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delayed response. I think you make a fair point there. It could indeed get unwieldy quite fast if we would need to cover all the fields in similar fashion. So I've now removed the explicit undefined field definitions and switched to using instanceof in the pointercontrols test.

@mikkom mikkom force-pushed the glbufferattribute-support branch from 0d8c83d to 47a2474 Compare January 10, 2023 13:42
@0b5vr 0b5vr self-requested a review January 11, 2023 01:56
Copy link
Contributor

@0b5vr 0b5vr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it looks good, 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.

2 participants