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

BufferGeometry: Add ability to read interleaved data in computeVertexNormals(). #19600

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jun 9, 2020

From #18433. This modification helps to read interleaved data for normal computations. A new attribute is not interleaved however the method computes now data which are at least usable.

@WestLangley
Copy link
Collaborator

Should the title be BufferGeometry: support interleaved positions in computeVertexNormals()?

In other words, is that an accurate description of what this is doing?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 9, 2020

First, I've hesitated a bit since I wanted to avoid confusion. The PR adds no real interleaved buffer support. Just the ability to read interleaved data by using the generic getter and setters from the BufferAttribute API. But your title is indeed more descriptive. Changing...

@Mugen87 Mugen87 changed the title BufferGeometry: Make computeVertexNormals() more generic. BufferGeometry: Add ability to read interleaved data in computeVertexNormals(). Jun 9, 2020
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 9, 2020

I've slightly changed the title compared to yours since reading index data is now different, too.

@WestLangley
Copy link
Collaborator

The PR adds no real interleaved buffer support. Just the ability to read interleaved data

It seems to me you are supporting reading and setting (existing) interleaved data.

Should the title be BufferGeometry: support interleaved data in computeVertexNormals()?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 9, 2020

The problem is when the buffer attribute for normals needs to be created, data are not interleaved.

normalAttribute = new BufferAttribute( new Float32Array( positionAttribute.count * 3 ), 3 );

It would only work for existing normals hence it's not a real support :(.

@WestLangley WestLangley added this to the r118 milestone Jun 9, 2020
@mrdoob
Copy link
Owner

mrdoob commented Jun 9, 2020

Hmm, if this is not fixing it... wouldn't it be better to check if it's interleaved and log a warning that is not supported?

@WestLangley
Copy link
Collaborator

WestLangley commented Jun 9, 2020

@mrdoob He is fixing it. It is just that if the normal attribute does not exist, then non-interleaved normals are added. That is reasonable.

@mrdoob
Copy link
Owner

mrdoob commented Jun 9, 2020

Ah, I see! 👍

@mrdoob mrdoob merged commit feef82c into mrdoob:dev Jun 9, 2020
@mrdoob
Copy link
Owner

mrdoob commented Jun 9, 2020

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.

None yet

3 participants