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

gltfpack produces broken result for skinned parts of mesh #433

Open
hybridherbst opened this issue Jul 9, 2022 · 7 comments
Open

gltfpack produces broken result for skinned parts of mesh #433

hybridherbst opened this issue Jul 9, 2022 · 7 comments

Comments

@hybridherbst
Copy link

  1. Download https://github.com/prefrontalcortex/glTF-Sample-Models/raw/carbon-frame-bike/2.0/CarbonFrameBike/glTF-Binary/CarbonFrameBike.glb
  2. See reference images at https://github.com/prefrontalcortex/glTF-Sample-Models/tree/carbon-frame-bike/2.0/CarbonFrameBike
  3. Run gltfpack -i CarbonFrameBike.glb -o CarbonFrameBike.packed.glb
  4. Open the result in e.g. Babylon Sandbox or Gestaltor
  5. Note that the wires don't survive packing well:
    image

Reference:
image

Using -noq works around the problem, but I thought it would be worth still raising an issue since the result looks relatively broken with default parameters.

@zeux
Copy link
Owner

zeux commented Jul 17, 2022

fwiw this looks as if the default number of bits (14) gltfpack uses for quantization isn't sufficient for this model. In certain cases this can be avoided by recentering the mesh, which gltfpack doesn't do automatically, but sometimes it's just something that needs to be raised via -vp command line flag.

I'll keep this open for a bit because I need to check if gltfpack can improve this automatically by changing the mesh internally (which may be difficult for skinned model because right now gltfpack avoids doing any transforms on the skinned nodes due to three.js issues with culling), but using something like -vp 16 is likely to improve this noticeably.

@zeux zeux added the gltfpack label Jul 17, 2022
@zeux
Copy link
Owner

zeux commented Jul 31, 2022

Right, so:

  1. This can be improved via -vp 16
  2. This happens because the wires in question have a very large internal scale value (of 100 - this looks like an improper coordinate system conversion somewhere; probably the inverse scale of 1/100 was baked into the vertex data). Since quantization is done in local space, but using a single global quantization grid (relaxing this is difficult because eg several meshes can reuse skin objects and the quantization scale needs to be corrected via skinning transforms), the vertices end up being quantized with suboptimal values.

I think you don't see the issue in glTF-Transform because it's not quantizing the values, either because it just doesn't support quantization on skinned meshes, or because it requires an explicit command line instruction to enable that.

Either way, I don't think this can realistically be fixed in quantization code, and propagating the scale value "into" mesh vertices would result in different bone transforms which may be important for other applications.

@zeux zeux closed this as not planned Won't fix, can't repro, duplicate, stale Jul 31, 2022
@zeux
Copy link
Owner

zeux commented Jul 31, 2022

P.S. The node with an abnormally high scale (100) that causes all this is Armature_S1/S2/S3. Maybe this is an artifact of a Blender export judging by the name, or maybe the source model is just configured in an odd way.

@zeux
Copy link
Owner

zeux commented Jul 31, 2022

I guess the one thing that gltfpack could do is compute two quantization grids, one for skinned meshes and one for non-skinned meshes... but it's a little silly to do that as it assumes that all or none of the skinned meshes in the model are poorly quantized due to abnormal scale values. If this example comes up in the future that could be a possible option but for now it feels like this shouldn't be handled by gltfpack.

@donmccurdy
Copy link
Contributor

donmccurdy commented Oct 28, 2023

@zeux glTF Transform does quantize the vertex positions to 14 bits as well. I think the difference is that glTF Transform offers two quantization grids:

    --quantization-volume <volume>       Bounds for quantization grid.
                                         one of "mesh","scene", default: "mesh"

While 'mesh' may occasionally introduce seams where meshes are expected to align, I've found it mostly fine as a default. Comparing the two modes in glTF Transform:

mesh scene
Screenshot 2023-10-27 at 11 12 17 PM Screenshot 2023-10-27 at 11 10 12 PM

I've exposed that flag to the CLI on the standalone quantize command, but not the meshopt command, which was an oversight...

EDIT: To be fixed in donmccurdy/glTF-Transform#1144.

@zeux
Copy link
Owner

zeux commented Oct 28, 2023

Yeah, seams are one reason why I've avoided per-mesh quantization. There's also issues with volume thickness (... this is really a spec issue in hindsight but oh well) in that if two meshes use the same material that has the extension, they need to use the same quantization scale. This also applies to skinning - since skinning matrices need to be adjusted, using a single skin for two meshes with different grids must also tie them together.

This is all doable but cumbersome which is part of why I've been avoiding fixing this :)

@zeux
Copy link
Owner

zeux commented Oct 30, 2023

the one thing that gltfpack could do is compute two quantization grids, one for skinned meshes and one for non-skinned meshes

Upon closer inspection this doesn't solve this scene fully, as it has 2 unskinned meshes in addition to skinned meshes that have scale 100; they are smaller and harder to see but it would still be an issue.

So besides using -vpf the only way to solve this appears to be per-mesh quantization scale which requires a fairly large change and some thought as to whether this changes the defaults, which would need a lot of testing... I think this will not happen for the upcoming version as it's too significant of a change for a fairly core part of gltfpack, so I'm removing this from consideration for 0.20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants