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

BufferAttribute: Support (de)normalization in accessors. #22874

Merged
merged 3 commits into from
Aug 17, 2022

Conversation

donmccurdy
Copy link
Collaborator

Related:

Currently normalized attributes do not work correctly with raycasting, and perhaps other logic. There are a few possible way to fix that, narrowly or broadly:

  1. Handle denormalization narrowly in Mesh.raycast
    • Fixes raycasting and nothing else
  2. Handle denormalization narrowly in VectorX.fromBufferAttribute
    • Fixes raycasting and perhaps some other cases
    • Means vector.fromBufferAttribute is not consistent with attribute.getX
  3. Handle denormalization/normalization in all BufferAttribute getters/setters
    • Fixes any usage that doesn't access attribute.array directly. Avoiding direct access to attribute.array is already necessary for code that intends to support interleaved attributes, so a similar rule for normalized attributes seems reasonable.

As shown in this PR, (3) does require some more code than the alternatives. Do we have a preference among these?

Sample model, with normalized and interleaved position attribute:

DH.glb.zip

/cc @gkjohnson

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 23, 2021

Currently normalized attributes do not work correctly with raycasting, and perhaps other logic.

Another place where denormalization is currently required is the new morph target implementation.

function denormalize( morph, attribute ) {
let denominator = 1;
const array = attribute.isInterleavedBufferAttribute ? attribute.data.array : attribute.array;
if ( array instanceof Int8Array ) denominator = 127;
else if ( array instanceof Int16Array ) denominator = 32767;
else if ( array instanceof Int32Array ) denominator = 2147483647;
else console.error( 'THREE.WebGLMorphtargets: Unsupported morph attribute data type: ', array );
morph.divideScalar( denominator );
}

Buffer attribute data might be normalized so it's necessary to denormalized the data for the morph texture.

I'm glad this issue is discussed since putting normalization/denormalization all over the code base is not ideal. I think BufferAttribute would be the best place for such a logic.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 23, 2021

Avoiding direct access to attribute.array

We should establish this as a policy and clean up all places in the code base that access the array property directly.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Nov 24, 2021

I prefer (3), as well, so dealing with reading and writing bufferAttribute data is consistently handled -- that's what I would have advocated for.

and perhaps other logic

BufferGeometryUtils mergeVertices probably doesn't work correctly at the moment with normalized attributes. And I would think this would make merging multiple geometries easier, as well, when they don't share a common normalized setting for an attribute between geometries since you can rely on getters correctly returning the expected "float" result. That might be relevant for BatchedMesh, too, though you'd still have to deal with which canonical representation to use when encountering conflicts.

My only question is what, if any, performance implication does this have when using the getters and setters? With and without the normalized field set?

src/math/MathUtils.js Outdated Show resolved Hide resolved
@donmccurdy
Copy link
Collaborator Author

My only question is what, if any, performance implication does this have when using the getters and setters? With and without the normalized field set?

Will try to benchmark that – assuming the difference in a tight loop is small, and Int32Array and other types are added, would there any objections to going ahead with (3) as shown in this PR?

@gkjohnson
Copy link
Collaborator

assuming the difference in a tight loop is small, and Int32Array and other types are added, would there any objections to going ahead with (3) as shown in this PR?

Option 3 is how I was thinking of solving this problem when I first noticed it and I think it's the most elegant and straight forward. I've only done a cursory look at the PR but at a glance it looks right. I would think the docs should be updated, as well, to communicate that the values passed into these functions get / set the non-quantized values as you'd use in a shader rather than the values that are necessarily directly stored in the buffer.

@elalish
Copy link
Contributor

elalish commented Apr 6, 2022

This looks great to me. Any chance we can get it updated and merged? It will also fix boneTransform, which will allow #22742 to work properly without my weird workarounds.

@donmccurdy
Copy link
Collaborator Author

I regret that this is +200 LOC in the main build, and also a breaking change if someone is already handling normalized attributes correctly. I guess the 2nd part is probably OK since this is a common gotcha.

Any ideas to trim down the size though? We could omit the if ( this.normalized ) ... checks and always call normalize/denormalize, would just have to see if it makes a difference in hot code.

src/math/MathUtils.js Outdated Show resolved Hide resolved
@donmccurdy
Copy link
Collaborator Author

@elalish
Copy link
Contributor

elalish commented Jul 19, 2022

@Mugen87 Since we decided to go with this approach, should we go ahead and merge this?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 19, 2022

@elalish Merging the PR is fine. I just want to defer the merge to @mrdoob since he should make the final design decision. I'm not sure how he thinks about #24087.

@donmccurdy It would be great if you could include WebGLMorphtargets in this PR. The following lines should not be necessary anymore since the denormalization now happens automatically when fromBufferAttribute() is executed.

if ( morphTarget.normalized === true ) denormalize( morph, morphTarget );

if ( morphNormal.normalized === true ) denormalize( morph, morphNormal );

if ( morphColor.normalized === true ) denormalize( morph, morphColor );

The denormalize() function in WebGLMorphtargets can also be removed then.

Clean up WebGLMorphtargets.js

BufferAttribute: Add normalization support for u32, i32, and UintClampedArray
@donmccurdy donmccurdy force-pushed the feat-normalized-accessors branch from af2d7e4 to c6a80f9 Compare August 16, 2022 01:17
@donmccurdy donmccurdy marked this pull request as ready for review August 16, 2022 01:26
@donmccurdy
Copy link
Collaborator Author

I think this is ready for review. I'm still happy to refactor it other ways, but note the prior discussion in #24087.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Aug 17, 2022

I think we should merge this in the next release. Users are complaining about three-mesh-bvh not supporting normalized positions when it's really an issue with three.js (see gkjohnson/three-mesh-bvh#448) I feel the approach in this PR is clean and can be improved in the future if needed. This issue is becoming more and more noticeable with common compression tools like gltf-transform now producing normalized data for values that are typically not, like positions.

Thanks for addressing this @donmccurdy 😁

@mrdoob mrdoob added this to the r144 milestone Aug 17, 2022
@mrdoob mrdoob merged commit e4f5e0f into dev Aug 17, 2022
@mrdoob
Copy link
Owner

mrdoob commented Aug 17, 2022

Thanks!

@gkjohnson
Copy link
Collaborator

Added to the migration guide:

https://github.com/mrdoob/three.js/wiki/Migration-Guide#143--144

@donmccurdy
Copy link
Collaborator Author

FYI @hybridherbst — with this change you should have better luck using meshopt compression and raycasting in r144.

@hybridherbst
Copy link
Contributor

Ooh, thanks for pinging me! That's great :)
Also bubbling up to @elalish related to google/model-viewer#3430

@elalish
Copy link
Contributor

elalish commented Aug 23, 2022

Indeed; I'm waiting with bated breath to pull in the new version once it's out.

@repalash
Copy link
Contributor

Hello @donmccurdy, since getX, getY, getZ functions are used in GLTFExporter (and maybe collada exporter), is there a separate PR/issue for updating those usages?

@donmccurdy
Copy link
Collaborator Author

Most uses of getX, getY, and getZ will not need to change. GLTFExporter does not yet support KHR_mesh_quantization (#20474), and that's the most common use case for normalized attributes in glTF. But I think you're right that the usage in GLTFExporter may need to be updated for normalized attributes – there is no issue or PR tracking this yet.

snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 13, 2022
* BufferAttribute: Support (de)normalization in accessors.

Clean up WebGLMorphtargets.js

BufferAttribute: Add normalization support for u32, i32, and UintClampedArray

* Revert changes to MathUtils

* Clean up Color.js
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* BufferAttribute: Support (de)normalization in accessors.

Clean up WebGLMorphtargets.js

BufferAttribute: Add normalization support for u32, i32, and UintClampedArray

* Revert changes to MathUtils

* Clean up Color.js
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 20, 2022
* BufferAttribute: Support (de)normalization in accessors.

Clean up WebGLMorphtargets.js

BufferAttribute: Add normalization support for u32, i32, and UintClampedArray

* Revert changes to MathUtils

* Clean up Color.js
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
* BufferAttribute: Support (de)normalization in accessors.

Clean up WebGLMorphtargets.js

BufferAttribute: Add normalization support for u32, i32, and UintClampedArray

* Revert changes to MathUtils

* Clean up Color.js
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 4, 2022

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.

7 participants