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

Add parameter to quantize to allow using non-normalized bufferAttributes #662

Closed
dmliao opened this issue Aug 15, 2022 · 3 comments
Closed
Labels
feature New enhancement or request package:functions wontfix This will not be worked on

Comments

@dmliao
Copy link

dmliao commented Aug 15, 2022

Is your feature request related to a problem? Please describe.
Relevant problem filed with three-mesh-bvh: gkjohnson/three-mesh-bvh#448 -- looks like normalization of the attributes when using quantize is causing three-mesh-bvh to fail, because the same model quantized with gltfpack did not have any issues.

Describe the solution you'd like
Either for quantized meshes to have non-normalized attributes (except for normals, which I believe require them), or an option to set whether those attributes are normalized.

Describe alternatives you've considered
I've been following mrdoob/three.js#22874, and pulled that into a custom build of three.js to see if that'd fix the problem -- which it did not, so further investigation would be needed there even if accessing normalized attributes is supported in three.js. I feel that it'd be simpler to try to solve the problem from the transformation step, rather than in three.js

Additional context
I'm currently looking into this myself and editing gltf-transform to try to get this functionality in; I'm not too familiar with how quantization works, however, and so I'm struggling to get the scaling (on the parent of the mesh) to work with non-normalized attributes. If there's anything that I can help with / get help with here, please let me know!

@dmliao dmliao added the feature New enhancement or request label Aug 15, 2022
@donmccurdy
Copy link
Owner

donmccurdy commented Aug 15, 2022

Hi @dmliao, thanks for looking into this!

There are several attributes that require normalization — UVs1, vertex colors, normals, tangents, and skinning weights. So I think vertex positions are the odd attribute out, most others do need to be normalized. If we don't normalize positions this also results in some very large scales on SkinnedMesh armatures, which will exacerbate issues caused by mrdoob/three.js#18334.

The quantize() implementation is just not the simplest chunk of code, so I'm not especially keen to add more paths through it. I noticed gltfpack has added the option of normalizing position attributes as well: zeux/meshoptimizer#443

My preference would be to see mrdoob/three.js#22874 merged, or something like it — I don't know if three-mesh-bvh is using that code path, but it's (hopefully?) an easy fix to do so if not. Or possibly three-mesh-bvh could support normalized attributes directly; I'd be willing to help with that. Thoughts @gkjohnson?


1 Technically we aren't required to normalize UVs, but we need a texture.scale transform without normalization and this causes more problems than it solves, in my opinion.

@donmccurdy donmccurdy added this to the Backlog milestone Aug 15, 2022
@gkjohnson
Copy link

Or possibly three-mesh-bvh could support normalized attributes directly; I'd be willing to help with that. Thoughts @gkjohnson?

In my opinion this is something that should be fixed in three.js or otherwise users need to be aware of the implications of using normalized attributes. Regular raycasting doesn't even work with normalized attributes in three.js as it is.

If a solution like mrdoob/three.js#22874 looks like it won't be merged for some reason then I'm happy to consider solution baked into three-mesh-bvh. But "raycasting doesn't work at all without it" isn't a reason I feel like people should be using the project.

@donmccurdy
Copy link
Owner

It looks like this will be added in three.js r144!

@donmccurdy donmccurdy added the wontfix This will not be worked on label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New enhancement or request package:functions wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants