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

GLTFExporter: Ensure normalized normal attribute #13767

Merged
merged 3 commits into from
Apr 7, 2018

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Apr 4, 2018

This PR lets GLTFExporter ensure normalized normal attribute. See #11951 (comment)

I still need to evaluate to know if this change (quite) slows the performance because this's our first computation with each vertex but let me first share the PR with you. You can test by yourselves, too, if you want.

@fernandojsg Can you share the model file on which this change can affect the performance if you have? I tested some standard models but I didn't see performance degradation (from same to just a few percents worse).

@WestLangley
Copy link
Collaborator

Normals in three.js are assumed to have unit length. The library does not validate inputs, however.

We already have BufferGeometry.normalizeNormals(). In that method, zero-vectors are left unchanged. Zero-vectors are considered a user error.

@takahirox
Copy link
Collaborator Author

takahirox commented Apr 4, 2018

We already concluded in another thread that GLTFExporter should ensure exporting valid glTF even if model has unexpected values, like zero-vectors or non-unit length normal.

@fernandojsg
Copy link
Collaborator

Looking good so far, just two requests:

@takahirox
Copy link
Collaborator Author

takahirox commented Apr 4, 2018

I'd add a checkNormalizedNormals (or so) parameter :)

I wanna think that option if we see the actual use case/model where the checking (quite) slows the performance. I still a bit doubt the checking does... So evaluating first.

What about checking just the normals that we're going to actually export?

If we do that, perhaps it might be better to check/ensure in processBuffer(View) as you first suggested. Let me think about it...

@fernandojsg
Copy link
Collaborator

fernandojsg commented Apr 4, 2018

I wanna think that option if we see the actual use case/model where checking (quite) slows the performance. I still a bit doubt checking does... So evaluating first.

You are right, I just did a quick test with a big enough buffer and it was quick to normalize:

var geometry = new THREE.BufferGeometry();
var numElements = 10000000;
var positions = new THREE.BufferAttribute( new Float32Array( numElements * 3 ), 3 );
positions.dynamic = true;
geometry.addAttribute( 'position', positions );

var normals = new THREE.BufferAttribute( new Float32Array( numElements * 3 ), 3 );
normals.dynamic = true;
geometry.addAttribute( 'normal', normals );
geometry.normalizeNormals();

@mrdoob mrdoob added this to the r93 milestone Apr 4, 2018
@fernandojsg
Copy link
Collaborator

I've just tried with a-painter and works as expected 👍

@takahirox takahirox changed the title GLTFExporter: Ensure normalized normal attribute (WIP) GLTFExporter: Ensure normalized normal attribute Apr 5, 2018
@takahirox
Copy link
Collaborator Author

Cool.

Let's think those two options if/when we see this checking actually affects the performance.

@mrdoob mrdoob modified the milestones: r93, r92 Apr 7, 2018
@mrdoob mrdoob merged commit 79872ad into mrdoob:dev Apr 7, 2018
@mrdoob
Copy link
Owner

mrdoob commented Apr 7, 2018

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

4 participants